Bug 80376 – [V8][Performance] Optimize V8 bindings for HTMLElement.classList, Element.dataset and Node.attributes

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 130284 [details]
Patch

[Kentaro Hara]

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.

[Adam Barth]

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.

[Kentaro Hara]

(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")); ?

[Kentaro Hara]

Created attachment 130310 [details]
WIP

[Kentaro Hara]

Created attachment 130322 [details]
Patch

[Adam Barth]

Comment on attachment 130322 [details]
Patch

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

> Source/WebCore/ChangeLog:63
> [phone]  Kentaro Hara  <[e-mail]>

Looks like you've got two ChangeLogs

[Kentaro Hara]

Created attachment 130323 [details]
Patch

[WebKit Review Bot]

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

[Erik Arvidsson]

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.

[Ryosuke Niwa]

Comment on attachment 130323 [details]
Patch

Okay, let's not land this patch until arv comments on the bug.

[Erik Arvidsson]

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.

[Erik Arvidsson]

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.

[Kentaro Hara]

Created attachment 130458 [details]
patch for landing

[Kentaro Hara]

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

[Kentaro Hara]

Committed r109969: <http://trac.webkit.org/changeset/109969>

[Kentaro Hara]

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

[Kentaro Hara]

Reverted r109969 for reason:

layout tests crash

Committed r110004: <http://trac.webkit.org/changeset/110004>

[Kentaro Hara]

It seems the patch was innocent. Landed again.

[Kentaro Hara]

Reverted r110011 for reason:

layout tests crash

Committed r110019: <http://trac.webkit.org/changeset/110019>

[Kentaro Hara]

Created attachment 130547 [details]
patch I tried to land

[Kentaro Hara]

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

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 130746 [details]
patch for landing

[Erik Arvidsson]

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

[WebKit Review Bot]

Comment on attachment 130746 [details]
patch for landing

Clearing flags on attachment: 130746

Committed r110137: <http://trac.webkit.org/changeset/110137>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

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

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 130759 [details]
follow-up patch

[Adam Barth]

Per-isolate data to the rescue.

[Kentaro Hara]

(In reply to comment #31)
> Per-isolate data to the rescue.

What do you mean?

[Adam Barth]

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.

[Adam Barth]

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.

[Kentaro Hara]

Created attachment 131262 [details]
Patch

[Kentaro Hara]

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.

[Kentaro Hara]

Comment on attachment 131262 [details]
Patch

It still crashes.

[Kentaro Hara]

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.

[Erik Arvidsson]

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

[Eric Seidel (no email)]

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.

[Anders Carlsson]

V8 is gone from WebKit.