Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13975005:
Update V8GCController to use new V8 GC-related APIs. (Closed)
|
Created:
7 years, 8 months ago by marja Modified:
7 years, 7 months ago CC:
blink-reviews, haraken, Nate Chapin, jochen (gone - plz use gerrit), loislo, alph, yurys Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionUpdate V8GCController to use new V8 GC-related APIs.
The purpose of this is to avoid copying v8::Persistent handles around, and
eventually make them non-copyable.
BUG=NONE
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149579
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : code review, other updates (api has changed) #Patch Set 4 : live root fix #Patch Set 5 : Use the isolate-based APIs. #Patch Set 6 : api renaming + references from an object #Patch Set 7 : add minor gc too #Patch Set 8 : only ctruct retainedobjectinfos if necessary #
Total comments: 16
Patch Set 9 : Code review (haraken) #
Total comments: 6
Patch Set 10 : code review (haraken) #Patch Set 11 : rebased #
Messages
Total messages: 20 (0 generated)
haraken, fyi, here's the Blink side of the V8 GC related api changes. This requires bleeding edge V8 and in addition https://codereview.chromium.org/14175005/ , which is not yet reviewed, so things are likely to change still. Just in case you'd like to have an early peek. No need to review this before the v8 side is in.
When I tested the performance of this code previously, using HashSets was significantly slower than the Vector+sort approach. Please run PerformanceTests/Bindings/gc-*.html to check the performance of this CL. https://codereview.chromium.org/13975005/diff/2001/Source/bindings/v8/V8GCCon... File Source/bindings/v8/V8GCController.cpp (right): https://codereview.chromium.org/13975005/diff/2001/Source/bindings/v8/V8GCCon... Source/bindings/v8/V8GCController.cpp:262: void MaybeAddRetainedObjectInfo(Node* root) MaybeAddRetainedObjectInfo -> maybeAddRetainedObjectInfo https://codereview.chromium.org/13975005/diff/2001/Source/bindings/v8/V8GCCon... Source/bindings/v8/V8GCController.cpp:270: void SetObjectGroupId(const v8::Persistent<v8::Object>& object, void* root) SetObjectGroupId -> setObjectGroupId https://codereview.chromium.org/13975005/diff/2001/Source/bindings/v8/V8GCCon... Source/bindings/v8/V8GCController.cpp:282: HashSet<Node*> groups_with_retained_info_; m_groupsWithRetainedInfo https://codereview.chromium.org/13975005/diff/2001/Source/bindings/v8/V8GCCon... Source/bindings/v8/V8GCController.cpp:283: HashSet<void*> groups_with_representative_objects_; m_groupsWithRepresentiveObjects
I ran the Bindings gc-* performance tests for this, and the results are not good :( Blink patch from this review (patch set 3) V8 patch from here: https://codereview.chromium.org/14007008/ (patch sets 2 and 3) Regression: gc-forest: 41% worse gc-mini-tree: 13% worse gc-tree: - However, if V8 is made to use std::vector instead of its own List for bookeeping (patch set 3)... gc-forest: 26% worse gc-mini-tree: 9% worse gc-tree: 5 % better (I haven't been able to reproduce this efficiency difference in a smaller test - in those V8 List seems to be as good as std::vector.) One major pain with the new API is keeping track of which object groups need a RetainedObjectInfo. If we just don't create them at all (and use std::vector in V8): gc-forest: 14% worse gc-mini-tree: - gc-tree: 12 % better I don't yet know why exactly it is so much more inefficient to do the group forming on the V8 side.
More information: It turns out that the problem is sorting the list (see https://code.google.com/p/v8/issues/detail?id=2639 ). jochen@ made this CL to use std::sort instead of qsort (https://codereview.chromium.org/14315005). So these perf results are for this CL + patch set 2 (no std containers) from https://codereview.chromium.org/14007008/ + jochen@'s sort CL. Bindings: append-child: 9% better create-element 25% worse (15% worse without RetainedDOMInfo) gc-forest: 32% worse (17% worse) gc-tree: 4% better (8% better) insert-before: 11% better node-list-access: 3% worse set-attribute 32% worse undefined-get-element-by-id: 6% better Dromaeo: dom-attr 7% worse dom-modify - dom-query 10% better dom-traverse - For these, it doesn't make a difference if we construct the RetainedObjectInfos or not.
I'm a bit skeptical about the accuracy of the measurement. > Dromaeo: > > dom-attr 7% worse > dom-modify - > dom-query 10% better > dom-traverse - Why are dom-attr and dom-query affected by GC changes?
On 2013/04/18 14:39:47, haraken-google wrote: > I'm a bit skeptical about the accuracy of the measurement. > > > Dromaeo: > > > > dom-attr 7% worse > > dom-modify - > > dom-query 10% better > > dom-traverse - > > Why are dom-attr and dom-query affected by GC changes? .. or set-attribute; that doesn't make much more sense either (it only has 8 object-group connections during the GC). o.O
Update: There was some stupidity left in this patch, we do unnecessary work when there are single-object object groups. When that is fixed, the perf looks much better, e.g., gc-forest: 8% worse (without RetainedDOMInfo, 4% better). (I'll post real results later...)
Alright, I fixed some inefficiencies in the handing of single-object groups. And there's another V8 data structure quirk: repeatedly doing (fill a list with a small number of elements, read them, clear the list) is slow compared to std::vector. Ensuring the list has always some initial capacity helps there, so I added that... (This was affecting tests like Dromaeo/dom-attr where we have 8 object groups of size 1 -> NOOP.) So, for this patch + jochen@'s sort patch + the V8 side, the results are: gc-forest: 11% worse (no retained object infos: 4 % better) gc-mini-tree: 9% worse (no retained object infos: no change) gc-tree: 4% better(no retained object infos: 9 % better) And for dromaeo: dom-attr 5% worse dom-modify - dom-query 2% better dom-traverse 4% worse (Again, not affected by whether we construct the RetainedObjectInfos or not.) Conclusion: We could modify Blink so that the RetainedObjectInfos are only constructed on demand, since they're only needed for doing a heap snapshot. But for real use cases that shouldn't be affected that much (they make a difference in the pathological case when there are many detached DOM trees and we construct a RetainedObjectInfo for each; this is the case for gc-* tests). Tests like dom-attr regress, *not* because they're doing a lot of gc. They do have a small number of single-object object groups (which are a NOOP), which they still need to handle (sort, go through the objects, form the single-object object groups).
On 2013/04/19 15:00:48, marja wrote: > But > for real use cases that shouldn't be affected that much (they make a difference > in the pathological case when there are many detached DOM trees and we construct > a RetainedObjectInfo for each; this is the case for gc-* tests). Yeah, the gc-* tests are meant to be pathological. If you're making gc-tree better, then making the others a little worse is probably ok. > Tests like dom-attr regress, *not* because they're doing a lot of gc. They do > have a small number of single-object object groups (which are a NOOP), which > they still need to handle (sort, go through the objects, form the single-object > object groups). I'm not sure I understand why dom-attr is regressing. Is that something that's like to affect real-world applications, or we triggering a pathological GC behavior of some sort?
> I'm not sure I understand why dom-attr is regressing. Is that something that's > like to affect real-world applications, or we triggering a pathological GC > behavior of some sort? We've discussed offline. marja didn't observe any regression in Dromaeo in Chrome, although she observed regressions in Dromaeo in run-perf-tests. I'm not sure what's going on in run-perf-tests, but I think it's OK to land patches if there is no regression in Dromaeo in Chrome.
Agreed.
The Dromaeo/dom-* tests seem to do gc when ran with DRT, we just discussed with haraken@ why. The gc results in forming the object groups, but after that, there's nothing to actually collect. My best guess on why we regress is that even though the amount of work done is small (just form the ~ 10 1-object object groups and decide they can be ignored), we don't get it done as efficiently as we used to, and it's either some data structure related inefficiency (the V8 list is still more inefficient than std::vector) or I'm just missing something. And the hope is that in real world use cases, there is normally some garbage to collect, so the actual collection time would hopefully dominate the object group forming time. But this is a shaky claim, I haven't investigated if that really is so. If I run the tests with chrome & http://dromaeo.com/?dom-something, the GC is not happening.
haraken: ptal Changes from the previous version: - Also modify minor gc - Only construct RetainedObjectInfos if necessary - The V8 API patch got in. - This depends on 3 V8 patches which are all in bleeding_edge, but not rolled into chromium yet: -- https://codereview.chromium.org/14315005 (sort) -- https://codereview.chromium.org/14007008 (APIs) -- https://codereview.chromium.org/14471028 (RetainedObjectInfo streamlining) Perf results: gc-forest: 13 % better gc-mini-tree: 11 % better gc-tree: 18 % better
https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... File Source/bindings/v8/V8GCController.cpp (right): https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:51: static void addImplicitReferencesForNodeWithEventListeners(v8::Isolate* isolate, Node* node, const v8::Persistent<v8::Object>& wrapper) addImplicitReferencesForNodeWithEventListeners => addReferenceFromNodeToEventListeners https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:91: Vector<Node*, initialNodeVectorSize> newSpaceWrappers; newSpaceWrappers => newSpaceNodes https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:137: v8::Persistent<v8::Object> wrapper = v8::Persistent<v8::Object>((*nodeIterator)->wrapper()); Nit: It looks redundant to access (*nodeIterator)->wrapper() twice. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:208: : m_isolate(isolate), m_liveRootGroupIdSet(false), m_constructRetainedObjectInfos(constructRetainedObjectInfos) Nit: We normally write one initialization per line. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:240: m_isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(V8GCController::opaqueRootForGC(*it, m_isolate))), wrapper); Nit: Shall we write this in two lines? v8::UniqueId id(reinterpret_cast<intptr_t>(V8GCController::opaqueRootForGC(*it, m_isolate)); m_isolate->SetReferenceFromGroup(id, wrapper); BTW, why can't you simply use: m_isolate->SetReference(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper); ? https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:270: Node* already_added = 0; Nit: already_added => alreadyAdded https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:290: v8::Persistent<v8::Value> liveRoot = V8PerIsolateData::current()->ensureLiveRoot(); V8PerIsolateData::current() is heavy. You should use V8PerIsolateData::from(Isolate*). https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:296: return *liveRoot; How about returning id from this method? Currently you are doing something redundant: return a root Node from this method => pass the root Node to setObjectGroupId() => retrieve an ID from the root Node.
Discussed offline: - The "collect representative wrappers for groups and add the impl references from a representative wrapper instead of just adding them from the root" behavior is because the root might not have a wrapper -> we need the SetReferenceFromGroup API in V8, SetReference is not enough. - Why do we create an object group for live objects instead of just adding implicit references from the live root? This is how it was in V8GCController before this patch. https://bugs.webkit.org/show_bug.cgi?id=100707 maybe has something to do with it. abarth@: Why don't we (in the current version of V8GCController) create implicit references from the live root to the live objects, but create an object group fpr holding all the live objects and the live root?
Thanks for comments! https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... File Source/bindings/v8/V8GCController.cpp (right): https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:51: static void addImplicitReferencesForNodeWithEventListeners(v8::Isolate* isolate, Node* node, const v8::Persistent<v8::Object>& wrapper) On 2013/04/25 14:55:54, haraken wrote: > addImplicitReferencesForNodeWithEventListeners => > addReferenceFromNodeToEventListeners As agreed offline: addReferencesFromNodeToEventListeners. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:91: Vector<Node*, initialNodeVectorSize> newSpaceWrappers; On 2013/04/25 14:55:54, haraken wrote: > newSpaceWrappers => newSpaceNodes Done. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:137: v8::Persistent<v8::Object> wrapper = v8::Persistent<v8::Object>((*nodeIterator)->wrapper()); On 2013/04/25 14:55:54, haraken wrote: > Nit: It looks redundant to access (*nodeIterator)->wrapper() twice. Discussed offline -> not changing this. Streamliend the "wrapper = " line though. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:208: : m_isolate(isolate), m_liveRootGroupIdSet(false), m_constructRetainedObjectInfos(constructRetainedObjectInfos) On 2013/04/25 14:55:54, haraken wrote: > Nit: We normally write one initialization per line. Done. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:240: m_isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(V8GCController::opaqueRootForGC(*it, m_isolate))), wrapper); On 2013/04/25 14:55:54, haraken wrote: > Nit: Shall we write this in two lines? > > v8::UniqueId id(reinterpret_cast<intptr_t>(V8GCController::opaqueRootForGC(*it, > m_isolate)); > m_isolate->SetReferenceFromGroup(id, wrapper); > > BTW, why can't you simply use: > > m_isolate->SetReference(V8GCController::opaqueRootForGC(*it, m_isolate), > wrapper); > > ? Offline discussion -> not going to change the logic from object groups to implicit references. Did the "UniqueId on its own line" change. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:270: Node* already_added = 0; On 2013/04/25 14:55:54, haraken wrote: > Nit: already_added => alreadyAdded Done. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:290: v8::Persistent<v8::Value> liveRoot = V8PerIsolateData::current()->ensureLiveRoot(); On 2013/04/25 14:55:54, haraken wrote: > V8PerIsolateData::current() is heavy. You should use > V8PerIsolateData::from(Isolate*). Ahh, thanks for pointing that out. The previous version used V8PerIsolateData::current() even though it could've used V8PerIsolateData::from(Isolate*). *But* it did it only once, whereas this CL does it repeatedly. Fixed. https://codereview.chromium.org/13975005/diff/26003/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:296: return *liveRoot; On 2013/04/25 14:55:54, haraken wrote: > How about returning id from this method? > > Currently you are doing something redundant: return a root Node from this method > => pass the root Node to setObjectGroupId() => retrieve an ID from the root > Node. Done.
LGTM with nits. Fantastic patch! https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... File Source/bindings/v8/V8GCController.cpp (right): https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:273: std::sort(m_groupsWhichNeedRetainerInfo.begin(), m_groupsWhichNeedRetainerInfo.end()); Shall we add if(m_constructRetainedObjectInfos) ? (Though I'm not sure how important it is for performance.) https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:286: void setObjectGroupId(const v8::Persistent<v8::Object>& object, void* root) Nit: This helper method isn't much helpful. Maybe you can expand the code inline in the call site. https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:292: v8::UniqueId ensureLiveRoot() ensureLiveRoot => liveRootId ?
Thanks for review! I'll commit this when V8 has rolled into chromium. https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... File Source/bindings/v8/V8GCController.cpp (right): https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:273: std::sort(m_groupsWhichNeedRetainerInfo.begin(), m_groupsWhichNeedRetainerInfo.end()); On 2013/04/26 09:00:11, haraken wrote: > Shall we add if(m_constructRetainedObjectInfos) ? (Though I'm not sure how > important it is for performance.) Done; why not, doesn't obscure the code too much. https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:286: void setObjectGroupId(const v8::Persistent<v8::Object>& object, void* root) On 2013/04/26 09:00:11, haraken wrote: > Nit: This helper method isn't much helpful. Maybe you can expand the code inline > in the call site. Done. https://codereview.chromium.org/13975005/diff/32001/Source/bindings/v8/V8GCCo... Source/bindings/v8/V8GCController.cpp:292: v8::UniqueId ensureLiveRoot() On 2013/04/26 09:00:11, haraken wrote: > ensureLiveRoot => liveRootId ? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/13975005/38001
Message was sent while issue was closed.
Change committed as 149579 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
