Bug 80376

Summary: [V8][Performance] Optimize V8 bindings for HTMLElement.classList, Element.dataset and Node.attributes
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, andersca, arv, japhet, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test cases
none
Patch
none
WIP
none
Patch
none
Patch
none
patch for landing
none
patch I tried to land
none
patch for landing
none
follow-up patch
none
Patch haraken: review-

Description Kentaro Hara 2012-03-05 21:47:14 PST
Created attachment 130283 [details]
Test cases

V8 bindings for HTMLElement.classList, Element.dataset and Node.attributes are much slower than JavaScriptCore bindings. We should optimize them.

Performance tests are attached. The results for the tests in my local Mac environment are as follows:

AppleWebKit/JavaScriptCore:
div.classList : 382ms
div.classList.foo = 123 : 335ms
div.dataset : 403ms
div.dataset.foo = 123 : 5250ms
div.attributes : 183ms

Chromium/V8:
div.classList : 9140ms
div.classList.foo = 123 : 9086ms
div.dataset : 9930ms
div.dataset.foo = 123 : 49698ms
div.attributes : 13489ms
Comment 1 Kentaro Hara 2012-03-05 21:48:09 PST
Created attachment 130284 [details]
Patch
Comment 2 Kentaro Hara 2012-03-05 21:48:41 PST
The patch improves the performance of HTMLElement.classList, Element.dataset and Node.attributes by 6.4 times, 7.1 times and 10.9 times, respectively.
Comment 3 Adam Barth 2012-03-05 22:08:10 PST
Comment on attachment 130284 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:95
> +        static v8::Persistent<v8::String> hiddenReferenceName = v8::Persistent<v8::String>::New(V8HiddenPropertyName::hiddenReferenceName("domStringMap"));

