Bug 84103 - [Performance][V8] Skip Isolate look-up to find StringCache
Summary: [Performance][V8] Skip Isolate look-up to find StringCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 84074
  Show dependency treegraph
 
Reported: 2012-04-16 17:19 PDT by Kentaro Hara
Modified: 2012-04-23 09:55 PDT (History)
5 users (show)

See Also:


Attachments
Performance tests (1.90 KB, text/html)
2012-04-16 17:21 PDT, Kentaro Hara
no flags Details
Patch (8.72 KB, patch)
2012-04-16 17:30 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (8.72 KB, patch)
2012-04-17 02:20 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (17.07 KB, patch)
2012-04-17 02:27 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch for relanding. It is revealed that the patch was innocent. (17.06 KB, patch)
2012-04-23 09:54 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-04-16 17:19:59 PDT
StringCache is an Isolate-local data structure. Currently, V8 bindings need to look up an Isolate to find the StringCache. By using AccessorInfo.GetIsolate() or Arguments.GetIsolate(), we can remove the Isolate look-up.
Comment 1 Kentaro Hara 2012-04-16 17:21:02 PDT
Created attachment 137440 [details]
Performance tests
Comment 2 Kentaro Hara 2012-04-16 17:30:26 PDT
Created attachment 137441 [details]
Patch
Comment 3 WebKit Review Bot 2012-04-16 18:08:39 PDT
Comment on attachment 137441 [details]
Patch

Attachment 137441 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12412665
Comment 4 Kentaro Hara 2012-04-17 02:20:40 PDT
Created attachment 137499 [details]
Patch
Comment 5 Kentaro Hara 2012-04-17 02:24:18 PDT
Rolled chromium DEPS to r132474 and re-uploaded the patch. The patch requires V8 r11306.
Comment 6 Kentaro Hara 2012-04-17 02:27:43 PDT
Created attachment 137502 [details]
Patch
Comment 7 Ryosuke Niwa 2012-04-17 09:54:54 PDT
Comment on attachment 137502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137502&action=review

> Source/WebCore/ChangeLog:34
> +        Performance test: https://bugs.webkit.org/attachment.cgi?id=137440
> +        div.id:           30.70ns -> 26.70ns (+15%)
> +        div.className:    31.10ns -> 26.40ns (+18%)
> +        div.nodeName:     37.70ns -> 33.00ns (+14%)
> +        text.nodeValue:   31.40ns -> 25.90ns (+21%)
> +        text.textContent: 51.50ns -> 45.90ns (+12%)

Should we add it as a performance test?
Comment 8 Kentaro Hara 2012-04-17 09:58:49 PDT
(In reply to comment #7)
> > Source/WebCore/ChangeLog:34
> > +        Performance test: https://bugs.webkit.org/attachment.cgi?id=137440
> > +        div.id:           30.70ns -> 26.70ns (+15%)
> > +        div.className:    31.10ns -> 26.40ns (+18%)
> > +        div.nodeName:     37.70ns -> 33.00ns (+14%)
> > +        text.nodeValue:   31.40ns -> 25.90ns (+21%)
> > +        text.textContent: 51.50ns -> 45.90ns (+12%)
> 
> Should we add it as a performance test?

Bindings/dom-attributes.html should cover it. I'll update it later. (Currently, the div.id test in dom-attributes.html returns null, which is a different call-path from the performance test attached in this bug.)
Comment 9 WebKit Review Bot 2012-04-17 13:05:40 PDT
Comment on attachment 137502 [details]
Patch

Clearing flags on attachment: 137502

Committed r114421: <http://trac.webkit.org/changeset/114421>
Comment 10 WebKit Review Bot 2012-04-17 13:05:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Kentaro Hara 2012-04-19 10:00:34 PDT
Reverted r114421 for reason:

Chromium crash

Committed r114645: <http://trac.webkit.org/changeset/114645>
Comment 12 Kentaro Hara 2012-04-23 09:54:45 PDT
Created attachment 138368 [details]
Patch for relanding. It is revealed that the patch was innocent.
Comment 13 Kentaro Hara 2012-04-23 09:55:28 PDT
Committed r114912: <http://trac.webkit.org/changeset/114912>