WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(6.51 KB, patch)
2012-11-26 22:47 PST
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
patch for landing
(6.46 KB, patch)
2012-11-27 05:53 PST
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 176179
[details]
Patch
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
>
Ojan Vafai
Comment 13
2012-11-30 16:45:45 PST
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.
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.
Top of Page
Format For Printing
XML
Clone This Bug