Bug 80462 – [V8] Add DEFINE_STATIC_HIDDEN_PROPERTY() macro and use it in CodeGeneratorV8.pm

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore JavaScript

[Kentaro Hara]

V8HiddenPropertyName::hiddenReferenceName() is very slow, and the goal is to replace the existing code

    someFunction(..., V8HiddenPropertyName::hiddenReferenceName("fooName"), ...);

with

    DEFINE_STATIC_HIDDEN_PROPERTY(fooName);
    someFunction(..., fooName, ...);

See bug 80453 for more details.

In this bug, we add the DEFINE_STATIC_HIDDEN_PROPERTY macro to V8BindingMacros.h, and replaces V8DOMWrapper::setNamedHiddenWindowReference() with the macro in CodeGeneratorV8.pm.

[Kentaro Hara]

Created attachment 130491 [details]
Patch

[Erik Arvidsson]

Comment on attachment 130491 [details]
Patch

LGMT but I'm not a reviewer

[Kentaro Hara]

Created attachment 130535 [details]
Patch

[Adam Barth]

Comment on attachment 130535 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8BindingMacros.h:57
> +#define DEFINE_STATIC_HIDDEN_PROPERTY(name) \
> +    DEFINE_STATIC_LOCAL(v8::Persistent<v8::String>, name, (v8::Persistent<v8::String>::New(v8::String::NewSymbol(#name "::HiddenProperty"))));

Can we stick with using a prefix (rather than a suffix)?  Also, we probably want to include "WebCore" somewhere in the prefix.

For context, the problem is that the hidden property names exist in a common namespace.  There's other code that pokes at hidden properties (e.g., the extension system), so we want to make sure we're using distinct names.

[Kentaro Hara]

Created attachment 130548 [details]
patch for landing

[Kentaro Hara]

Comment on attachment 130535 [details]
Patch

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

>> Source/WebCore/bindings/v8/custom/V8BindingMacros.h:57
>> +    DEFINE_STATIC_LOCAL(v8::Persistent<v8::String>, name, (v8::Persistent<v8::String>::New(v8::String::NewSymbol(#name "::HiddenProperty"))));
> 
> Can we stick with using a prefix (rather than a suffix)?  Also, we probably want to include "WebCore" somewhere in the prefix.
> 
> For context, the problem is that the hidden property names exist in a common namespace.  There's other code that pokes at hidden properties (e.g., the extension system), so we want to make sure we're using distinct names.

Makes sense. Fixed it and committed. Thanks.

[WebKit Review Bot]

Comment on attachment 130548 [details]
patch for landing

Rejecting attachment 130548 [details] from commit-queue.

New failing tests:
fast/filesystem/workers/file-writer-write-overlapped.html
fast/workers/worker-location.html
storage/indexeddb/objectstore-basics-workers.html
fast/filesystem/workers/file-writer-sync-write-overlapped.html
storage/indexeddb/basics.html
fast/filesystem/workers/simple-temporary-sync.html
Full output: http://queues.webkit.org/results/11836912

[Kentaro Hara]

(In reply to comment #7)
> (From update of attachment 130548 [details])
> Rejecting attachment 130548 [details] from commit-queue.
> 
> New failing tests:
> fast/filesystem/workers/file-writer-write-overlapped.html
> fast/workers/worker-location.html
> storage/indexeddb/objectstore-basics-workers.html
> fast/filesystem/workers/file-writer-sync-write-overlapped.html
> storage/indexeddb/basics.html
> fast/filesystem/workers/simple-temporary-sync.html
> Full output: http://queues.webkit.org/results/11836912

All these tests are passing on my local chromium/Linux environment. Let me cq+ it again.

[WebKit Review Bot]

Comment on attachment 130548 [details]
patch for landing

Rejecting attachment 130548 [details] from commit-queue.

New failing tests:
fast/filesystem/workers/file-writer-write-overlapped.html
fast/workers/worker-location.html
storage/indexeddb/objectstore-basics-workers.html
fast/filesystem/workers/file-writer-sync-write-overlapped.html
storage/indexeddb/basics.html
fast/filesystem/workers/simple-temporary-sync.html
Full output: http://queues.webkit.org/results/11851098

[Erik Arvidsson]

Isn't there an issue with workers here? We cannot use statics if the idl file has NoStaticTables.

[Kentaro Hara]

We cannot use static variables in the bindings code. Instead, we need to use V8BindingPerIsolateData, which implies that we need to use V8HiddenPropertyName::fooName() or something like that to access the hidden property from the generated code. However, it requires to add a new V(fooName) entry to the V8_HIDDEN_PROPERTIES() macro in V8HiddenPropertyName.h. This is difficult because the code generator cannot touch V8HiddenPropertyName.h...

One solution is to generate V8AutoGeneratedHiddenPropertyName.h, which includes all V(fooName) entries used in the generated code, and have V8HiddenPropertyName.h include V8AutoGeneratedHiddenPropertyName.h. But this requires non-trivial change in the code generator.

Anyway, V8DOMWrapper::setNamedHiddenWindowReference() in the generated code is in a cold path. It won't worth replacing it with V8HiddenPropertyName::fooName(). Let me mark this bug as WONTFIX.