Bug 38661

Summary: Persist V8's ScriptData to cache
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, ager, commit-queue, dglazkov, eric, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 37874, 39948, 40196, 40838    
Bug Blocks:    
Attachments:
Description Flags
Demonstrate caching ScriptData
none
Patch
none
Patch
none
Same patch triggering chromium EWS bot run
none
Identical patch. r60622 has the DEP roll this needed, trigger another EWS run to verify.
none
Patch
none
Patch
none
Change minPreparseLength from unsigned to int to fix mac compile none

Description Tony Gentilcore 2010-05-06 10:40:14 PDT
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.
Comment 1 Tony Gentilcore 2010-05-06 10:42:22 PDT
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
Comment 2 Tony Gentilcore 2010-05-27 13:10:43 PDT
Created attachment 57269 [details]
Patch
Comment 3 WebKit Review Bot 2010-05-27 13:28:36 PDT
Attachment 57269 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2540060
Comment 4 Tony Gentilcore 2010-05-27 13:33:50 PDT
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.
Comment 5 Adam Barth 2010-05-27 15:55:46 PDT
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?
Comment 6 Tony Gentilcore 2010-05-28 11:18:11 PDT
(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.
Comment 7 Tony Gentilcore 2010-05-28 11:19:56 PDT
Created attachment 57348 [details]
Patch
Comment 8 WebKit Review Bot 2010-05-28 12:26:54 PDT
Attachment 57348 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2597048
Comment 9 Adam Barth 2010-06-01 22:17:47 PDT
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.
Comment 10 Tony Gentilcore 2010-06-02 10:08:07 PDT
Created attachment 57664 [details]
Same patch triggering chromium EWS bot run
Comment 11 WebKit Review Bot 2010-06-02 10:54:34 PDT
Attachment 57664 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2777101
Comment 12 Tony Gentilcore 2010-06-02 10:59:49 PDT
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?
Comment 13 Dimitri Glazkov (Google) 2010-06-02 11:03:31 PDT
(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.
Comment 14 Tony Gentilcore 2010-06-02 11:13:20 PDT
(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.
Comment 15 Dimitri Glazkov (Google) 2010-06-02 11:14:48 PDT
(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? :)
Comment 16 Tony Gentilcore 2010-06-02 11:15:59 PDT
(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.
Comment 17 Adam Barth 2010-06-02 12:35:59 PDT
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.
Comment 18 Tony Gentilcore 2010-06-03 11:32:10 PDT
Created attachment 57795 [details]
Identical patch. r60622 has the DEP roll this needed, trigger another EWS run to verify.
Comment 19 Adam Barth 2010-06-04 09:30:26 PDT
Comment on attachment 57795 [details]
Identical patch. r60622 has the DEP roll this needed, trigger another EWS run to verify.

yay for green bubbles
Comment 20 WebKit Commit Bot 2010-06-04 09:59:33 PDT
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>
Comment 21 WebKit Commit Bot 2010-06-04 09:59:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Tony Gentilcore 2010-06-14 13:00:48 PDT
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.
Comment 23 Tony Gentilcore 2010-06-14 13:30:05 PDT
Created attachment 58694 [details]
Patch
Comment 24 Tony Gentilcore 2010-06-14 13:32:48 PDT
(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().
Comment 25 WebKit Review Bot 2010-06-14 13:33:15 PDT
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.
Comment 26 Tony Gentilcore 2010-06-14 13:41:22 PDT
(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?
Comment 27 David Levin 2010-06-14 23:34:13 PDT
(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.
Comment 28 David Levin 2010-06-14 23:43:25 PDT
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.
Comment 29 Tony Gentilcore 2010-06-15 12:39:32 PDT
(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.
Comment 30 Tony Gentilcore 2010-06-15 12:53:57 PDT
Created attachment 58809 [details]
Patch
Comment 31 WebKit Review Bot 2010-06-15 12:56:26 PDT
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.
Comment 32 WebKit Commit Bot 2010-06-18 08:00:39 PDT
Comment on attachment 58809 [details]
Patch

Clearing flags on attachment: 58809

Committed r61405: <http://trac.webkit.org/changeset/61405>
Comment 33 WebKit Commit Bot 2010-06-18 08:00:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2010-06-18 08:22:13 PDT
http://trac.webkit.org/changeset/61405 might have broken SnowLeopard Intel Release (Tests)
Comment 35 Tony Gentilcore 2010-06-18 09:21:52 PDT
Created attachment 59121 [details]
Change minPreparseLength from unsigned to int to fix mac compile
Comment 36 WebKit Review Bot 2010-06-18 09:23:06 PDT
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.
Comment 37 Tony Gentilcore 2010-06-18 09:23:36 PDT
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.
Comment 38 Adam Barth 2010-06-19 17:03:03 PDT
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.
Comment 39 Tony Gentilcore 2010-06-19 17:10:54 PDT
(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;
Comment 40 Adam Barth 2010-06-19 17:14:41 PDT
I see.  Yeah, I'm not very good at spotting signed / unsigned comparison mismatches.
Comment 41 WebKit Commit Bot 2010-06-19 17:38:10 PDT
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>
Comment 42 WebKit Commit Bot 2010-06-19 17:38:17 PDT
All reviewed patches have been landed.  Closing bug.