pushState and replaceState are supposed to create a structured clone of the state object, and per http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#structured-clone that includes special-casing RegExp objects. Right now neither the JSC nor the V8 implementation of SerializedScriptValue implement this. They handle treating of RegExp as Object differently, which is why fast/loader/stateobjects/pushstate-object-types.html fails on Chromium.
This involves using RegExp.h and RegExpObject.h from JavaScriptCore in WebCore. In turn, those depend on headers from JavaScriptCore/yarr. Is it really the right thing to be exporting all those headers from JavaScriptCore (I'm not that familiar with the forwarding header setup)?
(In reply to comment #1) > This involves using RegExp.h and RegExpObject.h from JavaScriptCore in WebCore. In turn, those depend on headers from JavaScriptCore/yarr. Is it really the right thing to be exporting all those headers from JavaScriptCore (I'm not that familiar with the forwarding header setup)? Serialization is intrinsically dependent on the JS engine, you can't really avoid knowing internal details.
(In reply to comment #2) > Serialization is intrinsically dependent on the JS engine, you can't really avoid knowing internal details. But all I want from the RegExp are the pattern and the flags (and to be able to create new RegExpObject instances), which shouldn't depend on internals. If RegExp.h didn't include YARR headers (e.g. if the m_regExpJITCOde/mRegExpBytecode/m_regExp members were moved to a pimpl class that was forward declared in the header) then it seems like I could use that header without bringing in a lot of other things. I'm new WebKit (and even newer to JSC/bindings), so I don't know if that's a common pattern though.
Created attachment 65651 [details] Patch for JSC::RegExp (In reply to comment #3) > If RegExp.h didn't include YARR headers (e.g. if the m_regExpJITCOde/mRegExpBytecode/m_regExp members were moved to a pimpl class that was forward declared in the header) then it seems like I could use that header without bringing in a lot of other things. I'm new WebKit (and even newer to JSC/bindings), so I don't know if that's a common pattern though. The attached patch implements that change (moves the JIT/bytecode/regexp to a RegExpRepresentation struct). If that seems reasonable, I can clean it up and send it out for review separately, so that the patch for this bug doesn't get too big.
Created attachment 65738 [details] Patch
Attachment 65738 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/runtime/RegExp.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Oliver, do you have time to review this? Let me know if you'd like me to split this into three patches (JavaScriptCore changes, JSC bindings changes, V8 bindings changes), though it seemed straightforward enough in its current state. The style warning is about indentation inside the JSC namespace in JavaScriptCore/runtime/RegExp.h. I was just following the indentation level of the file. Let me know if you'd like to re-indent the whole file.
Comment on attachment 65738 [details] Patch I'm slightly worried about the v8 serialisation -- what happens if i do: RegExp.prototype.toString = function() { return "foo"; } pushState(/a/) // or whatever ?
Attachment 65738 [details] did not build on qt: Build output: http://queues.webkit.org/results/3856030
Attachment 65738 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3833036
A few comments on the V8 part: > + void writeRegExp(v8::Handle<v8::Value> value) > + { > + // We store RegExp values using their toString() representation (which > + // is then eval-ed in readRegExp) since V8 does not let us get at RegExp > + // internals directly. > + // FIXME: Change once http://code.google.com/p/v8/issues/detail?id=852 > + // is fixed. > + append(RegExpTag); > + v8::String::Utf8Value regexpAsString(value->ToString()); > + doWriteString(*regexpAsString, regexpAsString.length()); > + } You could get RegExp source and flags using Object::Get() and property names "source", "global", etc. (See ECMA-262 15.10.7.) Users can't override these properties. Of course, this doesn't help without a way to reconstruct an object. > + else if (value->IsRegExp()) > + m_writer.writeRegExp(value); Should probably call the other writeRegExp. > + bool readRegExp(v8::Handle<v8::Value>* value) > + { > + v8::Handle<v8::Value> regExpAsString; > + if (!readString(®ExpAsString)) > + return false; > + v8::Local<v8::Script> regExpAsScript = v8::Script::New(regExpAsString->ToString()); > + *value = regExpAsScript->Run(); > + return true; > + } I'll expose RegExp::New to simplify this. Basically "new RegExp(source, flags)" should be eval-ed here, where "source" and "flags" can be built using the above property values. The problem is that RegExp can be changed by the user in the current context, so for now to get the original builtin constructor we should eval "new /a/.constructor(source, flags)". This is probably too much trouble without the API support.
Created attachment 65800 [details] Patch
Attachment 65800 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/runtime/RegExp.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
New version of the patch drops the V8 changes (filed bug 44809 about that). It also adds forwarding headers that I'm hoping will fix the Qt and GTK compiles (I'm not 100% clear on what the intended purpose of files under ForwardingHeaders is).
Attachment 65738 [details] did not build on win: Build output: http://queues.webkit.org/results/3871036
Attachment 65800 [details] did not build on win: Build output: http://queues.webkit.org/results/3853055
Oliver, would you be able to review this, now that the V8 changes have been moved out? (In reply to comment #16) > Attachment 65800 [details] did not build on win: > Build output: http://queues.webkit.org/results/3853055 Unfortunately this output is not very helpful. I'm guessing that the list of exported symbols needs to be updated on the Windows side too. Looking at http://trac.webkit.org/log/trunk/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def, that seems to be something that's done after the fact as a build fix (I don't have easy access to a Windows machine).
Created attachment 66760 [details] Patch
Latest version of the patch should work with the re-implementation of SerializedScriptValue done by r66850 (thanks for doing that, the new version is much less class-happy). Oliver or Sam, could one of you review this?
Comment on attachment 66760 [details] Patch > + RegExpRepresentation* const m_representation; Can we use OwnPtr for this?
Created attachment 66766 [details] Patch
(In reply to comment #20) > (From update of attachment 66760 [details]) > > + RegExpRepresentation* const m_representation; > > Can we use OwnPtr for this? Makes sense; latest version of the patch does that.
Attachment 66766 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/runtime/RegExp.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 66766 [details] Patch r=me! Sorry i forgot to mention the serialization change in this bug earlier :(
Comment on attachment 66766 [details] Patch > + , m_representation(adoptPtr(new RegExpRepresentation())) No need for the parentheses here. This means that every RegExp is using a second memory block. I’d expect that to slow down regular expression tests, perhaps even including the regular expression part of SunSpider. This is marked review+, but how can it be until we know the SunSpider impact?
Comment on attachment 66766 [details] Patch Removing cq bit while I look at SunSpider impact.
Created attachment 66773 [details] Patch
(In reply to comment #25) > (From update of attachment 66766 [details]) > > + , m_representation(adoptPtr(new RegExpRepresentation())) > > No need for the parentheses here. Removed in latest version of the patch. > This means that every RegExp is using a second memory block. I’d expect that to slow down regular expression tests, perhaps even including the regular expression part of SunSpider. This is marked review+, but how can it be until we know the SunSpider impact? I ran SunSpider in a release build with this change and compared it with the latest nightly build (at r66819), here are the averages for 5 runs: Total SunSpider time regexp-dna time r66819: 263.14 13.84 w/ patch: 266.9 13.9
Attachment 66773 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/runtime/RegExp.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #28) > (In reply to comment #25) > > (From update of attachment 66766 [details] [details]) > > > + , m_representation(adoptPtr(new RegExpRepresentation())) > > > > No need for the parentheses here. > > Removed in latest version of the patch. > > > This means that every RegExp is using a second memory block. I’d expect that to slow down regular expression tests, perhaps even including the regular expression part of SunSpider. This is marked review+, but how can it be until we know the SunSpider impact? > > I ran SunSpider in a release build with this change and compared it with the latest nightly build (at r66819), here are the averages for 5 runs: > > Total SunSpider time regexp-dna time > r66819: 263.14 13.84 > w/ patch: 266.9 13.9 can you do run-sunspider --runs 30 > oldTimes apply patch run-sunspider --runs 30 > newTimes and paste the result of sunspider-compare-results oldTimes newTimes into the bug that will be more helpful (your numbers above indicate a ~1% regression, so i'd rather have stable numbers)
$ sunspider-compare-results ~/Desktop/sunspider.before ~/Desktop/sunspider.after TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: - 260.9ms +/- 0.3% 260.3ms +/- 0.2% ============================================================================= 3d: - 35.4ms +/- 1.4% 35.4ms +/- 0.5% cube: ?? 12.3ms +/- 2.0% 12.4ms +/- 1.5% not conclusive: might be *1.003x as slow* morph: - 11.1ms +/- 3.3% 10.9ms +/- 1.0% raytrace: *1.011x as slow* 12.0ms +/- 0.6% 12.1ms +/- 0.9% significant access: ?? 32.7ms +/- 0.8% 32.9ms +/- 0.9% not conclusive: might be *1.004x as slow* binary-trees: - 3.2ms +/- 4.8% 3.2ms +/- 4.8% fannkuch: ?? 15.5ms +/- 1.2% 15.6ms +/- 1.2% not conclusive: might be *1.004x as slow* nbody: ?? 8.9ms +/- 1.1% 9.0ms +/- 1.3% not conclusive: might be *1.004x as slow* nsieve: ?? 5.1ms +/- 2.2% 5.1ms +/- 2.5% not conclusive: might be *1.007x as slow* bitops: ?? 20.8ms +/- 1.3% 20.9ms +/- 1.0% not conclusive: might be *1.003x as slow* 3bit-bits-in-byte: ?? 2.7ms +/- 6.5% 2.8ms +/- 5.8% not conclusive: might be *1.025x as slow* bits-in-byte: - 7.1ms +/- 1.6% 7.1ms +/- 1.3% bitwise-and: - 4.2ms +/- 3.8% 4.2ms +/- 3.6% nsieve-bits: ?? 6.8ms +/- 2.2% 6.9ms +/- 1.9% not conclusive: might be *1.010x as slow* controlflow: ?? 2.5ms +/- 7.5% 2.6ms +/- 7.0% not conclusive: might be *1.039x as slow* recursive: ?? 2.5ms +/- 7.5% 2.6ms +/- 7.0% not conclusive: might be *1.039x as slow* crypto: ?? 16.0ms +/- 1.4% 16.2ms +/- 1.4% not conclusive: might be *1.008x as slow* aes: ?? 9.2ms +/- 1.7% 9.2ms +/- 1.7% not conclusive: might be *1.004x as slow* md5: - 3.8ms +/- 3.7% 3.8ms +/- 4.0% sha1: *1.044x as slow* 3.0ms +/- 0.0% 3.1ms +/- 4.1% significant date: - 31.8ms +/- 1.3% 31.5ms +/- 0.9% format-tofte: - 18.8ms +/- 1.8% 18.5ms +/- 1.4% format-xparb: - 13.0ms +/- 1.3% 13.0ms +/- 0.5% math: - 26.4ms +/- 0.7% 26.3ms +/- 0.7% cordic: - 8.1ms +/- 1.4% 8.0ms +/- 0.9% partial-sums: - 13.0ms +/- 0.0% 13.0ms +/- 0.0% spectral-norm: - 5.3ms +/- 3.2% 5.3ms +/- 3.2% regexp: *1.026x as slow* 15.7ms +/- 1.1% 16.1ms +/- 0.6% significant dna: *1.026x as slow* 15.7ms +/- 1.1% 16.1ms +/- 0.6% significant string: 1.013x as fast 79.5ms +/- 0.5% 78.5ms +/- 0.5% significant base64: - 8.6ms +/- 2.2% 8.5ms +/- 2.2% fasta: 1.026x as fast 10.6ms +/- 1.8% 10.3ms +/- 1.7% significant tagcloud: - 20.4ms +/- 0.9% 20.2ms +/- 0.8% unpack-code: 1.016x as fast 30.1ms +/- 0.8% 29.6ms +/- 0.6% significant validate-input: - 9.8ms +/- 1.8% 9.8ms +/- 1.4%
Comment on attachment 66773 [details] Patch r=me
Comment on attachment 66773 [details] Patch Clearing flags on attachment: 66773 Committed r66936: <http://trac.webkit.org/changeset/66936>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/66936 might have broken Leopard Intel Debug (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/66936 http://trac.webkit.org/changeset/66935
(In reply to comment #35) > http://trac.webkit.org/changeset/66936 might have broken Leopard Intel Debug (Tests) > The following changes are on the blame list: > http://trac.webkit.org/changeset/66936 > http://trac.webkit.org/changeset/66935 fast/files/workers/worker-read-blob-sync.html failed, but it has failed before and after these changes too. It looks to be flaky (though only on Leopard, since I can't reproduce locally). I filed bug 45396 about it.
Yes, looks flaky. We need a bug on it.
(In reply to comment #37) > Yes, looks flaky. We need a bug on it. Filed bug 45396 for it. I guess we can skip it for Leopard for now?