Bug 111971 - [V8] Get rid of function-level static FunctionTemplates in generated bindings code
Summary: [V8] Get rid of function-level static FunctionTemplates in generated bindings...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Marja Hölttä
URL:
Keywords:
Depends on: 112117
Blocks: 111724
  Show dependency treegraph
 
Reported: 2013-03-11 04:04 PDT by Marja Hölttä
Modified: 2013-03-12 03:49 PDT (History)
5 users (show)

See Also:


Attachments
Patch (19.75 KB, patch)
2013-03-11 06:15 PDT, Marja Hölttä
marja: review-
Details | Formatted Diff | Diff
Patch (17.80 KB, patch)
2013-03-11 08:39 PDT, Marja Hölttä
no flags Details | Formatted Diff | Diff
Patch (17.85 KB, patch)
2013-03-12 02:53 PDT, Marja Hölttä
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marja Hölttä 2013-03-11 04:04:09 PDT
When we create and store function templates for main world and non-main worlds separately (see bug 111724), having function templates as static variables inside functions breaks it. (We cannot make it initialize with some world type when the execution runs through that line of code for the first time, and use it later for a different world type when the function is called again.)

The patch (which will be attached soon) gets rid of these function-level statics and stores the functiontemplates inside V8PerIsolateData instead.
Comment 1 Marja Hölttä 2013-03-11 06:15:07 PDT
Created attachment 192456 [details]
Patch
Comment 2 Kentaro Hara 2013-03-11 06:55:45 PDT
Comment on attachment 192456 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:807
> +    // This is only for getting a unique pointer which we can pass to privateTemplateMap.
> +    static String privateTemplateUniqueKey = "${funcName}PrivateTemplate";
> +    WrapperWorldType currentWorldType = worldType(info.GetIsolate());
> +    V8PerIsolateData::PrivateTemplateMap& templateMap = V8PerIsolateData::from(info.GetIsolate())->privateTemplateMap(currentWorldType);
> +    v8::Persistent<v8::FunctionTemplate> privateTemplate;
> +    V8PerIsolateData::PrivateTemplateMap::iterator result = templateMap.find(&privateTemplateUniqueKey);
> +    if (result == templateMap.end()) {
> +        privateTemplate = v8::Persistent<v8::FunctionTemplate>::New(info.GetIsolate(), $newTemplateString);
> +        templateMap.add(&privateTemplateUniqueKey, privateTemplate);
> +    } else
> +        privateTemplate = result->value;
> +
> +    v8::Handle<v8::Object> holder = info.This()->FindInstanceInPrototypeChain(${v8InterfaceName}::GetTemplate(info.GetIsolate(), currentWorldType));

You are duplicating the code to multiple places in V8LocationCustom.cpp. Shall we create a helper method that does the work in V8PerIsolateData.cpp ?

Just to confirm: This change won't affect any performance-sensitive DOM attributes, right?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:73
> +    typedef HashMap<void*, v8::Persistent<v8::FunctionTemplate> > PrivateTemplateMap;

Maybe PerWorldTemplateMap is a better name?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:75
> +    PrivateTemplateMap& privateTemplateMap(WrapperWorldType worldType)

perWorldTemplateMap ?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:147
> +    PrivateTemplateMap m_privateTemplatesForMainWorld;

m_perWorldTemplatesForMainWorld ?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:148
> +    PrivateTemplateMap m_privateTemplatesForNonMainWorld;

m_perWorldTemplatesForNonMainWorld ?

Let's add a comment about what "NonMainWorld" means, i.e. let's say that this template map can be used for both an isolated world and a worker world.

> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:198
> +        static String sharedTemplateUniqueKey = "$replaceSharedTemplate";

"$replaceSharedTemplate" => "replaceSharedTemplate"

A helper method will resolve your typo:)

> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:234
> +        static String sharedTemplateUniqueKey = "${funcName}SharedTemplate";

