| Summary: | [V8] Remove V8StringResource::m_string | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||
| Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||||||
| Status: | RESOLVED INVALID | ||||||||||
| Severity: | Normal | CC: | abarth, andersca, japhet, jochen, ojan, rjkroege, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 103331 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Kentaro Hara
2012-11-26 22:40:50 PST
Created attachment 176178 [details]
micro benchmarks
Micro benchmarks to confirm that this change won't regress performance.
Created attachment 176179 [details]
Patch
Thanks for running the microbenchmarks. (In reply to comment #3) > Thanks for running the microbenchmarks. It's difficult to statically predict performance of a String => AtomicString conversion. String ==(a)==> AtomicString AtomicString ====> String ==(b)==> AtomicString (a) is slow, but (b) is fast. In case of (b), since String knows that its StringImpl is already stored as an AtomicString (i.e. m_impl->isAtomic() returns true), (b) goes super fast. Comment on attachment 176179 [details]
Patch
Looks like a couple of tests are failing.
Created attachment 176249 [details]
patch for landing
Comment on attachment 176249 [details] patch for landing Clearing flags on attachment: 176249 Committed r135862: <http://trac.webkit.org/changeset/135862> All reviewed patches have been landed. Closing bug. fast/workers/worker-exception-during-navigation.html started failing on Linux 32 shortly after this patch landed. crash log for DumpRenderTree (pid 11503): STDOUT: <empty> STDERR: STDERR: # STDERR: # Fatal error in v8::V8::AddMessageListener() STDERR: # Error initializing V8 STDERR: # STDERR: I perhaps pressed commit too soon. The crash disappeared. (In reply to comment #9) > fast/workers/worker-exception-during-navigation.html started failing on Linux 32 shortly after this patch landed. > > crash log for DumpRenderTree (pid 11503): > STDOUT: <empty> > STDERR: > STDERR: # > STDERR: # Fatal error in v8::V8::AddMessageListener() > STDERR: # Error initializing V8 > STDERR: # > STDERR: We've been observing this failure on bots: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20%28Content%20Shell%29%20Linux/builds/2696/steps/webkit_tests/logs/stdio I couldn't reproduce it locally. I'm not sure if this patch (r135862) is a culprit. r135497 might be the real culprit. To confirm it, let me roll out r135862. Reverted r135862 for reason: We've been observing 'Fatal error in v8::V8::AddMessageListener()' in bots Committed r136188: <http://trac.webkit.org/changeset/136188> FWIW, this was also a ~10% regression on some tests: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcorequery/report.html?rev=170427&graph=dom_query_getElementById__not_in_document_&trace=score&history=100 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcorequery/report.html?rev=170427&graph=dom_query_getElementById__not_in_document_&trace=score&history=150 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcorequery/report.html?rev=170427&graph=dom_query&trace=score&history=100 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoreattr/report.html?rev=170427&graph=dom_attr_getAttribute&trace=score&history=100 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoreattr/report.html?rev=170427&graph=dom_attr_setAttribute&trace=score&history=100 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoreattr/report.html?rev=170427&graph=dom_attr_setAttribute&trace=score&history=100 And a bunch of others. V8 is gone. |