Bug 110875 – [V8] Add a "context type" parameter to GetTemplate and ConfigureV8SomethingTemplate functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marja Hölttä]

The context type can be one of {"main world", "isolated world", "worker"}.

The added parameter will later be used for
- retrieving the correct FunctionTemplate, once we have separate ones for separate context types
- generating a FunctionTemplate specific to the context type, and it will possibly have custom generated attribute getters and callbacks (which in turn will be more efficient than the non-customized ones, because they know in which of the three case we're in, so they don't need code for checking it.

See bug 110874 for the overall plan.

[Marja Hölttä]

Created attachment 190276 [details]
Patch

[Dan Carney]

Seems good.

Calls to V8DOMWindow::GetTemplate should probably use a different call like getMainIsolateContextType or something that doesn't check for workers

Run dromeao and binding performance tests and verify there are no regressions here.  If so, we might need to get tricky about how we do the checks.

[Marja Hölttä]

Created attachment 190291 [details]
Patch

[Kentaro Hara]

Comment on attachment 190291 [details]
Patch

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

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:50
> +    enum WrapperContextType {

Maybe WrapperWorldType is better?

[Adam Barth]

Comment on attachment 190291 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:477
> +    WrapperContextType getContextType(v8::Isolate*);

"get" <--- we usually skip the "get" prefix since it doesn't mean much.

[Adam Barth]

Comment on attachment 190291 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.cpp:316
> +        DOMWrapperWorld* isolatedWorld = DOMWrapperWorld::getWorld(v8::Context::GetEntered());

Does this always work?  I wonder if there's a case where we need to do an "instance of" check but we aren't in a context...

[Kentaro Hara]

Comment on attachment 190291 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8Binding.cpp:316
>> +        DOMWrapperWorld* isolatedWorld = DOMWrapperWorld::getWorld(v8::Context::GetEntered());
> 
> Does this always work?  I wonder if there's a case where we need to do an "instance of" check but we aren't in a context...

Yeah, v8::Context::GetEntered() can be empty, which might happen in GetTemplate()/GetRawTemplate() (I'm not sure, please check).

Also, I think you can implement getContextType() more easily by using V8Binding::worldForEnteredContext().

[Marja Hölttä]

Created attachment 190716 [details]
Patch.

[WebKit Review Bot]

Attachment 190716 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/v8/DOMWrapperWorld.cpp', u'Source/WebCore/bindings/v8/DOMWrapperWorld.h', u'Source/WebCore/bindings/v8/Dictionary.cpp', u'Source/WebCore/bindings/v8/PageScriptDebugServer.cpp', u'Source/WebCore/bindings/v8/V8AdaptorFunction.cpp', u'Source/WebCore/bindings/v8/V8AdaptorFunction.h', u'Source/WebCore/bindings/v8/V8Binding.cpp', u'Source/WebCore/bindings/v8/V8Binding.h', u'Source/WebCore/bindings/v8/V8DOMWindowShell.cpp', u'Source/WebCore/bindings/v8/V8DOMWrapper.cpp', u'Source/WebCore/bindings/v8/V8Initializer.cpp', u'Source/WebCore/bindings/v8/V8PerContextData.cpp', u'Source/WebCore/bindings/v8/WorkerScriptController.cpp', u'Source/WebCore/bindings/v8/WrapperTypeInfo.h', u'Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.h', u'Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp', u'Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp']" exit_code: 1
Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marja Hölttä]

Created attachment 190718 [details]
Patch.

[Marja Hölttä]

Created attachment 190720 [details]
Patch.

[WebKit Review Bot]

Attachment 190720 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/v8/DOMWrapperWorld.cpp', u'Source/WebCore/bindings/v8/DOMWrapperWorld.h', u'Source/WebCore/bindings/v8/Dictionary.cpp', u'Source/WebCore/bindings/v8/PageScriptDebugServer.cpp', u'Source/WebCore/bindings/v8/V8AdaptorFunction.cpp', u'Source/WebCore/bindings/v8/V8AdaptorFunction.h', u'Source/WebCore/bindings/v8/V8Binding.cpp', u'Source/WebCore/bindings/v8/V8Binding.h', u'Source/WebCore/bindings/v8/V8DOMWindowShell.cpp', u'Source/WebCore/bindings/v8/V8DOMWrapper.cpp', u'Source/WebCore/bindings/v8/V8Initializer.cpp', u'Source/WebCore/bindings/v8/V8PerContextData.cpp', u'Source/WebCore/bindings/v8/WorkerScriptController.cpp', u'Source/WebCore/bindings/v8/WrapperTypeInfo.h', u'Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.h', u'Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp', u'Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp']" exit_code: 1
Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 190720 [details]
Patch.

Attachment 190720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16782411

New failing tests:
fast/dom/title-directionality.html

[Kentaro Hara]

Nit: WebKit conventionally use [<<port>>] prefix in the bug title.

[Kentaro Hara]

Comment on attachment 190720 [details]
Patch.

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

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:44
> +static bool initializingWindow = false;

How about changing this to 'windowInitialized'? Then you can set false initially and flip it at the point where the initialization is completed.

> Source/WebCore/bindings/v8/V8Binding.cpp:316
> +    V8PerIsolateData* data = isolate ? V8PerIsolateData::from(isolate) : V8PerIsolateData::current();

When can the isolate be 0 ?

> Source/WebCore/bindings/v8/V8Binding.cpp:318
> +    if (UNLIKELY(!!data->domDataStore()))

Remove UNLIKELY unless you can observe any performance gain by adding UNLIKELY.

> Source/WebCore/bindings/v8/V8Binding.cpp:322
> +        DOMWrapperWorld* isolatedWorld = DOMWrapperWorld::getWorld(v8::Context::GetEntered());

We want to add ASSERT(!v8::Context::GetEntered().IsEmpty()).

> Source/WebCore/bindings/v8/V8Binding.cpp:329
> +WrapperWorldType mainIsolateWorldType(v8::Isolate* isolate)

Maybe 'worldTypeForMainThread' is a better name?

> Source/WebCore/bindings/v8/V8Binding.cpp:332
> +        DOMWrapperWorld* isolatedWorld = DOMWrapperWorld::getWorld(v8::Context::GetEntered());

Ditto.

[Marja Hölttä]

Created attachment 190918 [details]
Patch.

[Marja Hölttä]

Comment on attachment 190720 [details]
Patch.

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

>> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:44
>> +static bool initializingWindow = false;
> 
> How about changing this to 'windowInitialized'? Then you can set false initially and flip it at the point where the initialization is completed.

As discussed: That won't work, we'll run into problems when the next DOMWindow is getting initialized (after the previous one has finished initializing).

>> Source/WebCore/bindings/v8/V8Binding.cpp:316
>> +    V8PerIsolateData* data = isolate ? V8PerIsolateData::from(isolate) : V8PerIsolateData::current();
> 
> When can the isolate be 0 ?

Added a TODO here to assert that it can't be -> follow-up change.

>> Source/WebCore/bindings/v8/V8Binding.cpp:318
>> +    if (UNLIKELY(!!data->domDataStore()))
> 
> Remove UNLIKELY unless you can observe any performance gain by adding UNLIKELY.

Done.

>> Source/WebCore/bindings/v8/V8Binding.cpp:322
>> +        DOMWrapperWorld* isolatedWorld = DOMWrapperWorld::getWorld(v8::Context::GetEntered());
> 
> We want to add ASSERT(!v8::Context::GetEntered().IsEmpty()).

Done.

>> Source/WebCore/bindings/v8/V8Binding.cpp:329
>> +WrapperWorldType mainIsolateWorldType(v8::Isolate* isolate)
> 
> Maybe 'worldTypeForMainThread' is a better name?

Chatted with dcarney -> we don't think that it's a better name, but let me know if you feel strongly about this.

>> Source/WebCore/bindings/v8/V8Binding.cpp:332
>> +        DOMWrapperWorld* isolatedWorld = DOMWrapperWorld::getWorld(v8::Context::GetEntered());
> 
> Ditto.

Done.

[Marja Hölttä]

Also, a note about performance tests:

I ran the Bindings and Dromaeo perf tests locally. Most tests don't show any reproducible regression. There's one test, Bindings/gc-tree.html that seems to regress, but we verified by putting counters on the added functions, that they shouldn't be the problem. E.g., mainIsolateWorldType is called ~ 250 times during the test. So this patch shouldn't be related. And the performance gain of the separate generated bindings (which will be the next patch) should outweigh it anyway.

[Kentaro Hara]

Comment on attachment 190918 [details]
Patch.

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

Almost LGTM

> Source/WebCore/ChangeLog:7
> +

Please describe the rationale of the change so that others can understand what you are doing.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:44
> +static bool initializingWindow = false;

How about making this a static variable of DOMWrapperWorld?

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:51
> +void DOMWrapperWorld::setInitializingWindow(bool initializing)

We normally write a simple setter in a header file.

> Source/WebCore/bindings/v8/V8Binding.cpp:315
> +{

You might want to add ASSERT(!initializingWindow).

> Source/WebCore/bindings/v8/V8Binding.cpp:316
> +    // TODO: Assert that isolate is not 0.

When can it happen? As far as I see the patch, isolate is always non-0.

> Source/WebCore/bindings/v8/V8Binding.h:478
> +    WrapperWorldType mainIsolateWorldType(v8::Isolate*);

At least, let's rename to mainIsolatedWorldType.

[Adam Barth]

Comment on attachment 190918 [details]
Patch.

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

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:54
> +    enum WrapperWorldType {
> +        MainWorld,
> +        IsolatedWorld,
> +        WorkerWorld
> +    };

This enum already exists as DOMDataStore::Type.  We should unify them somehow.

[Dan Carney]

> 
> > Source/WebCore/bindings/v8/V8Binding.h:478
> > +    WrapperWorldType mainIsolateWorldType(v8::Isolate*);
> 
> At least, let's rename to mainIsolatedWorldType.

I don't care which it's called, except that It has to refer to either the mainThread or the mainIsolate.  It has to distinguish worker isolates from non worker isolates.  mainIsolatedWorld has no meaning.

[Kentaro Hara]

(In reply to comment #21)
> > 
> > > Source/WebCore/bindings/v8/V8Binding.h:478
> > > +    WrapperWorldType mainIsolateWorldType(v8::Isolate*);
> > 
> > At least, let's rename to mainIsolatedWorldType.
> 
> I don't care which it's called, except that It has to refer to either the mainThread or the mainIsolate.  It has to distinguish worker isolates from non worker isolates.  mainIsolatedWorld has no meaning.

I've discussed it with marja offline, and we decided to use worldTypeInMainThread.

[Dan Carney]

> This enum already exists as DOMDataStore::Type.  We should unify them somehow.

It's true, the point on the patch is to be able to access the correct DOMDataStore faster, so unification makes a ton of sense.  We'll need to use the same variable in the functiontemplate caches, so it'll need to be moved somewhere that makes sense for both uses cases.

[Marja Hölttä]

Created attachment 191182 [details]
Patch

[Kentaro Hara]

Comment on attachment 191182 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

For this kind of non-trivial changes, please explain your change (why you are making the change & what the change is doing).

> Source/WebCore/bindings/v8/V8Binding.cpp:316
> +    ASSERT(isolate);

Nit: Remove this. It is already asserted in V8PerIsolateData::from().

> Source/WebCore/bindings/v8/V8Binding.cpp:318
> +    // TODO: Rename domDataStore() to workerDataStore().

Nit: TODO => FIXME (This is a coding style of WebKit:-)

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:54
> +    enum WrapperWorldType {
> +        MainWorld,
> +        IsolatedWorld,
> +        WorkerWorld
> +    };

I prefer putting the enum in DOMWrapperWorld.h because it is about DOMWrapperWorld. Maybe is it hard for you due to build dependency?

[Dan Carney]

> I prefer putting the enum in DOMWrapperWorld.h because it is about DOMWrapperWorld. Maybe is it hard for you due to build dependency?

It is about domwrapperworld and the functiontemplate caches.  I think DOMWrapperWorld is no longer the best place for it.

[Dan Carney]

(In reply to comment #26)
> > I prefer putting the enum in DOMWrapperWorld.h because it is about DOMWrapperWorld. Maybe is it hard for you due to build dependency?

sorry i meant to say domwrappermap and the functiontemplate caches.  domwrapperworld knows nothing about workers.  it is a main thread class, which is why the enum doesn't really makes sense there

[Kentaro Hara]

(In reply to comment #27)
> (In reply to comment #26)
> > > I prefer putting the enum in DOMWrapperWorld.h because it is about DOMWrapperWorld. Maybe is it hard for you due to build dependency?
> 
> sorry i meant to say domwrappermap and the functiontemplate caches.  domwrapperworld knows nothing about workers.  it is a main thread class, which is why the enum doesn't really makes sense there

Makes sense. (Maybe we should rename DOMWrapperWorld to DOMIsolatedWorld (anyway in a follow-up patch)?)

[Marja Hölttä]

Created attachment 191186 [details]
Patch.

[Kentaro Hara]

Comment on attachment 191186 [details]
Patch.

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

LGTM. Thanks for the patch!

> Source/WebCore/ChangeLog:9
> +        The parameter will later be used for generating specialized V8
> +        bindings for the 3 different world types (main world, isolated
> +        work, worker).
> +

We normally write the description between the "Reviewed By" line and the "Test:" line. Please do it from your next patch.

[WebKit Review Bot]

Comment on attachment 191186 [details]
Patch.

Clearing flags on attachment: 191186

Committed r144617: <http://trac.webkit.org/changeset/144617>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #31)
> (From update of attachment 191186 [details])
> Clearing flags on attachment: 191186
> 
> Committed r144617: <http://trac.webkit.org/changeset/144617>

It broke the bindings generation tests. Could you fix it, please?

(In reply to comment #33)
> (In reply to comment #31)
> > (From update of attachment 191186 [details] [details])
> > Clearing flags on attachment: 191186
> > 
> > Committed r144617: <http://trac.webkit.org/changeset/144617>
> 
> It broke the bindings generation tests. Could you fix it, please?

fixed in http://trac.webkit.org/changeset/144651

[Ankur Taly]

I am curious to know why the WorldWrapperType is not passed to the GetRawTemplate and HasInstance methods in the generated bindings?

My understanding is as follows:
The GetTemplate method invokes GetRawTemplate, and raw templates gets cached in PerIsolateData. So going forward we will need 3 different template and raw template caches in PerIsolateData. Therefore GetRawTemplate must have the WorldWrapperType argument so that it can select the correct cache. Since HasInstance invokes GetRawTemplate, it should also have the WorldWrapperType argument.

Am I missing something?

[Marja Hölttä]

Adding the world type parameter to GetRawTemplate and GetTemplate is the next step (patch in bug 111724). HasInstance doesn't need a world type parameter, because it already gets an isolate, and can use worldType() to figure out the world type.

[Ankur Taly]

Thanks for the clarification.

Another related question: Invoking the HasInstance method in the generated bindings could potentially construct a new raw FunctionTemplate (if it does not find it the rawTemplateMap cache in PerIsolateData) and then invoke the HasInstance method on it. However the HasInstance call on this new FunctionTemplate will always return false.

I wonder if we should define HasInstance so that it never constructs a new template? Furthermore, HasInstance invocations appear inside various ASSERT statements, so ideally they should all be side-effect free.

Does this make sense?

[Marja Hölttä]

I don't have an opinion about the HasInstance; I'll let more experienced WebKit developers answer... dcarney?