Ditto.
Comment 3 Marja Hölttä 2013-03-11 08:39:12 PDT
Created attachment 192476 [details]
Patch
Comment 4 Marja Hölttä 2013-03-11 08:39:24 PDT
Comment on attachment 192456 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:807
>> +    v8::Handle<v8::Object> holder = info.This()->FindInstanceInPrototypeChain(${v8InterfaceName}::GetTemplate(info.GetIsolate(), currentWorldType));
> 
> You are duplicating the code to multiple places in V8LocationCustom.cpp. Shall we create a helper method that does the work in V8PerIsolateData.cpp ?
> 
> Just to confirm: This change won't affect any performance-sensitive DOM attributes, right?

Helper method created.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:73
>> +    typedef HashMap<void*, v8::Persistent<v8::FunctionTemplate> > PrivateTemplateMap;
> 
> Maybe PerWorldTemplateMap is a better name?

This no longer exists, everything is stored in the same map, as discussed.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:75
>> +    PrivateTemplateMap& privateTemplateMap(WrapperWorldType worldType)
> 
> perWorldTemplateMap ?

The same here.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:147
>> +    PrivateTemplateMap m_privateTemplatesForMainWorld;
> 
> m_perWorldTemplatesForMainWorld ?

This no longer exists.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:148
>> +    PrivateTemplateMap m_privateTemplatesForNonMainWorld;
> 
> m_perWorldTemplatesForNonMainWorld ?
> 
> Let's add a comment about what "NonMainWorld" means, i.e. let's say that this template map can be used for both an isolated world and a worker world.

This no longer exists.

>> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:198
>> +        static String sharedTemplateUniqueKey = "$replaceSharedTemplate";
> 
> "$replaceSharedTemplate" => "replaceSharedTemplate"
> 
> A helper method will resolve your typo:)

Fixed

>> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:234
>> +        static String sharedTemplateUniqueKey = "${funcName}SharedTemplate";
> 
> Ditto.

Fixed.
Comment 5 Kentaro Hara 2013-03-11 18:23:58 PDT
Comment on attachment 192476 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:809
> +        static String sharedTemplateUniqueKey = "${funcName}SharedTemplate";
> +        v8::Persistent<v8::FunctionTemplate> sharedTemplate = V8PerIsolateData::from(info.GetIsolate())->privateTemplate(currentWorldType, &sharedTemplateUniqueKey, $newTemplateParams);

I understand that we need to use a different template from 'privateTemplate', but still I don't understand why it is called 'sharedTemplate' (What does 'shared' mean?). We might want a more descriptive name.

Either way you can fix it in a follow-up patch.

> Source/WebCore/bindings/v8/V8PerIsolateData.h:68
> +    typedef HashMap<void*, v8::Persistent<v8::FunctionTemplate> > TemplateMap;

Do you have a plan to use any key whose type is not a string? (Although we discussed it offline, I forgot where we reached:-)

Otherwise, you can use a string key for safety. void* is hacky (and might be vulnerable).
Comment 6 Marja Hölttä 2013-03-12 00:30:23 PDT
Comment on attachment 192476 [details]
Patch

Offline discussion: void* needs to be void*, since WrapperTypeInfo* pointers are also used as a key. Renaming private & shared left out of this patch.
Comment 7 WebKit Review Bot 2013-03-12 00:54:44 PDT
Comment on attachment 192476 [details]
Patch

Clearing flags on attachment: 192476

Committed r145494: <http://trac.webkit.org/changeset/145494>
Comment 8 WebKit Review Bot 2013-03-12 00:54:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2013-03-12 02:06:33 PDT
Re-opened since this is blocked by bug 112117
Comment 10 Marja Hölttä 2013-03-12 02:53:19 PDT
Created attachment 192685 [details]
Patch
Comment 11 Marja Hölttä 2013-03-12 02:55:01 PDT
The new patch changes static String -> static const char*; no other changes.
Comment 12 jochen 2013-03-12 02:59:08 PDT
Comment on attachment 192685 [details]
Patch

rs=me
Comment 13 WebKit Review Bot 2013-03-12 03:49:25 PDT
Comment on attachment 192685 [details]
Patch

Clearing flags on attachment: 192685

Committed r145511: <http://trac.webkit.org/changeset/145511>
Comment 14 WebKit Review Bot 2013-03-12 03:49:30 PDT
All reviewed patches have been landed.  Closing bug.