[Webkit-unassigned] [Bug 91850] add 7 bit strings capabilities to the v8 binding layer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91850





--- Comment #34 from Dan Carney <dcarney at google.com>  2012-10-18 15:50:54 PST ---
(From update of attachment 169406)
View in context: https://bugs.webkit.org/attachment.cgi?id=169406&action=review

>> Source/WebCore/bindings/v8/V8PerIsolateData.cpp:116
>> +                base = static_cast<WebCoreStringResource16*>(resource);
> 
> This code looks very strange.  Why is it important to use the right static cast?  Presumably WebCoreStringResourceBase is the only base class for WebCoreStringResource8 and WebCoreStringResource16, which means these static casts should be zero offsets.  We can add an ASSERT to that effect.

The hierarchy is a little wierd here because we computing offsets between 2 different base classes with respect to 4 subclasses.  I expect the offsets are the same because of the similarity of ExternalAsciiStringResource and ExternalStringResource, but the offsets are definitely not 0.  I can add an ASSERT to verify the sameness, but I was assuming the compiler would remove the branch if they happened to be equal.

>> Source/WebCore/bindings/v8/V8StringResource.cpp:82
>> +    static const int inlineBufferSize = 16;
> 
> Is this constant related to something intrinsic to AtomicString?  if so, can we get the constant from AtomicString rather than having to duplicate it here?

I didn't write this, but I expect there is a performance gain up to a point doing everything on the stack, and the original writer just guessed at a number that sounded reasonable.  It has nothing to do with AtomicString.

>> Source/WebCore/bindings/v8/V8StringResource.cpp:100
>> +    v8String->WriteAscii(reinterpret_cast<char*>(buffer), 0, length, v8::String::PRESERVE_ASCII_NULL);
> 
> The NULL in this constant got me thinking.  Are we getting things correct w.r.t. null termination?  For example, the buffer in String isn't null terminated.  If V8 is trying to write a null terminator, the lengths will be off by one.  I guess that would show up in tests if we were goofing that up.

PRESERVE_ASCII_NULL is a bug fix for some bizarre legacy thing in v8 where '\0' was swapped out with ' '.  As far as null termination goes, v8 doesn't need it and doesn't add null terminators, just like webcore

>> Source/WebCore/bindings/v8/V8StringResource.cpp:125
>> +        }
> 
> This code looks largely duplicated from V8PerIsolateData.cpp.  Is there no way to share this logic?
> 
> I also don't understand why we need this tricky static_cast logic (same question as above)

I was going to share the logic, but this particular block gets hit literally billions of times in certain dromeao tests, so i duplicated the code.  Otherwise there would need to be an additional branch here.

>> Source/WebCore/bindings/v8/V8StringResource.cpp:129
>> +    if (UNLIKELY(!length))
> 
> Did you measure this UNLIKELY as having a performance benefit?  It's most likely a no-op.

I didn't measure the performance impact, but because this code gets hit hard in the benchmarks (less so than the above block), i added it.  I figured it might swap the branch contents to put everything after the branch in starting at the next instruction, but i didn't bother to check.

>> Source/WebCore/bindings/v8/V8ValueCache.h:151
>> +        return static_cast<WebCoreStringResource16*>(v8String->GetExternalStringResource());
> 
> Should we add an ASSERT about the encoding of v8String?

this might no longer be used.  if so i'll drop it, but the original intention of the call was to check whether the resource existed, as v8 has no cheap way of checking so the assert would be incorrect.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.