Summary: | [V8] IntrusiveDOMWrapperMap should be usable for more than just Nodes | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aboxhall, eric.carlson, eric, feature-media-reviews, gyuyoung.kim, haraken, japhet, jsbell, ojan, rakuco, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 101388, 101413, 101422, 101430, 101493, 101494, 101520, 101523, 101525 | ||||||||||||||||
Bug Blocks: | 101121, 101279 | ||||||||||||||||
Attachments: |
|
Description
Adam Barth
2012-11-02 15:07:46 PDT
Created attachment 172153 [details]
Patch
Comment on attachment 172153 [details]
Patch
LGTM.
*** Bug 101121 has been marked as a duplicate of this bug. *** Created attachment 172376 [details]
Patch
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.
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. 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. 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. I've made all other changes locally. Created attachment 172396 [details]
Patch for landing
Comment on attachment 172396 [details] Patch for landing Clearing flags on attachment: 172396 Committed r133526: <http://trac.webkit.org/changeset/133526> All reviewed patches have been landed. Closing bug. 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? How can we repro that regression locally? This set of changes should be causing memory progressions, but it's possible something was missed here... Re-opened since this is blocked by bug 101388 > 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.
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 I bet it is the V8 roll: http://src.chromium.org/viewvc/chrome?view=rev&revision=166220 The V8 roll is being reverted. 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. 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. As far as I can tell, this is a non-problem. Maybe I'm misunderstanding the significance of this chart? 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. 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 lol whoops. nevermind. i misread it and thought that patch had been processed in the previous run. (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. 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. Created attachment 172855 [details]
Patch
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). (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). Created attachment 172876 [details]
Patch
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. Committed r133810: <http://trac.webkit.org/changeset/133810> Re-opened since this is blocked by bug 101520 Created attachment 172902 [details]
Hopefully final patch
Comment on attachment 172902 [details] Hopefully final patch Clearing flags on attachment: 172902 Committed r133837: <http://trac.webkit.org/changeset/133837> All reviewed patches have been landed. Closing bug. |