Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 10 months ago by sof Modified:
5 years, 10 months ago Reviewers:
oilpan-reviews, haraken CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionPrevent GCs when constructing GC mixin objects.
R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190650
Patch Set 1 #Patch Set 2 : Restrict visibility #Patch Set 3 : add assert #
Total comments: 10
Patch Set 4 : Improve naming + optimize allocateOnHeapIndex() usage #
Created: 5 years, 10 months ago
Messages
Total messages: 16 (4 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look. I suggest we do this.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:1072: // vtabke after the constructors of all its subclasses have run, vtable https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:1092: ThreadState::current()->enterNoGCAllowedScope(); Are compilers smart enough to remove the two ThreadState::current() lookups (one in Heap::allocateOnHeapIndex and the other here)? https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:239: , m_noGCAllowedCount(0) Can we add ASSERT(!m_noGCAllowedCount) to somewhere to check that the enter and exit are balanced (e.g., when entering SafePointScope)? https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount)) I'm wondering how unlikely this happens. What happens if we create a ton of Nodes? for (...) document.createElement("div"); Given Node is a GCMixin, we might end up not triggering any conservative GC? https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:647: void enterNoGCAllowedScope() { m_noGCAllowedCount++; } enterGCForbiddenScope ? (for consistency with SweepForbiddenScope)
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount)) On 2015/02/23 08:24:23, haraken wrote: > > I'm wondering how unlikely this happens. What happens if we create a ton of > Nodes? > > for (...) > document.createElement("div"); > > Given Node is a GCMixin, we might end up not triggering any conservative GC? Not sure I understand the concern - Node is an EventTarget, which is not a mixin.
On 2015/02/23 08:30:20, sof wrote: > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > File Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount)) > On 2015/02/23 08:24:23, haraken wrote: > > > > I'm wondering how unlikely this happens. What happens if we create a ton of > > Nodes? > > > > for (...) > > document.createElement("div"); > > > > Given Node is a GCMixin, we might end up not triggering any conservative GC? > > Not sure I understand the concern - Node is an EventTarget, which is not a > mixin. Sorry, div was not a good example. I meant something like: for (...) document.createElement("media"); where HTMLMediaElement is a mixin. Can we have a conservative GC during the for loop?
On 2015/02/23 08:47:18, haraken wrote: > On 2015/02/23 08:30:20, sof wrote: > > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > > File Source/platform/heap/ThreadState.cpp (right): > > > > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > > Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount)) > > On 2015/02/23 08:24:23, haraken wrote: > > > > > > I'm wondering how unlikely this happens. What happens if we create a ton of > > > Nodes? > > > > > > for (...) > > > document.createElement("div"); > > > > > > Given Node is a GCMixin, we might end up not triggering any conservative GC? > > > > Not sure I understand the concern - Node is an EventTarget, which is not a > > mixin. > > Sorry, div was not a good example. I meant something like: > > for (...) > document.createElement("media"); > > where HTMLMediaElement is a mixin. Can we have a conservative GC during the for > loop? A heavy object. The allocations made by the HTMLMediaElement ctor may trigger conservative GCs; we leave the no-GC scope once the GarbageCollectedFinalized<> ctor runs.
On 2015/02/23 08:50:39, sof wrote: > On 2015/02/23 08:47:18, haraken wrote: > > On 2015/02/23 08:30:20, sof wrote: > > > > > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > > > File Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > > > Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount)) > > > On 2015/02/23 08:24:23, haraken wrote: > > > > > > > > I'm wondering how unlikely this happens. What happens if we create a ton > of > > > > Nodes? > > > > > > > > for (...) > > > > document.createElement("div"); > > > > > > > > Given Node is a GCMixin, we might end up not triggering any conservative > GC? > > > > > > Not sure I understand the concern - Node is an EventTarget, which is not a > > > mixin. > > > > Sorry, div was not a good example. I meant something like: > > > > for (...) > > document.createElement("media"); > > > > where HTMLMediaElement is a mixin. Can we have a conservative GC during the > for > > loop? > > A heavy object. > > The allocations made by the HTMLMediaElement ctor may trigger conservative GCs; > we leave the no-GC scope once the GarbageCollectedFinalized<> ctor runs. OK, LGTM. My concern is if we will end up suppressing conservative GCs too aggressively by this change. If that is the case, we should add a way to trigger a conservative GC in leaveGCForbiddenScope (if a conservative GC has tried to run in the GCForbiddenScope we're about to leave).
New patchsets have been uploaded after l-g-t-m from haraken@chromium.org
Thanks. If Oilpan alumni have feedback&comments on this one, I'd be most interested to hear (on or off review), as this involves issues that have received considerable attention & thought previously. https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:1072: // vtabke after the constructors of all its subclasses have run, On 2015/02/23 08:24:23, haraken wrote: > > vtable Done. https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:1092: ThreadState::current()->enterNoGCAllowedScope(); On 2015/02/23 08:24:22, haraken wrote: > > Are compilers smart enough to remove the two ThreadState::current() lookups (one > in Heap::allocateOnHeapIndex and the other here)? Reasonable to assume so, but we might as well manually CSE to remove the possibility that it won't be. Now done. I left in a templated version of allocateOnHeapIndex<T>; please check if you find that agreeable. https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:239: , m_noGCAllowedCount(0) On 2015/02/23 08:24:23, haraken wrote: > > Can we add ASSERT(!m_noGCAllowedCount) to somewhere to check that the enter and > exit are balanced (e.g., when entering SafePointScope)? It is currently asserted for on leaving; is that sufficient? https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:647: void enterNoGCAllowedScope() { m_noGCAllowedCount++; } On 2015/02/23 08:24:23, haraken wrote: > > enterGCForbiddenScope ? (for consistency with SweepForbiddenScope) Switched naming to align with that.
On 2015/02/23 08:57:27, haraken wrote: > On 2015/02/23 08:50:39, sof wrote: > > On 2015/02/23 08:47:18, haraken wrote: > > > On 2015/02/23 08:30:20, sof wrote: > > > > > > > > > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > > > > File Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr... > > > > Source/platform/heap/ThreadState.cpp:673: if > (UNLIKELY(m_noGCAllowedCount)) > > > > On 2015/02/23 08:24:23, haraken wrote: > > > > > > > > > > I'm wondering how unlikely this happens. What happens if we create a ton > > of > > > > > Nodes? > > > > > > > > > > for (...) > > > > > document.createElement("div"); > > > > > > > > > > Given Node is a GCMixin, we might end up not triggering any conservative > > GC? > > > > > > > > Not sure I understand the concern - Node is an EventTarget, which is not a > > > > mixin. > > > > > > Sorry, div was not a good example. I meant something like: > > > > > > for (...) > > > document.createElement("media"); > > > > > > where HTMLMediaElement is a mixin. Can we have a conservative GC during the > > for > > > loop? > > > > A heavy object. > > > > The allocations made by the HTMLMediaElement ctor may trigger conservative > GCs; > > we leave the no-GC scope once the GarbageCollectedFinalized<> ctor runs. > > OK, LGTM. > > My concern is if we will end up suppressing conservative GCs too aggressively by > this change. If that is the case, we should add a way to trigger a conservative > GC in leaveGCForbiddenScope (if a conservative GC has tried to run in the > GCForbiddenScope we're about to leave). I added a comment/FIXME capturing this in leaveGCForbiddenScope().
LGTM
Last Oilpan bot run appears fine also, let's try this.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/946163003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190650 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
