Bug 91850

Summary: add 7 bit strings capabilities to the v8 binding layer
Product: WebKit Reporter: Dan Carney <dcarney>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, haraken, japhet, jochen, leviw, msaboff, ojan, pdr, sullivan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
typical performance test results without patch
none
typical performance test results with patch
none
Patch
haraken: review-, webkit.review.bot: commit-queue-
patch for perfalizer
none
perfalizer patch
none
perf results of 163333
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dan Carney 2012-07-20 04:12:41 PDT
add 7 bit strings capabilities to the v8 binding layer
Comment 1 Dan Carney 2012-07-20 05:47:26 PDT
Created attachment 153483 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-20 05:51:42 PDT
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.
Comment 3 Dan Carney 2012-07-20 05:59:14 PDT
Created attachment 153485 [details]
Patch
Comment 4 Kentaro Hara 2012-07-20 06:00:42 PDT
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.)
Comment 5 Dan Carney 2012-07-20 08:08:38 PDT
Created attachment 153506 [details]
typical performance test results without patch
Comment 6 Dan Carney 2012-07-20 08:09:06 PDT
Created attachment 153507 [details]
typical performance test results with patch
Comment 7 Dan Carney 2012-07-20 08:10:57 PDT
(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.
Comment 8 Kentaro Hara 2012-07-20 08:15:23 PDT
(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
Comment 9 Kentaro Hara 2012-07-20 08:18:40 PDT
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).
Comment 10 Adam Barth 2012-07-20 08:47:08 PDT
Comment on attachment 153485 [details]
Patch

This patch is causing a large number of tests to fail.
Comment 11 Dan Carney 2012-07-23 02:38:23 PDT
(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.
Comment 12 Dan Carney 2012-07-24 08:24:25 PDT
Created attachment 154067 [details]
Patch
Comment 13 Dan Carney 2012-07-24 08:29:45 PDT
reuploaded patch with bug fix from v8
Comment 14 WebKit Review Bot 2012-07-24 08:42:06 PDT
Comment on attachment 154067 [details]
Patch

Attachment 154067 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13323635
Comment 15 Kentaro Hara 2012-07-24 08:46:53 PDT
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."?)
Comment 16 Kentaro Hara 2012-07-24 08:49:36 PDT
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.)
Comment 17 jochen 2012-07-25 00:45:13 PDT
(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 :-/
Comment 18 Kentaro Hara 2012-07-25 00:51:38 PDT
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.)
Comment 19 Dan Carney 2012-09-10 06:12:33 PDT
Created attachment 163109 [details]
patch for perfalizer
Comment 20 WebKit Review Bot 2012-09-10 06:14:07 PDT
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.
Comment 21 Dan Carney 2012-09-11 04:33:58 PDT
Created attachment 163333 [details]
perfalizer patch
Comment 22 Dan Carney 2012-09-11 04:35:18 PDT
Created attachment 163334 [details]
perf results of 163333
Comment 23 Dan Carney 2012-10-17 08:03:06 PDT
Created attachment 169187 [details]
Patch
Comment 24 WebKit Review Bot 2012-10-17 09:04:55 PDT
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
Comment 25 WebKit Review Bot 2012-10-17 09:56:21 PDT
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
Comment 26 Eric Seidel (no email) 2012-10-17 14:18:04 PDT
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?
Comment 27 Adam Barth 2012-10-17 14:23:42 PDT
(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.
Comment 28 Dan Carney 2012-10-18 06:53:14 PDT
Created attachment 169406 [details]
Patch
Comment 29 Dan Carney 2012-10-18 07:07:59 PDT
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.
Comment 30 Adam Barth 2012-10-18 15:12:52 PDT
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 :
Comment 31 Adam Barth 2012-10-18 15:13:32 PDT
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.
Comment 32 Adam Barth 2012-10-18 15:14:49 PDT
(If I were you, I'd add a link to https://bug-91850-attachments.webkit.org/attachment.cgi?id=163334 in the ChangeLog entry.)
Comment 33 Eric Seidel (no email) 2012-10-18 15:18:13 PDT
Some of those perf results show large swings in "bytes".  Is that expected?
Comment 34 Dan Carney 2012-10-18 15:50:54 PDT
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.
Comment 35 Dan Carney 2012-10-18 15:52:33 PDT
(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.
Comment 36 Adam Barth 2012-10-18 16:05:35 PDT
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.
Comment 37 Adam Barth 2012-10-18 16:06:05 PDT
Comment on attachment 169406 [details]
Patch

I've adjusted the flags to more closely match my understanding of the status of this patch.
Comment 38 Dan Carney 2012-10-30 08:03:14 PDT
Created attachment 171452 [details]
Patch
Comment 39 Dan Carney 2012-10-30 08:04:03 PDT
Comment on attachment 171452 [details]
Patch

The necessary v8 fix has been committed.
Comment 40 WebKit Review Bot 2012-10-30 08:48:04 PDT
Comment on attachment 171452 [details]
Patch

Clearing flags on attachment: 171452

Committed r132913: <http://trac.webkit.org/changeset/132913>
Comment 41 WebKit Review Bot 2012-10-30 08:48:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Ojan Vafai 2012-11-02 11:53:52 PDT
Reopening, this was rolled out in https://bugs.webkit.org/show_bug.cgi?id=100872.
Comment 43 Dan Carney 2012-11-16 07:08:36 PST
Created attachment 174676 [details]
Patch
Comment 44 Dan Carney 2012-11-16 07:11:48 PST
(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 45 jochen 2012-11-16 07:26:39 PST
Comment on attachment 174676 [details]
Patch

let's give it another try
Comment 46 WebKit Review Bot 2012-11-16 07:47:42 PST
Comment on attachment 174676 [details]
Patch

Clearing flags on attachment: 174676

Committed r134950: <http://trac.webkit.org/changeset/134950>
Comment 47 WebKit Review Bot 2012-11-16 07:47:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 48 Adam Barth 2012-11-16 09:39:02 PST
I'm very happy to see this patch land again.  :)
Comment 49 Eric Seidel (no email) 2012-11-16 09:41:43 PST
Yes, thank you Dan!
Comment 50 Ojan Vafai 2012-11-16 18:49:19 PST
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!
Comment 52 Eric Seidel (no email) 2012-11-17 14:13:59 PST
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
Comment 53 Dan Carney 2012-11-18 06:28:16 PST
(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.
Comment 54 noel gordon 2012-11-20 17:22:06 PST
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?