Bug 38661

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

Created attachment 57269 [details] Patch

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

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

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

Created attachment 57348 [details] Patch

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

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.

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

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

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?

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

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

(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? :)

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

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

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

All reviewed patches have been landed. Closing bug.

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.

Created attachment 58694 [details] Patch

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

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.

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

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

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

Created attachment 58809 [details] Patch

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 on attachment 58809 [details] Patch Clearing flags on attachment: 58809 Committed r61405: <http://trac.webkit.org/changeset/61405>

All reviewed patches have been landed. Closing bug.

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

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

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.

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

(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;

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

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>

All reviewed patches have been landed. Closing bug.