Bug 99129 – [V8] PerContextEnabled methods should be installed when the constructor is created

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Currently PerContextEnabled methods are installed for each wrapper instead of for each prototype object,
even though the method is installed into the prototype.
This can be a performance waste when there are many wrappers for a same prototype object.

[Hajime Morrita]

Created attachment 168362 [details]
Patch

[Hajime Morrita]

Haraken, could you take a look?

[Kentaro Hara]

Comment on attachment 168362 [details]
Patch

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

The overall approach looks good.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066
> +WrapperTypeInfo V8${implClassName}Constructor::info = { V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject, V8${implClassName}::toActiveDOMObject, 0, V8${implClassName}::installPerContextPrototypeProperties, 0, WrapperTypeObjectPrototype };

Can't you pass 0 instead of passing an empty function? Then you can avoid generating empty functions.

[Hajime Morrita]

Hi Haraken, thanks for reviewing!

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066
> > +WrapperTypeInfo V8${implClassName}Constructor::info = { V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject, V8${implClassName}::toActiveDOMObject, 0, V8${implClassName}::installPerContextPrototypeProperties, 0, WrapperTypeObjectPrototype };
> 
> Can't you pass 0 instead of passing an empty function? Then you can avoid generating empty functions.
I cannot. In precise, I can do so for this patch. But I have another patch which make it harder, unfortunately.

[Kentaro Hara]

Comment on attachment 168362 [details]
Patch

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

This patch will affect the performance of toV8Slow(). Before landing, please confirm that this patch (+ other patches you're trying to land) won't regress performance of Bindings/create-element.html and Dromaeo DOM tests.

> Source/WebCore/ChangeLog:16
> +        generated method, which is desined to be called for each time when the prototype

Nit: desined => designed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:845
> +    if ($implClassName eq "DOMWindow") {
> +        push(@implContentDecls, <<END);
> +    return perContextData->constructorForType(WrapperTypeInfo::unwrap(data), V8DOMWindow::toNative(info.Holder())->document());
> +END
> +    } elsif ($implClassName eq "WorkerContext") {
> +        push(@implContentDecls, <<END);
> +    return perContextData->constructorForType(WrapperTypeInfo::unwrap(data), V8WorkerContext::toNative(info.Holder()));
> +END
> +    } else {
> +        die "Unknown Context ${implClassName}"
> +    }
> +    push(@implContentDecls, <<END);
> +}

Nit: I would prefer push(@implContentDecls, "...") rather than here documents for one-line code.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066
>> +WrapperTypeInfo V8${implClassName}Constructor::info = { V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject, V8${implClassName}::toActiveDOMObject, 0, V8${implClassName}::installPerContextPrototypeProperties, 0, WrapperTypeObjectPrototype };
> 
> Can't you pass 0 instead of passing an empty function? Then you can avoid generating empty functions.

I discussed with morrita-san offline. I understood why you want to pass an empty function.

[Hajime Morrita]

Created attachment 168376 [details]
Patch for landing

[Hajime Morrita]

Hi Haraken, thanks for quick review!

(In reply to comment #5)
> (From update of attachment 168362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168362&action=review
> 
> This patch will affect the performance of toV8Slow(). Before landing, please confirm that this patch (+ other patches you're trying to land) won't regress performance of Bindings/create-element.html and Dromaeo DOM tests.

Done. I don't see any slowdown.

[WebKit Review Bot]

Comment on attachment 168376 [details]
Patch for landing

Rejecting attachment 168376 [details] from commit-queue.

New failing tests:
fast/notifications/notifications-with-permission.html
Full output: http://queues.webkit.org/results/14266248

[Hajime Morrita]

Created attachment 168384 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168384 [details]
Patch for landing

Clearing flags on attachment: 168384

Committed r131167: <http://trac.webkit.org/changeset/131167>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

(In reply to comment #10)
> (From update of attachment 168384 [details])
> Clearing flags on attachment: 168384
> 
> Committed r131167: <http://trac.webkit.org/changeset/131167>


Bindings-tests failed on multiple bots after the patch:

Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839.

gcc: error trying to exec 'cc1obj': execvp: No such file or directory

[Kentaro Hara]

(In reply to comment #12)
> gcc: error trying to exec 'cc1obj': execvp: No such file or directory

I would guess you need to install gobjc.

[Csaba Osztrogonác]

(In reply to comment #13)
> (In reply to comment #12)
> > gcc: error trying to exec 'cc1obj': execvp: No such file or directory
> 
> I would guess you need to install gobjc.

Hm? What changed why we need one more dependency?
I haven't seen a heads-up mail on webkit-dev about it.

[Csaba Osztrogonác]

Otherwise bindings tests fail on _all_ bots: Chromium, Mac, Qt, ...
Could you update the result please?

[Kentaro Hara]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > gcc: error trying to exec 'cc1obj': execvp: No such file or directory
> > 
> > I would guess you need to install gobjc.
> 
> Hm? What changed why we need one more dependency?
> I haven't seen a heads-up mail on webkit-dev about it.

Nothing changed. I think the error is not related to this change. I've observed the error when I've not installed gobjc.

On what bots is the error happening?

[Kentaro Hara]

(In reply to comment #15)
> Bindings-tests failed on multiple bots after the patch:
> 
> Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839.

On the other hand, this is undoubtedly the error caused by this patch. Let me check it.

[Csaba Osztrogonác]

The "gcc: error trying to exec 'cc1obj': execvp: No such file or directory" error message isn't related to this patch, it is present long time ago. (at least on the Qt bot)

But the "Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839." error message introduced in r131167 on all platform.

[Kentaro Hara]

Makes sense.

> But the "Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839." error message introduced in r131167 on all platform.

This will be fixed in minutes.

> The "gcc: error trying to exec 'cc1obj': execvp: No such file or directory" error message isn't related to this patch, it is present long time ago. (at least on the Qt bot)

This will be fixed by installing gobjc to the bots.

[Kentaro Hara]

(In reply to comment #19)
> Makes sense.
> 
> > But the "Unknown Context TestObj at WebCore/bindings/scripts/CodeGeneratorV8.pm line 839." error message introduced in r131167 on all platform.
> 
> This will be fixed in minutes.

Fixed in r131180. Sorry for the trouble.

[Kentaro Hara]

Comment on attachment 168384 [details]
Patch for landing

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:840
> +    } else {
> +        die "Unknown Context ${implClassName}"
> +    }

morrita-san: In r131180, as a quick fix, I commented out this code to hide a run-bindings-tests failure for TestObj.idl (which contains methods with [V8EnabledPerContext]). Please fix the code in a more reasonable way.