Bug 84103

Summary: [Performance][V8] Skip Isolate look-up to find StringCache
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, japhet, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84074    
Attachments:
Description Flags
Performance tests
none
Patch
none
Patch
none
Patch
none
Patch for relanding. It is revealed that the patch was innocent. none

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>