Bug 101110 – [V8] IntrusiveDOMWrapperMap should be usable for more than just Nodes

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

[Adam Barth]

[V8] IntrusiveDOMWrapperMap should be usable for more than just Nodes

[Adam Barth]

Created attachment 172153 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 172153 [details]
Patch

LGTM.

[Adam Barth]

*** Bug 101121 has been marked as a duplicate of this bug. ***

[Adam Barth]

Created attachment 172376 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.h:56:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.h:59:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.h:58:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.h:62:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/v8/DOMDataStore.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.h:57:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h:63:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.h:58:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h:58:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestException.h:56:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.h:59:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 12 in 22 files


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

[Kentaro Hara]

Comment on attachment 172376 [details]
Patch

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

super cool!

> Source/WebCore/ChangeLog:9
> +        to be useable for more than just nodes. After this patch, any object

Typo: usable

> Source/WebCore/bindings/v8/DOMDataStore.cpp:59
> +    if (!isolate)
> +        isolate = v8::Isolate::GetCurrent();
>      V8PerIsolateData* data = V8PerIsolateData::from(isolate);

You can use V8PerIsolateData::current(isolate = 0)

> Source/WebCore/bindings/v8/DOMDataStore.cpp:75
> +{

You can add ASSERT(m_type == MainWorld).

> Source/WebCore/bindings/v8/DOMDataStore.cpp:81
> +    // Note: |object| might not be equal to |key|, e.g., if ScriptWrappable isn't a left-most base class.

Nit: This comment could be put on the above line.

> Source/WebCore/bindings/v8/DOMDataStore.h:67
> +        if (wrapperIsStoredInObject(object))
> +            return getWrapperFromObject(object);
> +        return m_hashMap.get(object);

How about killing wrapperIsStoredInObject() and writing the code like this?

v8::Handle<v8::Object> wrapper = getWrapperFromObject(object);
if (!wrapper.IsEmpty())
    return wrapper;
return m_hashMap.get(object);

> Source/WebCore/bindings/v8/DOMDataStore.h:81
>  protected:

private: ?

> Source/WebCore/bindings/v8/DOMDataStore.h:83
> +    inline bool wrapperIsStoredInObject(void*) const { return false; }
> +    inline bool wrapperIsStoredInObject(ScriptWrappable*) const { return m_type == MainWorld; }

As mentioned above, I think you can remove these methods.

> Source/WebCore/bindings/v8/DOMDataStore.h:86
> +    inline v8::Handle<v8::Object> getWrapperFromObject(ScriptWrappable* key) const { return key->wrapper(); }

You can add ASSERT(m_type == MainWorld).

> Source/WebCore/bindings/v8/DOMDataStore.h:89
> +    inline bool storeWrapperInObject(void*, v8::Persistent<v8::Object>) { return false; }
> +    inline bool storeWrapperInObject(ScriptWrappable* key, v8::Persistent<v8::Object> wrapper)

setWrapperInObject() would be better. (c.f. getWrapperFromObject())

> Source/WebCore/bindings/v8/DOMDataStore.h:101
>      Type m_type;

m_worldType might be better. You can also rename Type to WorldType.

> Source/WebCore/bindings/v8/DOMDataStore.h:102
> +    DOMWrapperHashMap<void> m_hashMap;

m_wrapperMap might be better.

[Eric Seidel (no email)]

Comment on attachment 172376 [details]
Patch

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

> Source/WebCore/bindings/v8/DOMDataStore.h:58
>      explicit DOMDataStore(Type);
>      ~DOMDataStore();

I think we should add a little blurb to the top of this function which says that callers need to be careful to always call these functions with specific types.  Explain that we're depending on the compiler's type system to dispatch to the right function, and that if we've lost our type (say void*) we'll end up down the hash-map path always, even if we should be going down the ScriptWrappable path.

[Adam Barth]

Comment on attachment 172376 [details]
Patch

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

>> Source/WebCore/bindings/v8/DOMDataStore.cpp:75
>> +{
> 
> You can add ASSERT(m_type == MainWorld).

Yes, expect that this function is static.  :)

>> Source/WebCore/bindings/v8/DOMDataStore.h:58
>>      ~DOMDataStore();
> 
> I think we should add a little blurb to the top of this function which says that callers need to be careful to always call these functions with specific types.  Explain that we're depending on the compiler's type system to dispatch to the right function, and that if we've lost our type (say void*) we'll end up down the hash-map path always, even if we should be going down the ScriptWrappable path.

Done.

>> Source/WebCore/bindings/v8/DOMDataStore.h:67
>> +        return m_hashMap.get(object);
> 
> How about killing wrapperIsStoredInObject() and writing the code like this?
> 
> v8::Handle<v8::Object> wrapper = getWrapperFromObject(object);
> if (!wrapper.IsEmpty())
>     return wrapper;
> return m_hashMap.get(object);

I wasn't sure whether the compiler would be smart enough to optimize that away in the non-ScriptWrappable case.  It might think that creating an empty handle has side effects.

>> Source/WebCore/bindings/v8/DOMDataStore.h:101
>>      Type m_type;
> 
> m_worldType might be better. You can also rename Type to WorldType.

It can also be "Worker", which isn't really a world type.

[Adam Barth]

I've made all other changes locally.

[Adam Barth]

Created attachment 172396 [details]
Patch for landing

[Adam Barth]

Comment on attachment 172396 [details]
Patch for landing

Clearing flags on attachment: 172396

Committed r133526: <http://trac.webkit.org/changeset/133526>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

I'm pretty sure this patch caused a 15% memory regression.

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?rev=166240&graph=ws_single_peak_r&history=100

Regression range: http://trac.webkit.org/log/?verbose=on&rev=133531&stop_rev=133523

http://trac.webkit.org/changeset/133529 has already been rolled out. I don't see any other patches in that range other than this one that could possibly be it.

Rollout?

[Eric Seidel (no email)]

How can we repro that regression locally?  This set of changes should be causing memory progressions, but it's possible something was missed here...

[WebKit Review Bot]

Re-opened since this is blocked by bug 101388

[Adam Barth]

> This set of changes should be causing memory progressions, but it's possible something was missed here...

The only think I can think of is if somehow the overloading isn't working the way we think it is.  However, it's not clear how that could be without showing up on a performance or correctness test.

[Adam Barth]

Something is very wrong with DOM performance:

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dom_perf/report.html?history=150&rev=-1

[Adam Barth]

I bet it is the V8 roll:

http://src.chromium.org/viewvc/chrome?view=rev&revision=166220

[Adam Barth]

The V8 roll is being reverted.

[Adam Barth]

As far as I can tell, this stat isn't important.  It's just the working set size for the render process.  If the time and total commit charge don't change, what does it matter that the working set size has changed?  The stat is even marked as not important in perf_test.cc.

[Adam Barth]

I couldn't find anyone who could answer my questions authoritatively, but my guess is that we track this stat to help us diagnose regressions in "times".  Notice that we don't have expectations set for this value in http://src.chromium.org/viewvc/chrome/trunk/src/tools/perf_expectations/perf_expectations.json either.

[Adam Barth]

As far as I can tell, this is a non-problem.  Maybe I'm misunderstanding the significance of this chart?

[Ojan Vafai]

I thought working set == amount of physical memory used on Windows, which is exactly what we care about WRT memory usage. A cursory Google search seems to back up that theory. Maybe my understanding is wrong though.

[Adam Barth]

The chart went back down when the patch was rolled out: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1&graph=ws_single_peak_r

[Ojan Vafai]

lol whoops. nevermind. i misread it and thought that patch had been processed in the previous run.

[Ojan Vafai]

(In reply to comment #25)
> lol whoops. nevermind. i misread it and thought that patch had been processed in the previous run.

omg. ignore me. it is this patch that caused the regression.

[Adam Barth]

I'm going to re-land the easy parts of this change to reduce the size of the diff that could have caused the graph to move.

[Adam Barth]

Created attachment 172855 [details]
Patch

[Kentaro Hara]

Comment on attachment 172855 [details]
Patch

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

> Source/WebCore/bindings/v8/DOMDataStore.h:63
> -    inline v8::Handle<v8::Object> get(Node* object) const
> +    inline v8::Handle<v8::Object> get(ScriptWrappable* object) const

I'm pretty sure it will work, but if something in your original patch is the culprit of the regression, I might want to suspect this part (just an intuition).

[Adam Barth]

(In reply to comment #29)
> (From update of attachment 172855 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172855&action=review
> 
> > Source/WebCore/bindings/v8/DOMDataStore.h:63
> > -    inline v8::Handle<v8::Object> get(Node* object) const
> > +    inline v8::Handle<v8::Object> get(ScriptWrappable* object) const
> 
> I'm pretty sure it will work, but if something in your original patch is the culprit of the regression, I might want to suspect this part (just an intuition).

Yeah.  The only other thing that is left is changing from OwnPtr to holding the Map directly (and can't see how that would make a difference).

[Adam Barth]

Created attachment 172876 [details]
Patch

[Adam Barth]

I changed this patch to be more similar to the original one because we need to delay the static_cast to ScriptWrappable* until after we decide to use the inline wrapper.

[Adam Barth]

Committed r133810: <http://trac.webkit.org/changeset/133810>

[WebKit Review Bot]

Re-opened since this is blocked by bug 101520

[Adam Barth]

Created attachment 172902 [details]
Hopefully final patch

[WebKit Review Bot]

Comment on attachment 172902 [details]
Hopefully final patch

Clearing flags on attachment: 172902

Committed r133837: <http://trac.webkit.org/changeset/133837>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.