Bug 91850 – add 7 bit strings capabilities to the v8 binding layer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dan Carney]

add 7 bit strings capabilities to the v8 binding layer

[Dan Carney]

Created attachment 153483 [details]
Patch

[WebKit Review Bot]

Attachment 153483 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/V8Binding.cpp:406:  The parameter name "resource" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dan Carney]

Created attachment 153485 [details]
Patch

[Kentaro Hara]

Comment on attachment 153483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153483&action=review

> Source/WebCore/ChangeLog:12
> +        No new tests.  Refactor - no new functionality.

(I've not looked into the patch in detail but) would you check performance impact? We want to confirm that this change won't regress binding performance. You can test binding performance by WebKit/PerformanceTests/Dromaeo/dom-*.html and WebKit/PerformanceTests/Bindings/*. (You can add a new one if needed.)

[Dan Carney]

Created attachment 153506 [details]
typical performance test results without patch

[Dan Carney]

Created attachment 153507 [details]
typical performance test results with patch

[Dan Carney]

(In reply to comment #4)
> (From update of attachment 153483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153483&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests.  Refactor - no new functionality.
> 
> (I've not looked into the patch in detail but) would you check performance impact? We want to confirm that this change won't regress binding performance. You can test binding performance by WebKit/PerformanceTests/Dromaeo/dom-*.html and WebKit/PerformanceTests/Bindings/*. (You can add a new one if needed.)




I've run the performance tests for you.  No obvious degradation, but the one place I suspect there could be performance degradation String::containsOnlyASCII() I intend to fix upstream, so that the cost of calling that will be gone.

[Kentaro Hara]

(In reply to comment #7)
> I've run the performance tests for you.  No obvious degradation, 

Great!

Is this just a noise?

    - without your patch: dom-attr= 7587.33618079 runs/s
    - with your patch: dom-attr= 7055.17692413 runs/s

[Kentaro Hara]

Anyway this patch looks really great. I believe this patch will definitely save memory but I'm not sure how much. It would be also helpful to investigate how much memory are saved by this patch in a couple of real-world websites (Google Docs, Google SpreadSheet, Gmail, facebook, twitter etc).

[Adam Barth]

Comment on attachment 153485 [details]
Patch

This patch is causing a large number of tests to fail.

[Dan Carney]

(In reply to comment #10)
> (From update of attachment 153485 [details])
> This patch is causing a large number of tests to fail.

The errors come from v8 ascii handling.  A bug has been filed with the v8 team.

[Dan Carney]

Created attachment 154067 [details]
Patch

[Dan Carney]

reuploaded patch with bug fix from v8

[WebKit Review Bot]

Comment on attachment 154067 [details]
Patch

Attachment 154067 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13323635

[Kentaro Hara]

Comment on attachment 154067 [details]
Patch

This is a big (wonderful) change. Before looking into details, I'd like to confirm a couple of things. Would you describe the following points in ChangeLog?

- Performance impact. (Was the dom-attr.html result just a noise? WebKit performance tests can just catch 10%~ regression. So it is important to manually make sure that a doubtful patch never regresses performance. Please check Bindings/* and Dromaeo/dom-*)

- Memory benefit. (How much memory is saved by this patch? Let's take a couple of real-world websites or artificial benchmarks and measure the memory benefit.)

- Test coverage. (You mentioned that existing tests are already testing the call path of this patch. Would you list up some of them instead of writing "No new tests. Refactor - no new functionality."?)

[Kentaro Hara]

Looks like you need to update the V8 DEPS of chromium-ews. (You can ask it in IRC. If tomorrow is OK, I can do it.)

(In reply to comment #16)
> Looks like you need to update the V8 DEPS of chromium-ews. (You can ask it in IRC. If tomorrow is OK, I can do it.)

The V8 update was reverted because it caused random crashes, so we'll have to wait a bit longer it seems :-/

[Kentaro Hara]

Comment on attachment 154067 [details]
Patch

OK. For now marking r- due to the build failure. (We do not just want to list up r? patches in the review list.)

[Dan Carney]

Created attachment 163109 [details]
patch for perfalizer

[WebKit Review Bot]

Attachment 163109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/bindings/v8/V8PerIsolateDat..." exit_code: 1
Source/WebCore/bindings/v8/V8StringResource.cpp:143:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/bindings/v8/V8StringResource.cpp:134:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/bindings/v8/V8ValueCache.h:146:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8ValueCache.h:147:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8ValueCache.h:164:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8ValueCache.h:165:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dan Carney]

Created attachment 163333 [details]
perfalizer patch

[Dan Carney]

Created attachment 163334 [details]
perf results of 163333

[Dan Carney]

Created attachment 169187 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169187 [details]
Patch

Attachment 169187 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14412150

New failing tests:
inspector/timeline/timeline-load.html

[WebKit Review Bot]

Comment on attachment 169187 [details]
Patch

Attachment 169187 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14390604

New failing tests:
inspector/timeline/timeline-load.html

[Eric Seidel (no email)]

Comment on attachment 169187 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169187&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests. Test coverage already extensive.

Since this should be a perf win, it seems appropriate to comment on how much of a win it is (in whatever example benchmarks you like) in the ChangeLog.

> Source/WebCore/bindings/v8/V8StringResource.cpp:42
> +    inline static String fromStringResource(WebCoreStringResourceBase* resource)

Should this return const String&?  it seems the resource always owns the String.

> Source/WebCore/bindings/v8/V8StringResource.cpp:135
>          return String("");

String::empty() is faster. :)

> Source/WebCore/bindings/v8/V8ValueCache.cpp:44
> +    if (string.is8Bit() && string.containsOnlyASCII()) {
> +        WebCoreStringResource8* stringResource = new WebCoreStringResource8(string);
> +        v8::Local<v8::String> newString = v8::String::NewExternal(stringResource);
> +        if (newString.IsEmpty())
> +            delete stringResource;
> +        return newString;
> +    }
> +
> +    WebCoreStringResource16* stringResource = new WebCoreStringResource16(string);

Can't we share this block by using a WebCoreStringResourceBase* ?  Or does it not have a virtual destructor?

> Source/WebCore/bindings/v8/V8ValueCache.h:87
> +    inline explicit WebCoreStringResourceBase(const AtomicString& string)

I was under the impression that marking things "inline" in headers was meanlingless?  I thought 'inline' was implied for anything which was listed as part of the class definition?

[Adam Barth]

(In reply to comment #26)
> (From update of attachment 169187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169187&action=review
> 
> > Source/WebCore/bindings/v8/V8StringResource.cpp:135
> >          return String("");
> 
> String::empty() is faster. :)

But not thread safe.  My understanding is that this code is used in workers as well.

[Dan Carney]

Created attachment 169406 [details]
Patch

[Dan Carney]

I've cleaned up the patch a bit.  It will fail until a v8 bug fix makes its way through the pipeline, but I'd still like another review so I can get this patch in the commit queue as soon as that happens. I have the feeling that this patch will get rolled back a few times, as it's using a bunch of v8 functionality never used before, and I'd like to work through that sooner rather than later.

> Since this should be a perf win, it seems appropriate to comment on how much of a win it is (in whatever example benchmarks you like) in the ChangeLog.

Okay, I forgot to add them in this patch, I'll add those in on the final submit. Some representative numbers are in https://bugs.webkit.org/attachment.cgi?id=163334, but like are perf numbers are probably somewhat wrong.

In particular:

Bindings/append-child
Bindings/create-element 
Bindings/id-getter
Dromaeo/jslib-style-jquery
Dromaeo/jslib-attr-jquery
Dromaeo/dromaeo-object-regexp
Dromaeo/sunspider-crypto-md5 

see some consistent improvement.  There are a few loses but they seem less drastic than the gains, and I have future patches to v8 which should mitigate them.

> > Source/WebCore/bindings/v8/V8ValueCache.cpp:44
> > +    if (string.is8Bit() && string.containsOnlyASCII()) {
> > +        WebCoreStringResource8* stringResource = new WebCoreStringResource8(string);
> > +        v8::Local<v8::String> newString = v8::String::NewExternal(stringResource);
> > +        if (newString.IsEmpty())
> > +            delete stringResource;
> > +        return newString;
> > +    }
> > +
> > +    WebCoreStringResource16* stringResource = new WebCoreStringResource16(string);
> 
> Can't we share this block by using a WebCoreStringResourceBase* ?  Or does it not have a virtual destructor?

It's not obvious here, but  v8::String::NewExternal() is the problem as it's overloaded and doesn't accept ExternalStringBase.

[Adam Barth]

Comment on attachment 169406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169406&action=review

> Source/WebCore/bindings/v8/V8PerIsolateData.cpp:116
> +            if (encoding == v8::String::ASCII_ENCODING)
> +                base = static_cast<WebCoreStringResource8*>(resource);
> +            else
> +                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.

> 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?

> 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.

> Source/WebCore/bindings/v8/V8StringResource.cpp:125
> +        v8::String::Encoding encoding;
> +        v8::String::ExternalStringResourceBase* resource = v8String->GetExternalStringResourceBase(&encoding);
> +        if (LIKELY(!!resource)) {
> +            WebCoreStringResourceBase* base;
> +            if (encoding == v8::String::ASCII_ENCODING)
> +                base = static_cast<WebCoreStringResource8*>(resource);
> +            else
> +                base = static_cast<WebCoreStringResource16*>(resource);
> +            return StringTraits<StringType>::fromStringResource(base);
> +        }

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)

> Source/WebCore/bindings/v8/V8StringResource.cpp:129
> -    if (!length)
> +    if (UNLIKELY(!length))

Did you measure this UNLIKELY as having a performance benefit?  It's most likely a no-op.

> Source/WebCore/bindings/v8/V8ValueCache.h:151
> +        return static_cast<WebCoreStringResource16*>(v8String->GetExternalStringResource());

Should we add an ASSERT about the encoding of v8String?

> Source/WebCore/bindings/v8/V8ValueCache.h:154
> +    explicit WebCoreStringResource16(const String& string): WebCoreStringResourceBase(string) { }
> +    explicit WebCoreStringResource16(const AtomicString& string): WebCoreStringResourceBase(string) { }

nit: Space before :

[Adam Barth]

Comment on attachment 169406 [details]
Patch

I'm marking this r- because its causing a lot of tests to fail, but I actually think this is looking pretty good.  My previous comments are mostly just details.

[Adam Barth]

(If I were you, I'd add a link to https://bug-91850-attachments.webkit.org/attachment.cgi?id=163334 in the ChangeLog entry.)

[Eric Seidel (no email)]

Some of those perf results show large swings in "bytes".  Is that expected?

[Dan Carney]

Comment on attachment 169406 [details]
Patch

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.

[Dan Carney]

(In reply to comment #33)
> Some of those perf results show large swings in "bytes".  Is that expected?

I'll double check once the v8 fix is in and this patch can go out, but those number were largely random in my original tests.

[Adam Barth]

Ah, I think I didn't understand the class hierarchy quite correctly.  Thanks.

This patch seems close to ready to be landed once we roll in a new version of V8.

[Adam Barth]

Comment on attachment 169406 [details]
Patch

I've adjusted the flags to more closely match my understanding of the status of this patch.

[Dan Carney]

Created attachment 171452 [details]
Patch

[Dan Carney]

Comment on attachment 171452 [details]
Patch

The necessary v8 fix has been committed.

[WebKit Review Bot]

Comment on attachment 171452 [details]
Patch

Clearing flags on attachment: 171452

Committed r132913: <http://trac.webkit.org/changeset/132913>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Reopening, this was rolled out in https://bugs.webkit.org/show_bug.cgi?id=100872.

[Dan Carney]

Created attachment 174676 [details]
Patch

[Dan Carney]

(In reply to comment #43)
> Created an attachment (id=174676) [details]
> Patch

I believe I've addressed the performance issues that caused the rollback of the last patch. See bug 100872  for more details.

Comment on attachment 174676 [details]
Patch

let's give it another try

[WebKit Review Bot]

Comment on attachment 174676 [details]
Patch

Clearing flags on attachment: 174676

Committed r134950: <http://trac.webkit.org/changeset/134950>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

I'm very happy to see this patch land again.  :)

[Eric Seidel (no email)]

Yes, thank you Dan!

[Ojan Vafai]

3% memory improvement: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/morejs/report.html?rev=168337&graph=ws_single_peak_r&trace=ws_spk_r&history=30

No performance regressions yet. Fingers crossed!

[Ojan Vafai]

Not sure what to make of this data. But it looks like there's a regression on Mac. Maybe there some conversion we're doing only on Mac with this patch?

17% improvement on DOMDivWalk on Windows and Linux:
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dom_perf/report.html?rev=168452&graph=DOMDivWalk&trace=score&history=50
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dom_perf/report.html?rev=168452&graph=Get%20Elements&trace=score&history=50

12% improvement on jslib_attr_jquery_jQuery___addClass on Mac 10.6: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibattrjquery/report.html?rev=168452&graph=jslib_attr_jquery_jQuery___addClass&trace=score&history=50

9% regression on DOMDivWalk on Mac 10.6: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=168452&graph=DOMDivWalk&trace=score&history=50

8% regression on GetElements on Mac 10.6: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=168452&graph=Get%20Elements&trace=score&history=50

[Eric Seidel (no email)]

Mac is the only platform on which we don't use tcmalloc (I believe by mistake): http://code.google.com/p/chromium/issues/detail?id=158645

[Dan Carney]

(In reply to comment #51)
> Not sure what to make of this data. But it looks like there's a regression on Mac. Maybe there some conversion we're doing only on Mac with this patch?

I only tested on linux, but I can look further into why this is happening.

[noel gordon]

Comment on attachment 174676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174676&action=review

> Source/WebCore/bindings/v8/V8ValueCache.h:86
> +        v8::V8::AdjustAmountOfExternalAllocatedMemory(memoryConsumption(string));

Here and elsewhere: memoryConsumption()?  I believe you can get that from WTF::String.sizeInBytes(), right?

[Philip Rogers]

Nice work! This patch produced a ~2.5% improvement on two SVG tests on linux: http://webkit-perf.appspot.com/graph.html#tests=[[10927939,2001,173262],[11590529,2001,173262]]&sel=1353031234357.7446,1353130502442.851&displayrange=30&datatype=running