Bug 38661 – Persist V8's ScriptData to cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Gentilcore]

V8 exposes ScriprtData which represents the work done in pre-compiling JavaScript. It should be persisted to cache to speed up future compilations of the same script.

[Tony Gentilcore]

Created attachment 55260 [details]
Demonstrate caching ScriptData

This isn't ready for review yet, but helps provide context for https://bugs.webkit.org/show_bug.cgi?id=37874

[Tony Gentilcore]

Created attachment 57269 [details]
Patch

[WebKit Review Bot]

Attachment 57269 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2540060

[Tony Gentilcore]

The build breakage indicates that the V8 roll hasn't taken place on the builder yet. That roll was landed this morning in:
http://src.chromium.org/viewvc/chrome?view=rev&revision=48370

Should just be a matter of time before it updates, this should still be ready for review.

[Adam Barth]

Comment on attachment 57269 [details]
Patch

This looks good.  I'm sad about the magic number, as we've discussed before.  Also, you're comments aren't they way we usually do things in WebKit-land.  Maybe worth tweaking slightly before landing?

WebCore/ChangeLog:12
 +          Chromium's morejs benchmark shows a 3-4% improvement on fast hardware.
Awesome.

WebCore/bindings/v8/V8Proxy.cpp:345
 +      static const unsigned dataTypeID = 0xECC13BD7;
