RESOLVED INVALID Bug 103367
[V8] Remove V8StringResource::m_string
https://bugs.webkit.org/show_bug.cgi?id=103367
Summary [V8] Remove V8StringResource::m_string
Kentaro Hara
Reported 2012-11-26 22:40:50 PST
V8StringResource::m_string can be removed.
Attachments
micro benchmarks (2.54 KB, text/html)
2012-11-26 22:41 PST, Kentaro Hara
no flags
Patch (6.51 KB, patch)
2012-11-26 22:47 PST, Kentaro Hara
no flags
patch for landing (6.46 KB, patch)
2012-11-27 05:53 PST, Kentaro Hara
no flags
Kentaro Hara
Comment 1 2012-11-26 22:41:37 PST
Created attachment 176178 [details] micro benchmarks Micro benchmarks to confirm that this change won't regress performance.
Kentaro Hara
Comment 2 2012-11-26 22:47:29 PST
Adam Barth
Comment 3 2012-11-26 23:00:46 PST
Thanks for running the microbenchmarks.
Kentaro Hara
Comment 4 2012-11-26 23:08:46 PST
(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.
Kentaro Hara
Comment 5 2012-11-27 04:05:50 PST
Comment on attachment 176179 [details] Patch Looks like a couple of tests are failing.
Kentaro Hara
Comment 6 2012-11-27 05:53:24 PST
Created attachment 176249 [details] patch for landing
WebKit Review Bot
Comment 7 2012-11-27 07:50:55 PST
Comment on attachment 176249 [details] patch for landing Clearing flags on attachment: 176249 Committed r135862: <http://trac.webkit.org/changeset/135862>
WebKit Review Bot
Comment 8 2012-11-27 07:50:59 PST
All reviewed patches have been landed. Closing bug.
Robert Kroeger
Comment 9 2012-11-27 08:57:00 PST
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:
Robert Kroeger
Comment 10 2012-11-27 09:02:18 PST
I perhaps pressed commit too soon. The crash disappeared.
Kentaro Hara
Comment 11 2012-11-29 17:12:28 PST
(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.
Kentaro Hara
Comment 12 2012-11-29 17:14:26 PST
Reverted r135862 for reason: We've been observing 'Fatal error in v8::V8::AddMessageListener()' in bots Committed r136188: <http://trac.webkit.org/changeset/136188>
Anders Carlsson
Comment 14 2013-09-01 10:32:46 PDT
V8 is gone.
Note You need to log in before you can comment on or make changes to this bug.