We should use DEFINE_STATIC_LOCAL.  Actually, consider using V8HiddenPropertyName, which should solve this problem too.
Comment 4 Kentaro Hara 2012-03-05 22:37:47 PST
(In reply to comment #3)
> > Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:95
> > +        static v8::Persistent<v8::String> hiddenReferenceName = v8::Persistent<v8::String>::New(V8HiddenPropertyName::hiddenReferenceName("domStringMap"));
> 
> We should use DEFINE_STATIC_LOCAL. 

Sure. Thanks.

> Actually, consider using V8HiddenPropertyName, which should solve this problem too.

What do you mean?

DEFINE_STATIC_LOCAL(v8::Persistent<v8::String>, hiddenReferenceName, v8::Persistent<v8::String>::New(V8HiddenPropertyName::hiddenReferenceName("domStringMap")); ?
Comment 5 Kentaro Hara 2012-03-05 23:58:27 PST
Created attachment 130310 [details]
WIP
Comment 6 Kentaro Hara 2012-03-06 00:46:58 PST
Created attachment 130322 [details]
Patch
Comment 7 Adam Barth 2012-03-06 00:49:05 PST
Comment on attachment 130322 [details]
Patch

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

> Source/WebCore/ChangeLog:63
> +2012-03-05  Kentaro Hara  <haraken@chromium.org>

Looks like you've got two ChangeLogs
Comment 8 Kentaro Hara 2012-03-06 00:49:12 PST
Created attachment 130323 [details]
Patch
Comment 9 WebKit Review Bot 2012-03-06 03:57:36 PST
Comment on attachment 130323 [details]
Patch

Rejecting attachment 130323 [details] from commit-queue.

New failing tests:
fast/xmlhttprequest/xmlhttprequest-gc.html
inspector/debugger/script-formatter-breakpoints.html
Full output: http://queues.webkit.org/results/11835315
Comment 10 Erik Arvidsson 2012-03-06 08:03:15 PST
I have a patch that removes this code entirely. We should not be using hidden properties at all. We should use hidden references in the GC phase instead. 

I'm currently using my phone. I'll provide more details when I get to work.
Comment 11 Ryosuke Niwa 2012-03-06 08:26:37 PST
Comment on attachment 130323 [details]
Patch

Okay, let's not land this patch until arv comments on the bug.
Comment 12 Erik Arvidsson 2012-03-06 09:13:17 PST
In the WIP patch for bug 78149 I introduces V8GenerateIsReachable (name is matching JSC but implementation is very different). If this extended attribute is set we generate code that calls v8::V8::AddImplicitReferences which adds an edge from the parent to the child object during the GC phase.

When working on bug 78052 I also found that creating a new symbol every time is slow. We should define the name statically in the code generator too. See patch for 78052. 

I think it does not hurt to submit this. Since this is a perf win we will need to ensure that the other work er are doing is at least as good.
Comment 13 Erik Arvidsson 2012-03-06 09:18:31 PST
Comment on attachment 130323 [details]
Patch

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

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:58
> +#define V8_DECLARE_PROPERTY(name, prefix) static v8::Handle<v8::String> name();

prefix is not used

> Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:95
> +        toV8(element)->ToObject()->SetHiddenValue(V8HiddenPropertyName::domStringMap(), wrapper);

Use v8::Object::Cast or As() instead. If you look at the code it is a lot cheaper.
Comment 14 Kentaro Hara 2012-03-06 15:52:54 PST
Created attachment 130458 [details]
patch for landing
Comment 15 Kentaro Hara 2012-03-06 15:53:11 PST
(In reply to comment #13)
> > Source/WebCore/bindings/v8/V8HiddenPropertyName.h:58
> > +#define V8_DECLARE_PROPERTY(name, prefix) static v8::Handle<v8::String> name();
> 
> prefix is not used

prefix is necessary, since V8_DECLARE_PROPERTY() is called by V8_HIDDEN_PROPERTIES().

> > Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:95
> > +        toV8(element)->ToObject()->SetHiddenValue(V8HiddenPropertyName::domStringMap(), wrapper);
> 
> Use v8::Object::Cast or As() instead. If you look at the code it is a lot cheaper.

Fixed. (I couldn't see any performance gain though.)

> I think it does not hurt to submit this. Since this is a perf win we will need to ensure that the other work er are doing is at least as good.

Thanks. Let me land it.
Comment 16 Kentaro Hara 2012-03-06 15:55:04 PST
Committed r109969: <http://trac.webkit.org/changeset/109969>
Comment 17 Kentaro Hara 2012-03-06 16:03:14 PST
(In reply to comment #12)
> In the WIP patch for bug 78149 I introduces V8GenerateIsReachable (name is matching JSC but implementation is very different). If this extended attribute is set we generate code that calls v8::V8::AddImplicitReferences which adds an edge from the parent to the child object during the GC phase.

arv: Sounds great!

In following patches, I am planning to remove V8HiddenPropertyName::hiddenReferenceName() completely (and may remove V8HiddenPropertyName.{h,cpp}). V8HiddenPropertyName::fooName() is not good in that we need to add V8_HIDDEN_PROPERTIES(V) entries to V8HiddenPropertyName.h every time we want to add a new fooName. This makes it difficult to generate V8HiddenPropertyName::fooName() in code generators (since the code generators cannot touch V8HiddenPropertyName.h). Maybe what we need is just one macro, i.e. DEFINE_STATIC_HIDDEN_NAME(fooName), which defines the hidden fooName statically on v8::Persistent. I am afraid this work would conflict with bug 78052. If you have concern, I can stop the work. WDYT?
Comment 18 Kentaro Hara 2012-03-06 19:53:43 PST
Reverted r109969 for reason:

layout tests crash

Committed r110004: <http://trac.webkit.org/changeset/110004>
Comment 19 Kentaro Hara 2012-03-06 21:07:11 PST
It seems the patch was innocent. Landed again.
Comment 20 Kentaro Hara 2012-03-06 22:35:43 PST
Reverted r110011 for reason:

layout tests crash

Committed r110019: <http://trac.webkit.org/changeset/110019>
Comment 21 Kentaro Hara 2012-03-06 22:38:41 PST
Created attachment 130547 [details]
patch I tried to land
Comment 22 Kentaro Hara 2012-03-06 22:40:07 PST
The following tests are crashing in V8 gc:

fast/writing-mode/Kusa-Makura-background-canvas.html
inspector/debugger/script-formatter-breakpoints.html
inspector/debugger/script-formatter.html
storage/indexeddb/index-count.html
storage/indexeddb/objectstore-basics.html
Comment 23 Kentaro Hara 2012-03-07 16:27:06 PST
Comment on attachment 130547 [details]
patch I tried to land

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

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:47
> +    DEFINE_STATIC_LOCAL(v8::Persistent<v8::String>, hiddenPropertyName, (createString(prefix V8_AS_STRING(name)))); \

It seems we cannot use a static variable in bindings code which can be executed by multiple threads. I'll fix bug 80453 before committing this patch.
Comment 24 Kentaro Hara 2012-03-07 18:41:23 PST
Created attachment 130746 [details]
patch for landing
Comment 25 Erik Arvidsson 2012-03-07 20:00:08 PST
Comment on attachment 130746 [details]
patch for landing

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

Overall comments

> Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:96
>          if (!elementValue.IsEmpty() && elementValue->IsObject())

This test can be removed like you said in the ChangeLog.

> Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp:50
>          if (!elementValue.IsEmpty() && elementValue->IsObject())

remove

> Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp:81
>      if (!wrapper.IsEmpty() && element)

remove
Comment 26 WebKit Review Bot 2012-03-07 20:02:44 PST
Comment on attachment 130746 [details]
patch for landing

Clearing flags on attachment: 130746

Committed r110137: <http://trac.webkit.org/changeset/110137>
Comment 27 WebKit Review Bot 2012-03-07 20:02:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Kentaro Hara 2012-03-07 20:11:16 PST
(In reply to comment #25)
> > Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:96
> >          if (!elementValue.IsEmpty() && elementValue->IsObject())
> 
> This test can be removed like you said in the ChangeLog.
>
> > Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp:50
> >          if (!elementValue.IsEmpty() && elementValue->IsObject())
> 
> remove

OK. I'll do it in a following patch.
 
> > Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp:81
> >      if (!wrapper.IsEmpty() && element)

I think this cannot be removed.
Comment 29 Kentaro Hara 2012-03-07 21:12:58 PST
Reopening to attach new patch.
Comment 30 Kentaro Hara 2012-03-07 21:13:02 PST
Created attachment 130759 [details]
follow-up patch
Comment 31 Adam Barth 2012-03-07 21:19:39 PST
Per-isolate data to the rescue.
Comment 32 Kentaro Hara 2012-03-07 21:21:08 PST
(In reply to comment #31)
> Per-isolate data to the rescue.

What do you mean?
Comment 33 Adam Barth 2012-03-07 21:27:57 PST
Comment on attachment 130759 [details]
follow-up patch

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

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:49
> +    return V8BindingPerIsolateData::current()->hiddenPropertyName()->m_##name; \

I mean that using V8BindingPerIsolateData lets you get around the threading problems since each isolate is only used on one thread.
Comment 34 Adam Barth 2012-03-09 16:58:25 PST
Comment on attachment 130759 [details]
follow-up patch

  fast/filesystem/workers/file-from-file-entry.html = CRASH
  fast/filesystem/workers/file-writer-gc-blob.html = CRASH
  fast/filesystem/workers/file-writer-sync-truncate-extend.html = CRASH
  fast/filesystem/workers/file-writer-sync-write-overlapped.html = CRASH
  fast/filesystem/workers/file-writer-truncate-extend.html = CRASH
  fast/filesystem/workers/file-writer-write-overlapped.html = CRASH
  http/tests/eventsource/workers/eventsource-simple.html = CRASH
  http/tests/filesystem/workers/resolve-url-sync.html = CRASH
  http/tests/filesystem/workers/resolve-url.html = CRASH
  http/tests/security/contentSecurityPolicy/worker-connect-src-allowed.html = CRASH
  http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html = CRASH

This patch makes many tests crash.
Comment 35 Kentaro Hara 2012-03-11 17:32:03 PDT
Created attachment 131262 [details]
Patch
Comment 36 Kentaro Hara 2012-03-11 17:33:46 PDT
Restored "if (!elementValue.IsEmpty() && elementValue->IsObject())" check. I think that theoretically the check won't be necessary, but actually it seems to be necessary. Watching the cr-linux ews-bot.
Comment 37 Kentaro Hara 2012-03-11 19:30:25 PDT
Comment on attachment 131262 [details]
Patch

It still crashes.
Comment 38 Kentaro Hara 2012-03-12 18:28:58 PDT
arv: Do you know why creating a string in the V8HiddenPropertyName constructor causes test crashes? Maybe this seems to be a similar crash when we use a static variable in bindings code.
Comment 39 Erik Arvidsson 2012-03-13 12:00:27 PDT
(In reply to comment #38)
> arv: Do you know why creating a string in the V8HiddenPropertyName constructor causes test crashes? Maybe this seems to be a similar crash when we use a static variable in bindings code.

I don't think these are thread safe.
Comment 40 Eric Seidel (no email) 2012-03-27 12:50:35 PDT
Comment on attachment 130322 [details]
Patch

Cleared Adam Barth's review+ from obsolete attachment 130322 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 41 Anders Carlsson 2013-05-02 11:15:13 PDT
V8 is gone from WebKit.