:(

WebCore/bindings/v8/V8Proxy.cpp:353
 +      // If there is cached data, use it.
We usually don't comment about *what* the code is doing, but more *why* it's doing that.

WebCore/bindings/v8/V8Proxy.cpp:359
 +      v8::ScriptData* scriptData = v8::ScriptData::PreCompile(sourceCode.source().utf8().data(), sourceCode.source().utf8().length());
Is the conversion to utf8 expensive?  Maybe we should do that once instead of twice?

[Tony Gentilcore]

(In reply to comment #5)
> (From update of attachment 57269 [details])
> This looks good.  I'm sad about the magic number, as we've discussed before.

As far as I can tell it is either (1) magic number or (2) enum that lives in WebCore with value names that represent things outside of WebCore. Initially you didn't like such an enum, but if you think it beats the magic number approach, I'm happy to switch back to that.

  Also, you're comments aren't they way we usually do things in WebKit-land. 

Thanks. I've gone back to some more spartan comments that only explain "why". Let me know if they need more tweaking.

 Maybe worth tweaking slightly before landing?
> 
> WebCore/ChangeLog:12
>  +          Chromium's morejs benchmark shows a 3-4% improvement on fast hardware.
> Awesome.

It appears even faster now without that silly double UTF8 encoding. I'm anxious to see what it looks like on the bots.
> 
> WebCore/bindings/v8/V8Proxy.cpp:345
>  +      static const unsigned dataTypeID = 0xECC13BD7;
> :(
> 
> WebCore/bindings/v8/V8Proxy.cpp:353
>  +      // If there is cached data, use it.
> We usually don't comment about *what* the code is doing, but more *why* it's doing that.
> 
> WebCore/bindings/v8/V8Proxy.cpp:359
>  +      v8::ScriptData* scriptData = v8::ScriptData::PreCompile(sourceCode.source().utf8().data(), sourceCode.source().utf8().length());
> Is the conversion to utf8 expensive?  Maybe we should do that once instead of twice?

Good catch. That was dumb.

Mads, it really seems like it would be an improvement here if I could pass the v8 external string that is already available in evaluate() instead of having to convert to utf8 here. Initially you didn't want me to modify PreCompile() to take a handle to a v8 string. Another approach would be to add an overloaded PreCompile() that takes a v8 string. What do you think?

I wouldn't have to block this patch on that, but could just switch to it after it lands.

[Tony Gentilcore]

Created attachment 57348 [details]
Patch

[WebKit Review Bot]

Attachment 57348 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2597048

[Adam Barth]

Comment on attachment 57348 [details]
Patch

Ok.  This looks like a good starting point.  We can iterate from here if we get some good ideas about how to deal with external strings, etc.

[Tony Gentilcore]

Created attachment 57664 [details]
Same patch triggering chromium EWS bot run

[WebKit Review Bot]

Attachment 57664 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2777101

[Tony Gentilcore]

I'm confused as to what controls the V8 dependency version on the EWS bot. The required version of V8 rolled into chromium over a week ago. Is there a separate process to roll V8 into WebKit?

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> I'm confused as to what controls the V8 dependency version on the EWS bot. The required version of V8 rolled into chromium over a week ago. Is there a separate process to roll V8 into WebKit?

Yep. WebKit/chromium/DEPS.

[Tony Gentilcore]

(In reply to comment #13)
> (In reply to comment #12)
> > I'm confused as to what controls the V8 dependency version on the EWS bot. The required version of V8 rolled into chromium over a week ago. Is there a separate process to roll V8 into WebKit?
> 
> Yep. WebKit/chromium/DEPS.

Thanks! It looks like tkent tried to do a roll yesterday but had to roll back. I'll just wait for that to occur.

[Dimitri Glazkov (Google)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > I'm confused as to what controls the V8 dependency version on the EWS bot. The required version of V8 rolled into chromium over a week ago. Is there a separate process to roll V8 into WebKit?
> > 
> > Yep. WebKit/chromium/DEPS.
> 
> Thanks! It looks like tkent tried to do a roll yesterday but had to roll back. I'll just wait for that to occur.

Well, technically I'll have to do that, because I'll need to clean up some stuff on bots. Maybe we'll do it together? :)

[Tony Gentilcore]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > I'm confused as to what controls the V8 dependency version on the EWS bot. The required version of V8 rolled into chromium over a week ago. Is there a separate process to roll V8 into WebKit?
> > > 
> > > Yep. WebKit/chromium/DEPS.
> > 
> > Thanks! It looks like tkent tried to do a roll yesterday but had to roll back. I'll just wait for that to occur.
> 
> Well, technically I'll have to do that, because I'll need to clean up some stuff on bots. Maybe we'll do it together? :)

I'd love to learn and help.

[Adam Barth]

Comment on attachment 57664 [details]
Same patch triggering chromium EWS bot run

Hum...  Not sure you got the bot run you wanted, but this patch still LGTM.

[Tony Gentilcore]

Created attachment 57795 [details]
Identical patch. r60622 has the DEP roll this needed, trigger another EWS run to verify.

[Adam Barth]

Comment on attachment 57795 [details]
Identical patch. r60622 has the DEP roll this needed, trigger another EWS run to verify.

yay for green bubbles

[WebKit Commit Bot]

Comment on attachment 57795 [details]
Identical patch. r60622 has the DEP roll this needed, trigger another EWS run to verify.

Clearing flags on attachment: 57795

Committed r60684: <http://trac.webkit.org/changeset/60684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Gentilcore]

Reopening this as it had to be rolled back because it broke the chromium reliability tests.

The page at http://www.mbaobao.com/ for instance cased a reliable crash.

I'm uploading a new fixed patch shortly.

[Tony Gentilcore]

Created attachment 58694 [details]
Patch

[Tony Gentilcore]

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

This patch has two differences from the original that had to be rolled back:
1. It includes a LayoutTest that fails with the original patch, but passes now.
2. It doesn't call .utf8() on the source string, but instead uses the same external V8 string that is passed to compile().

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/V8Proxy.h:56:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


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

[Tony Gentilcore]

(In reply to comment #25)
> Attachment 58694 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/bindings/v8/V8Proxy.h:56:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This has happened to me a couple of other times. If the rest of the class has improper indentation then check-webkit-style complains about a new addition that follows the current style of the class.

For future reference, I'm wondering if it is acceptable to submit a separate patch that fixes the indentation error for a whole class or if that messes with revision history too much.

Alternatively, would it be possible to modify check-webkit-style to not complain in this case if the rest of the class is indented improperly?

[David Levin]

(In reply to comment #26)
> This has happened to me a couple of other times. If the rest of the class has improper indentation then check-webkit-style complains about a new addition that follows the current style of the class.

True.

> For future reference, I'm wondering if it is acceptable to submit a separate patch that fixes the indentation error for a whole class or if that messes with revision history too much.

Sounds fine to me.

> Alternatively, would it be possible to modify check-webkit-style to not complain in this case if the rest of the class is indented improperly?

We did one fix in this direction. It only complains if the first line in the namespace is indented improperly so this doesn't fire too often but it still does go off at time like this. Sorry.

[David Levin]

Comment on attachment 58694 [details]
Patch

A few minor things that would be nice to clean up.

WebCore/ChangeLog:13
 +          originally submitted (before it had to be rolled back.
You're missing a closing ) here.

WebCore/bindings/v8/V8Proxy.cpp:34
 +  #include "CachedMetadata.h"
This header is out of order. (case sensitive sorting and 'a' > 'S').


WebCore/bindings/v8/V8Proxy.cpp:358
 +      v8::ScriptData* scriptData = v8::ScriptData::PreCompile(code);
imo, ideally this would be an OwnPtr<> even though it is returned in two lines. (Code tends to grow over time and then people add early returns without realizing that there is something to be cleaned up.)

WebCore/bindings/v8/V8Proxy.h:289
 +          static v8::Handle<v8::Script> compileScript(v8::Handle<v8::String> code, const String& fileName, int baseLine, v8::ScriptData* scriptData = 0);
The param name "scriptData" shouldn't be here.

[Tony Gentilcore]

(In reply to comment #28)
> (From update of attachment 58694 [details])
> A few minor things that would be nice to clean up.
> 
> WebCore/ChangeLog:13
>  +          originally submitted (before it had to be rolled back.
> You're missing a closing ) here.

Thanks. Fixed.

> 
> WebCore/bindings/v8/V8Proxy.cpp:34
>  +  #include "CachedMetadata.h"
> This header is out of order. (case sensitive sorting and 'a' > 'S').
> 

Sneaky. Good catch. Fixed.

> 
> WebCore/bindings/v8/V8Proxy.cpp:358
>  +      v8::ScriptData* scriptData = v8::ScriptData::PreCompile(code);
> imo, ideally this would be an OwnPtr<> even though it is returned in two lines. (Code tends to grow over time and then people add early returns without realizing that there is something to be cleaned up.)
> 

Done.

> WebCore/bindings/v8/V8Proxy.h:289
>  +          static v8::Handle<v8::Script> compileScript(v8::Handle<v8::String> code, const String& fileName, int baseLine, v8::ScriptData* scriptData = 0);
> The param name "scriptData" shouldn't be here.

Done.

[Tony Gentilcore]

Created attachment 58809 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/V8Proxy.h:56:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


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

[WebKit Commit Bot]

Comment on attachment 58809 [details]
Patch

Clearing flags on attachment: 58809

Committed r61405: <http://trac.webkit.org/changeset/61405>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61405 might have broken SnowLeopard Intel Release (Tests)

[Tony Gentilcore]

Created attachment 59121 [details]
Change minPreparseLength from unsigned to int to fix mac compile

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/V8Proxy.h:56:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


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

[Tony Gentilcore]

This broke the chromium mac compile due to this warning (warnings treated as errors):
/Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebCore/WebCore.gyp/../bindings/v8/V8Proxy.cpp:351: warning: comparison between signed and unsigned integer expressions

This latest patch corrects that.

[Adam Barth]

Comment on attachment 59121 [details]
Change minPreparseLength from unsigned to int to fix mac compile

ok.  I wish we had interdiffs so I could see what you changed, but this look correct.

[Tony Gentilcore]

(In reply to comment #38)
> (From update of attachment 59121 [details])
> ok.  I wish we had interdiffs so I could see what you changed, but this look correct.

Old:
static const unsigned minPreparseLength = 1024;

New:
static const int minPreparseLength = 1024;

[Adam Barth]

I see.  Yeah, I'm not very good at spotting signed / unsigned comparison mismatches.

[WebKit Commit Bot]

Comment on attachment 59121 [details]
Change minPreparseLength from unsigned to int to fix mac compile

Clearing flags on attachment: 59121

Committed r61499: <http://trac.webkit.org/changeset/61499>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.