Bug 103367 – [V8] Remove V8StringResource::m_string

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

[Kentaro Hara]

V8StringResource::m_string can be removed.

[Kentaro Hara]

Created attachment 176178 [details]
micro benchmarks

Micro benchmarks to confirm that this change won't regress performance.

[Kentaro Hara]

Created attachment 176179 [details]
Patch

[Adam Barth]

Thanks for running the microbenchmarks.

[Kentaro Hara]

(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 on attachment 176179 [details]
Patch

Looks like a couple of tests are failing.

[Kentaro Hara]

Created attachment 176249 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 176249 [details]
patch for landing

Clearing flags on attachment: 176249

Committed r135862: <http://trac.webkit.org/changeset/135862>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Kroeger]

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]

I perhaps pressed commit too soon. The crash disappeared.

[Kentaro Hara]

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

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]

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]

V8 is gone.