Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
875503003:
Allow Oilpan heap objects account for their external allocations. (Closed)
DescriptionAllow Oilpan heap objects account for their external allocations.
The GC heuristics take into account the amount of objects allocated since
the last GC, in terms of the amount of Oilpan heap bytes allocated.
It does not consider the amount of external memory owned by an
Oilpan object. Allow objects to register such external allocations and
have the GC heuristics consider it -- if the heap isn't otherwise
considered worth GCing but it contains references to a large amount of
external memory, scheduling a GC may lessen the overall memory pressure
for the process.
Along with this, also add support for letting the outside world notify
Oilpan that a GC is now really worth considering. Intended used by other
allocators if they are running into near-OOM conditions.
R=haraken
BUG=456498
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190702
Patch Set 1 #Patch Set 2 : improvements; have external allocations trigger a conservative GC #Patch Set 3 : Have reporting of external allocations check if a GC is now reqd #Patch Set 4 : bail out early if urgent-gc has been set #Patch Set 5 : Have conservative GC take prio over scheduled GC when urgent GCing #
Total comments: 10
Patch Set 6 : Sweep after urgent GC + minor updates #Patch Set 7 : Adjust urgent GC limits + rebase #
Total comments: 20
Patch Set 8 : Inline increaseExternallyAllocatedBytes() #
Total comments: 10
Patch Set 9 : minor optimizations and tuning #
Total comments: 2
Patch Set 10 : minor tidying #
Total comments: 2
Patch Set 11 : more minor stuff #
Messages
Total messages: 56 (8 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
fyi. With Text nodes instrumented to account for the length of their externally-allocated strings (provided it is over some minimum), neither of the OOMing perf tests (textarea-dom.html and dom-modify.html) avoid OOMs. As mentioned elsewhere, textarea-dom doesn't trigger an OOM if we go back to schedule precise GCs instead of "idle ones", but dom-modify.html still OOMs even if setting the urgent-gc flag is made to trigger a GC on the next allocation. textarea-dom still OOMs if it uses idle GCs and reports Text node external allocations. In short, I don't think this extra bit of accounting has proven its worth yet & will let it sit here for the time being.
Thanks for the patch! I think this is going a right way though. > As mentioned elsewhere, textarea-dom doesn't trigger an OOM if we go back to > schedule precise GCs instead of "idle ones", but dom-modify.html still OOMs even > if setting the urgent-gc flag is made to trigger a GC on the next allocation. > textarea-dom still OOMs if it uses idle GCs and reports Text node external > allocations. Do you know why dom-modify.html OOMs even with the urgent-GC request? The next allocation that falls back to outOfLineAllocate (and thus calls shouldForceConservativeGC) doesn't happen?
On 2015/02/09 13:59:34, haraken wrote: > Thanks for the patch! I think this is going a right way though. > > > As mentioned elsewhere, textarea-dom doesn't trigger an OOM if we go back to > > schedule precise GCs instead of "idle ones", but dom-modify.html still OOMs > even > > if setting the urgent-gc flag is made to trigger a GC on the next allocation. > > textarea-dom still OOMs if it uses idle GCs and reports Text node external > > allocations. > > Do you know why dom-modify.html OOMs even with the urgent-GC request? The next > allocation that falls back to outOfLineAllocate (and thus calls > shouldForceConservativeGC) doesn't happen? If I tune down the policy to use ~10M threshold for external allocations, I can make dom-modify.html pass without OOMing. But at what cost perf-wise, I do not know.
On 2015/02/09 15:52:49, sof wrote: > On 2015/02/09 13:59:34, haraken wrote: > > Thanks for the patch! I think this is going a right way though. > > > > > As mentioned elsewhere, textarea-dom doesn't trigger an OOM if we go back to > > > schedule precise GCs instead of "idle ones", but dom-modify.html still OOMs > > even > > > if setting the urgent-gc flag is made to trigger a GC on the next > allocation. > > > textarea-dom still OOMs if it uses idle GCs and reports Text node external > > > allocations. > > > > Do you know why dom-modify.html OOMs even with the urgent-GC request? The next > > allocation that falls back to outOfLineAllocate (and thus calls > > shouldForceConservativeGC) doesn't happen? > > If I tune down the policy to use ~10M threshold for external allocations, I can > make dom-modify.html pass without OOMing. But at what cost perf-wise, I do not > know. Changed the logic a bit, so if an urgent GC is requested and only a idle/precise GC is scheduled, a conservative GC takes priority and handles the GC right away. With that in place, both of these failing tests run to completion (w/ the above threshold.) I've updated the CL with the complete set of changes I'm using, should anyone want to experiment separately.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/875503003/diff/80001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/core/dom/Text.cpp... Source/core/dom/Text.cpp:50: Heap::increaseExternallyAllocatedBytes(length); Would it be possible to use these memory APIs in PartitionAlloc, not in Blink code? (If it's not trivial to balance increaseExternallyAllocatedBytes and decreaseExternallyAllocatedBytes, we can start with just issuing Heap::urgentGC when PartitionAlloc is reaching a limit.) https://codereview.chromium.org/875503003/diff/80001/Source/core/dom/Text.cpp... Source/core/dom/Text.cpp:420: Heap::increaseExternallyAllocatedBytesAlive(length); It looks dangerous to call increaseExternallyAllocatedBytesAlive here since it can trigger a nested GC inside a marking phase... BTW, if we follow the AjustExternalAmountOfMemory API of V8, "externallyAllocatedBytes" should keep track of the amount of memory currently used in the embedder side, not the amount of memory allocated since the last GC. https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:242: RefPtrWillBeRawPtr<Element> protectElement(m_element.get()); What is this change for? https://codereview.chromium.org/875503003/diff/80001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:722: Heap::clearUrgentGC(); We should call Heap::clearUrgentGC() in Heap::collectGarbage(), so that the flag is cleared when a precise GC runs as well. https://codereview.chromium.org/875503003/diff/80001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:751: if (!Heap::isGCUrgentlyRequested() || !isSweepingScheduled()) If an urgent GC is scheduled, I think we can just force completeSweep(). if (Heap::isGCUrgentlyRequested()) completeSweep(); if (isSweepingInProgress()) return;
haraken@chromium.org changed reviewers: + kouhei@chromium.org
kouhei-san: Do you have any thoughts on how Oilpan should interact with PartiitonAlloc?
https://codereview.chromium.org/875503003/diff/80001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/core/dom/Text.cpp... Source/core/dom/Text.cpp:50: Heap::increaseExternallyAllocatedBytes(length); On 2015/02/10 01:23:17, haraken wrote: > > Would it be possible to use these memory APIs in PartitionAlloc, not in Blink > code? (If it's not trivial to balance increaseExternallyAllocatedBytes and > decreaseExternallyAllocatedBytes, we can start with just issuing Heap::urgentGC > when PartitionAlloc is reaching a limit.) PartitionAlloc would have to know that it was an Oilpan-initiated allocation..and deallocation. Having it separately signal memory pressure by setting the urgent-gc flag would be helpful, but would require some thought/coordination. To my mind, external allocation accounting can be beneficial for "externally heavy" Oilpan objects & have those guide GC scheduling. Accounting for every allocation adds overhead and is too fine grained a measurement, i feel. https://codereview.chromium.org/875503003/diff/80001/Source/core/dom/Text.cpp... Source/core/dom/Text.cpp:420: Heap::increaseExternallyAllocatedBytesAlive(length); On 2015/02/10 01:23:17, haraken wrote: > > It looks dangerous to call increaseExternallyAllocatedBytesAlive here since it > can trigger a nested GC inside a marking phase... > > BTW, if we follow the AjustExternalAmountOfMemory API of V8, > "externallyAllocatedBytes" should keep track of the amount of memory currently > used in the embedder side, not the amount of memory allocated since the last GC. As is, re-up'ing the allocation during tracing cannot trigger a GC. https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:242: RefPtrWillBeRawPtr<Element> protectElement(m_element.get()); On 2015/02/10 01:23:17, haraken wrote: > > What is this change for? Good question - running into stray Release crashes due to m_element->document() being empty below. I haven't worked out why; it seems like that shouldn't be possible. https://codereview.chromium.org/875503003/diff/80001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:751: if (!Heap::isGCUrgentlyRequested() || !isSweepingScheduled()) On 2015/02/10 01:23:17, haraken wrote: > > If an urgent GC is scheduled, I think we can just force completeSweep(). > > if (Heap::isGCUrgentlyRequested()) > completeSweep(); > if (isSweepingInProgress()) > return; That would happen on entering "StoppingOtherThreads", but sweeping afterwards makes good sense.
Discussed this in Tokyo as well.
Another (probably simpler) idea might be as follows:
ThreadState::postGCProcessing() {
m_externalMemoryUsageAtLastGC = Partitions::currentDOMMemoryUsage();
...;
}
ThreadState::shouldForceConservativeGC() {
...;
size_t externalMemoryUsage = Partitions::currentDOMMemoryUsage();
if (externalMemoryUsage >= 32 * 1024 * 1024 && externalMemoryUsage > 4 *
m_externalMemoryUsageAtLastGC)
return true;
}
ThreadState::shouldScheduleIdleGC() {
// We might want to add a similar logic here.
}
ThreadState::shouldSchedulePreciseGC() {
// We might want to add a similar logic here.
}
What do you think?
Both sof@'s idea and haraken@'s idea sgtm, however I'd prefer to try out the haraken@'s simple approach first and see how it goes.
On 2015/02/12 01:44:52, haraken wrote:
> Discussed this in Tokyo as well.
>
> Another (probably simpler) idea might be as follows:
>
> ThreadState::postGCProcessing() {
> m_externalMemoryUsageAtLastGC = Partitions::currentDOMMemoryUsage();
> ...;
> }
>
> ThreadState::shouldForceConservativeGC() {
> ...;
> size_t externalMemoryUsage = Partitions::currentDOMMemoryUsage();
> if (externalMemoryUsage >= 32 * 1024 * 1024 && externalMemoryUsage > 4 *
> m_externalMemoryUsageAtLastGC)
> return true;
> }
>
> ThreadState::shouldScheduleIdleGC() {
> // We might want to add a similar logic here.
> }
>
> ThreadState::shouldSchedulePreciseGC() {
> // We might want to add a similar logic here.
> }
>
> What do you think?
Won't Partitions::currentDOMMemoryUsage() always return 0 with Oilpan enabled?
https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:242: RefPtrWillBeRawPtr<Element> protectElement(m_element.get()); On 2015/02/11 15:58:25, sof wrote: > On 2015/02/10 01:23:17, haraken wrote: > > > > What is this change for? > > Good question - running into stray Release crashes due to m_element->document() > being empty below. I haven't worked out why; it seems like that shouldn't be > possible. Current theory is that this is an ImageLoader element keepalive problem; the task not keeping the loader's element alive. Trying to reproduce elsewhere.
On 2015/02/12 07:28:31, sof wrote: > https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... > File Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/875503003/diff/80001/Source/core/loader/Image... > Source/core/loader/ImageLoader.cpp:242: RefPtrWillBeRawPtr<Element> > protectElement(m_element.get()); > On 2015/02/11 15:58:25, sof wrote: > > On 2015/02/10 01:23:17, haraken wrote: > > > > > > What is this change for? > > > > Good question - running into stray Release crashes due to > m_element->document() > > being empty below. I haven't worked out why; it seems like that shouldn't be > > possible. > > Current theory is that this is an ImageLoader element keepalive problem; the > task not keeping the loader's element alive. Trying to reproduce elsewhere. Located the problem; addressed by https://codereview.chromium.org/923443004/
On 2015/02/12 07:23:56, sof wrote:
> On 2015/02/12 01:44:52, haraken wrote:
> > Discussed this in Tokyo as well.
> >
> > Another (probably simpler) idea might be as follows:
> >
> > ThreadState::postGCProcessing() {
> > m_externalMemoryUsageAtLastGC = Partitions::currentDOMMemoryUsage();
> > ...;
> > }
> >
> > ThreadState::shouldForceConservativeGC() {
> > ...;
> > size_t externalMemoryUsage = Partitions::currentDOMMemoryUsage();
> > if (externalMemoryUsage >= 32 * 1024 * 1024 && externalMemoryUsage > 4 *
> > m_externalMemoryUsageAtLastGC)
> > return true;
> > }
> >
> > ThreadState::shouldScheduleIdleGC() {
> > // We might want to add a similar logic here.
> > }
> >
> > ThreadState::shouldSchedulePreciseGC() {
> > // We might want to add a similar logic here.
> > }
> >
> > What do you think?
>
> Won't Partitions::currentDOMMemoryUsage() always return 0 with Oilpan enabled?
Maybe use WTF::Partitions::getBufferPartition()->totalSizeOfCommittedPages for
String allocations?
On 2015/02/13 00:30:09, kouhei wrote:
> On 2015/02/12 07:23:56, sof wrote:
> > On 2015/02/12 01:44:52, haraken wrote:
> > > Discussed this in Tokyo as well.
> > >
> > > Another (probably simpler) idea might be as follows:
> > >
> > > ThreadState::postGCProcessing() {
> > > m_externalMemoryUsageAtLastGC = Partitions::currentDOMMemoryUsage();
> > > ...;
> > > }
> > >
> > > ThreadState::shouldForceConservativeGC() {
> > > ...;
> > > size_t externalMemoryUsage = Partitions::currentDOMMemoryUsage();
> > > if (externalMemoryUsage >= 32 * 1024 * 1024 && externalMemoryUsage > 4 *
> > > m_externalMemoryUsageAtLastGC)
> > > return true;
> > > }
> > >
> > > ThreadState::shouldScheduleIdleGC() {
> > > // We might want to add a similar logic here.
> > > }
> > >
> > > ThreadState::shouldSchedulePreciseGC() {
> > > // We might want to add a similar logic here.
> > > }
> > >
> > > What do you think?
> >
> > Won't Partitions::currentDOMMemoryUsage() always return 0 with Oilpan
enabled?
>
> Maybe use WTF::Partitions::getBufferPartition()->totalSizeOfCommittedPages for
> String allocations?
Wouldn't there be a more general way to get the amount of memory used in
PartitionAlloc?
On 2015/02/13 00:30:09, kouhei wrote:
> On 2015/02/12 07:23:56, sof wrote:
> > On 2015/02/12 01:44:52, haraken wrote:
> > > Discussed this in Tokyo as well.
> > >
> > > Another (probably simpler) idea might be as follows:
> > >
> > > ThreadState::postGCProcessing() {
> > > m_externalMemoryUsageAtLastGC = Partitions::currentDOMMemoryUsage();
> > > ...;
> > > }
> > >
> > > ThreadState::shouldForceConservativeGC() {
> > > ...;
> > > size_t externalMemoryUsage = Partitions::currentDOMMemoryUsage();
> > > if (externalMemoryUsage >= 32 * 1024 * 1024 && externalMemoryUsage > 4 *
> > > m_externalMemoryUsageAtLastGC)
> > > return true;
> > > }
> > >
> > > ThreadState::shouldScheduleIdleGC() {
> > > // We might want to add a similar logic here.
> > > }
> > >
> > > ThreadState::shouldSchedulePreciseGC() {
> > > // We might want to add a similar logic here.
> > > }
> > >
> > > What do you think?
> >
> > Won't Partitions::currentDOMMemoryUsage() always return 0 with Oilpan
enabled?
>
> Maybe use WTF::Partitions::getBufferPartition()->totalSizeOfCommittedPages for
> String allocations?
It would be a number; what's the underlying reasoning for using it in general?
On 2015/02/13 06:44:16, sof wrote:
> On 2015/02/13 00:30:09, kouhei wrote:
> > On 2015/02/12 07:23:56, sof wrote:
> > > On 2015/02/12 01:44:52, haraken wrote:
> > > > Discussed this in Tokyo as well.
> > > >
> > > > Another (probably simpler) idea might be as follows:
> > > >
> > > > ThreadState::postGCProcessing() {
> > > > m_externalMemoryUsageAtLastGC = Partitions::currentDOMMemoryUsage();
> > > > ...;
> > > > }
> > > >
> > > > ThreadState::shouldForceConservativeGC() {
> > > > ...;
> > > > size_t externalMemoryUsage = Partitions::currentDOMMemoryUsage();
> > > > if (externalMemoryUsage >= 32 * 1024 * 1024 && externalMemoryUsage > 4
*
> > > > m_externalMemoryUsageAtLastGC)
> > > > return true;
> > > > }
> > > >
> > > > ThreadState::shouldScheduleIdleGC() {
> > > > // We might want to add a similar logic here.
> > > > }
> > > >
> > > > ThreadState::shouldSchedulePreciseGC() {
> > > > // We might want to add a similar logic here.
> > > > }
> > > >
> > > > What do you think?
> > >
> > > Won't Partitions::currentDOMMemoryUsage() always return 0 with Oilpan
> enabled?
> >
> > Maybe use WTF::Partitions::getBufferPartition()->totalSizeOfCommittedPages
for
> > String allocations?
>
> It would be a number; what's the underlying reasoning for using it in general?
I guess that would be fine, as long as we use a condition like "we trigger a GC
on a XXX% increase in the number", don't we? Or will the number be likely to
increase and decrease dramatically to trigger Oilpan's GC too frequently?
Any update on this one?
On 2015/02/18 04:42:22, haraken wrote: > Any update on this one? Not really, the above suggestion to use some global counter on string allocations doesn't make much sense.
On 2015/02/18 07:38:08, sof wrote: > On 2015/02/18 04:42:22, haraken wrote: > > Any update on this one? > > Not really, the above suggestion to use some global counter on string > allocations doesn't make much sense. I'm curious why -- even if the returned number is large, I guess we can just rely on the percentage diff (and that's what Oilpan's GC and V8's GC are doing to determine when to trigger a GC). In your proposal, are you planning to add Heap::increaseExternallyAllocatedBytes to a bunch of other places that allocate Strings?
On 2015/02/18 08:12:54, haraken wrote: > On 2015/02/18 07:38:08, sof wrote: > > On 2015/02/18 04:42:22, haraken wrote: > > > Any update on this one? > > > > Not really, the above suggestion to use some global counter on string > > allocations doesn't make much sense. > > I'm curious why -- even if the returned number is large, I guess we can just > rely on the percentage diff (and that's what Oilpan's GC and V8's GC are doing > to determine when to trigger a GC). > > In your proposal, are you planning to add Heap::increaseExternallyAllocatedBytes > to a bunch of other places that allocate Strings? There is no causal connection between that number and what Strings were allocated by Oilpan objects; what if some other code in the renderer process allocated a lot of string, do you want that to trigger Oilpan GCs? The API can be used for other heavy object e.g. Canvas. To the extent I like having APIs like this, my plan was to use it in a manner similar to v8's AdjustExternal..() -- in places known to be OOM-triggering/problematic.
On 2015/02/18 08:54:18, sof wrote: > On 2015/02/18 08:12:54, haraken wrote: > > On 2015/02/18 07:38:08, sof wrote: > > > On 2015/02/18 04:42:22, haraken wrote: > > > > Any update on this one? > > > > > > Not really, the above suggestion to use some global counter on string > > > allocations doesn't make much sense. > > > > I'm curious why -- even if the returned number is large, I guess we can just > > rely on the percentage diff (and that's what Oilpan's GC and V8's GC are doing > > to determine when to trigger a GC). > > > > In your proposal, are you planning to add > Heap::increaseExternallyAllocatedBytes > > to a bunch of other places that allocate Strings? > > There is no causal connection between that number and what Strings were > allocated by Oilpan objects; what if some other code in the renderer process > allocated a lot of string, do you want that to trigger Oilpan GCs? This is a good point --- and my answer is yes. We had a similar discussion in V8 before. Previously the AdjustAmountOfMemory API was intended to be used to report the amount of memory charged on V8 (i.e., the amount of memory that can be collected by triggering V8 GC). However, given the complexity of the lifetime relationship between Blink and V8, it's not clear to distinguish what is charged on V8 and not charged on V8 (e.g., V8 wrapper ==(Persistent)==> DOM object ==(RefPtr)==> object ==(OwnPtr)==> big object <--- Should we report the memory of this big object?). Thus we changed our approach and started to report the amount of memory consumed in the embedder side, no matter if the memory is reclaimable by triggering V8 GC or not. It's a responsibility of V8 GC not to trigger too many GCs if V8 GCs are turned out to be unhelpful to decrease the memory usage. Then V8 GC should decrease the GC frequency. For example, V8GCController is reporting DOMMemoryUsage to V8 even though not all the DOM memory is associated with V8 wrappers (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). We have the same situation in Oilpan GC. It's hard to tell what objects are charged on Oilpan and what objects aren't. So, just like V8, I think it's better to just report the amount of memory in the embedder side (i.e., PartitionAlloc). It's a responsibility of Oilpan not to trigger too many GCs by observing how much external memory has been collected by recent Oilpan's GC.
On 2015/02/18 14:46:36, haraken wrote: > On 2015/02/18 08:54:18, sof wrote: > > On 2015/02/18 08:12:54, haraken wrote: > > > On 2015/02/18 07:38:08, sof wrote: > > > > On 2015/02/18 04:42:22, haraken wrote: > > > > > Any update on this one? > > > > > > > > Not really, the above suggestion to use some global counter on string > > > > allocations doesn't make much sense. > > > > > > I'm curious why -- even if the returned number is large, I guess we can just > > > rely on the percentage diff (and that's what Oilpan's GC and V8's GC are > doing > > > to determine when to trigger a GC). > > > > > > In your proposal, are you planning to add > > Heap::increaseExternallyAllocatedBytes > > > to a bunch of other places that allocate Strings? > > > > There is no causal connection between that number and what Strings were > > allocated by Oilpan objects; what if some other code in the renderer process > > allocated a lot of string, do you want that to trigger Oilpan GCs? > > This is a good point --- and my answer is yes. > > We had a similar discussion in V8 before. Previously the AdjustAmountOfMemory > API was intended to be used to report the amount of memory charged on V8 (i.e., > the amount of memory that can be collected by triggering V8 GC). However, given > the complexity of the lifetime relationship between Blink and V8, it's not clear > to distinguish what is charged on V8 and not charged on V8 (e.g., V8 wrapper > ==(Persistent)==> DOM object ==(RefPtr)==> object ==(OwnPtr)==> big object <--- > Should we report the memory of this big object?). > > Thus we changed our approach and started to report the amount of memory consumed > in the embedder side, no matter if the memory is reclaimable by triggering V8 GC > or not. It's a responsibility of V8 GC not to trigger too many GCs if V8 GCs are > turned out to be unhelpful to decrease the memory usage. Then V8 GC should > decrease the GC frequency. For example, V8GCController is reporting > DOMMemoryUsage to V8 even though not all the DOM memory is associated with V8 > wrappers > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > We have the same situation in Oilpan GC. It's hard to tell what objects are > charged on Oilpan and what objects aren't. So, just like V8, I think it's better > to just report the amount of memory in the embedder side (i.e., PartitionAlloc). > It's a responsibility of Oilpan not to trigger too many GCs by observing how > much external memory has been collected by recent Oilpan's GC. BTW, there is another difference. Whereas the AdjustAmountOfMemory API reports a diff of the allocated/deallocated memory, ParitionAlloc::xxx() reports an absolute value of the current memory usage. I don't have a strong opinion on which API is better but would slightly prefer the latter API, because we've caused a bunch of OOM bugs by unbalanced AdjustAmountOfMemory calls.
On 2015/02/18 15:02:38, haraken wrote: > On 2015/02/18 14:46:36, haraken wrote: > > On 2015/02/18 08:54:18, sof wrote: > > > On 2015/02/18 08:12:54, haraken wrote: > > > > On 2015/02/18 07:38:08, sof wrote: > > > > > On 2015/02/18 04:42:22, haraken wrote: > > > > > > Any update on this one? > > > > > > > > > > Not really, the above suggestion to use some global counter on string > > > > > allocations doesn't make much sense. > > > > > > > > I'm curious why -- even if the returned number is large, I guess we can > just > > > > rely on the percentage diff (and that's what Oilpan's GC and V8's GC are > > doing > > > > to determine when to trigger a GC). > > > > > > > > In your proposal, are you planning to add > > > Heap::increaseExternallyAllocatedBytes > > > > to a bunch of other places that allocate Strings? > > > > > > There is no causal connection between that number and what Strings were > > > allocated by Oilpan objects; what if some other code in the renderer process > > > allocated a lot of string, do you want that to trigger Oilpan GCs? > > > > This is a good point --- and my answer is yes. > > > > We had a similar discussion in V8 before. Previously the AdjustAmountOfMemory > > API was intended to be used to report the amount of memory charged on V8 > (i.e., > > the amount of memory that can be collected by triggering V8 GC). However, > given > > the complexity of the lifetime relationship between Blink and V8, it's not > clear > > to distinguish what is charged on V8 and not charged on V8 (e.g., V8 wrapper > > ==(Persistent)==> DOM object ==(RefPtr)==> object ==(OwnPtr)==> big object > <--- > > Should we report the memory of this big object?). > > > > Thus we changed our approach and started to report the amount of memory > consumed > > in the embedder side, no matter if the memory is reclaimable by triggering V8 > GC > > or not. It's a responsibility of V8 GC not to trigger too many GCs if V8 GCs > are > > turned out to be unhelpful to decrease the memory usage. Then V8 GC should > > decrease the GC frequency. For example, V8GCController is reporting > > DOMMemoryUsage to V8 even though not all the DOM memory is associated with V8 > > wrappers > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > > We have the same situation in Oilpan GC. It's hard to tell what objects are > > charged on Oilpan and what objects aren't. So, just like V8, I think it's > better > > to just report the amount of memory in the embedder side (i.e., > PartitionAlloc). > > It's a responsibility of Oilpan not to trigger too many GCs by observing how > > much external memory has been collected by recent Oilpan's GC. > > BTW, there is another difference. Whereas the AdjustAmountOfMemory API reports a > diff of the allocated/deallocated memory, ParitionAlloc::xxx() reports an > absolute value of the current memory usage. I don't have a strong opinion on > which API is better but would slightly prefer the latter API, because we've > caused a bunch of OOM bugs by unbalanced AdjustAmountOfMemory calls. If v8&oilpan people want to go that route, then having a global OOM notification service makes some sense -- it notifying interested parties when PartitionAlloc has reached certain ominous levels, or the derivative of such is alarming. The accounting here isn't needed then, just the ability to flag an urgent GC.
On 2015/02/18 15:47:38, sof wrote: > On 2015/02/18 15:02:38, haraken wrote: > > On 2015/02/18 14:46:36, haraken wrote: > > > On 2015/02/18 08:54:18, sof wrote: > > > > On 2015/02/18 08:12:54, haraken wrote: > > > > > On 2015/02/18 07:38:08, sof wrote: > > > > > > On 2015/02/18 04:42:22, haraken wrote: > > > > > > > Any update on this one? > > > > > > > > > > > > Not really, the above suggestion to use some global counter on string > > > > > > allocations doesn't make much sense. > > > > > > > > > > I'm curious why -- even if the returned number is large, I guess we can > > just > > > > > rely on the percentage diff (and that's what Oilpan's GC and V8's GC are > > > doing > > > > > to determine when to trigger a GC). > > > > > > > > > > In your proposal, are you planning to add > > > > Heap::increaseExternallyAllocatedBytes > > > > > to a bunch of other places that allocate Strings? > > > > > > > > There is no causal connection between that number and what Strings were > > > > allocated by Oilpan objects; what if some other code in the renderer > process > > > > allocated a lot of string, do you want that to trigger Oilpan GCs? > > > > > > This is a good point --- and my answer is yes. > > > > > > We had a similar discussion in V8 before. Previously the > AdjustAmountOfMemory > > > API was intended to be used to report the amount of memory charged on V8 > > (i.e., > > > the amount of memory that can be collected by triggering V8 GC). However, > > given > > > the complexity of the lifetime relationship between Blink and V8, it's not > > clear > > > to distinguish what is charged on V8 and not charged on V8 (e.g., V8 wrapper > > > ==(Persistent)==> DOM object ==(RefPtr)==> object ==(OwnPtr)==> big object > > <--- > > > Should we report the memory of this big object?). > > > > > > Thus we changed our approach and started to report the amount of memory > > consumed > > > in the embedder side, no matter if the memory is reclaimable by triggering > V8 > > GC > > > or not. It's a responsibility of V8 GC not to trigger too many GCs if V8 GCs > > are > > > turned out to be unhelpful to decrease the memory usage. Then V8 GC should > > > decrease the GC frequency. For example, V8GCController is reporting > > > DOMMemoryUsage to V8 even though not all the DOM memory is associated with > V8 > > > wrappers > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > > > > We have the same situation in Oilpan GC. It's hard to tell what objects are > > > charged on Oilpan and what objects aren't. So, just like V8, I think it's > > better > > > to just report the amount of memory in the embedder side (i.e., > > PartitionAlloc). > > > It's a responsibility of Oilpan not to trigger too many GCs by observing how > > > much external memory has been collected by recent Oilpan's GC. > > > > BTW, there is another difference. Whereas the AdjustAmountOfMemory API reports > a > > diff of the allocated/deallocated memory, ParitionAlloc::xxx() reports an > > absolute value of the current memory usage. I don't have a strong opinion on > > which API is better but would slightly prefer the latter API, because we've > > caused a bunch of OOM bugs by unbalanced AdjustAmountOfMemory calls. > > If v8&oilpan people want to go that route, then having a global OOM notification > service makes some sense -- it notifying interested parties when PartitionAlloc > has reached certain ominous levels, or the derivative of such is alarming. The > accounting here isn't needed then, just the ability to flag an urgent GC. It seems jochen@ is implementing the urgent GC API for V8 (according to the discussion in oilpan-reviews@). I hope such an urgent GC API for Oilpan would be enough to handle our current OOM issues at the moment. That said, the urgent GC API is just the last resort and we'll need an accounting API to coordinate memory allocators more modestly. However, we'll need more discussions to decide how the accounting API should look like :)
On 2015/02/18 15:55:17, haraken wrote: > On 2015/02/18 15:47:38, sof wrote: > > On 2015/02/18 15:02:38, haraken wrote: > > > On 2015/02/18 14:46:36, haraken wrote: > > > > On 2015/02/18 08:54:18, sof wrote: > > > > > On 2015/02/18 08:12:54, haraken wrote: > > > > > > On 2015/02/18 07:38:08, sof wrote: > > > > > > > On 2015/02/18 04:42:22, haraken wrote: > > > > > > > > Any update on this one? > > > > > > > > > > > > > > Not really, the above suggestion to use some global counter on > string > > > > > > > allocations doesn't make much sense. > > > > > > > > > > > > I'm curious why -- even if the returned number is large, I guess we > can > > > just > > > > > > rely on the percentage diff (and that's what Oilpan's GC and V8's GC > are > > > > doing > > > > > > to determine when to trigger a GC). > > > > > > > > > > > > In your proposal, are you planning to add > > > > > Heap::increaseExternallyAllocatedBytes > > > > > > to a bunch of other places that allocate Strings? > > > > > > > > > > There is no causal connection between that number and what Strings were > > > > > allocated by Oilpan objects; what if some other code in the renderer > > process > > > > > allocated a lot of string, do you want that to trigger Oilpan GCs? > > > > > > > > This is a good point --- and my answer is yes. > > > > > > > > We had a similar discussion in V8 before. Previously the > > AdjustAmountOfMemory > > > > API was intended to be used to report the amount of memory charged on V8 > > > (i.e., > > > > the amount of memory that can be collected by triggering V8 GC). However, > > > given > > > > the complexity of the lifetime relationship between Blink and V8, it's not > > > clear > > > > to distinguish what is charged on V8 and not charged on V8 (e.g., V8 > wrapper > > > > ==(Persistent)==> DOM object ==(RefPtr)==> object ==(OwnPtr)==> big object > > > <--- > > > > Should we report the memory of this big object?). > > > > > > > > Thus we changed our approach and started to report the amount of memory > > > consumed > > > > in the embedder side, no matter if the memory is reclaimable by triggering > > V8 > > > GC > > > > or not. It's a responsibility of V8 GC not to trigger too many GCs if V8 > GCs > > > are > > > > turned out to be unhelpful to decrease the memory usage. Then V8 GC should > > > > decrease the GC frequency. For example, V8GCController is reporting > > > > DOMMemoryUsage to V8 even though not all the DOM memory is associated with > > V8 > > > > wrappers > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > > > > > > We have the same situation in Oilpan GC. It's hard to tell what objects > are > > > > charged on Oilpan and what objects aren't. So, just like V8, I think it's > > > better > > > > to just report the amount of memory in the embedder side (i.e., > > > PartitionAlloc). > > > > It's a responsibility of Oilpan not to trigger too many GCs by observing > how > > > > much external memory has been collected by recent Oilpan's GC. > > > > > > BTW, there is another difference. Whereas the AdjustAmountOfMemory API > reports > > a > > > diff of the allocated/deallocated memory, ParitionAlloc::xxx() reports an > > > absolute value of the current memory usage. I don't have a strong opinion on > > > which API is better but would slightly prefer the latter API, because we've > > > caused a bunch of OOM bugs by unbalanced AdjustAmountOfMemory calls. > > > > If v8&oilpan people want to go that route, then having a global OOM > notification > > service makes some sense -- it notifying interested parties when > PartitionAlloc > > has reached certain ominous levels, or the derivative of such is alarming. The > > accounting here isn't needed then, just the ability to flag an urgent GC. > > It seems jochen@ is implementing the urgent GC API for V8 (according to the > discussion in oilpan-reviews@). I hope such an urgent GC API for Oilpan would be > enough to handle our current OOM issues at the moment. > > That said, the urgent GC API is just the last resort and we'll need an > accounting API to coordinate memory allocators more modestly. However, we'll > need more discussions to decide how the accounting API should look like :) As agreed off-review, we need to make headway on this issue. Here's what I propose doing initially (i.e., summarizing what's in this CL right now): - Provide the ability to externally flag urgent Oilpan GCs, as intended used by some allocator-overarching component. This being the "urgent GC API". - Add the ability to have Oilpan objects register their external allocations, and re-register those when alive&being traced. That way we have a mechanism we can address specific OOM issues faced right now & short term (due to Text nodes or whatever else comes up.) This accounting mechanism relies on being able to flag urgent GCs. Getting the local Oilpan policy "right" for when it makes to perform an urgent GCs due to external allocations is something to iterate on. I won't rehash arguments on merits of doing Oilpan-specific allocation accounting or not. The accounting mechanism is low overhead in terms of codebase footprint and complexity; to my mind. (Uploaded a new patchset that adjust limits and comments a bit; it avoids the Windows 32-bit OOMs on the two perf tests we know of as being problematic.)
On 2015/02/19 13:11:19, sof wrote: > On 2015/02/18 15:55:17, haraken wrote: > > On 2015/02/18 15:47:38, sof wrote: > > > On 2015/02/18 15:02:38, haraken wrote: > > > > On 2015/02/18 14:46:36, haraken wrote: > > > > > On 2015/02/18 08:54:18, sof wrote: > > > > > > On 2015/02/18 08:12:54, haraken wrote: > > > > > > > On 2015/02/18 07:38:08, sof wrote: > > > > > > > > On 2015/02/18 04:42:22, haraken wrote: > > > > > > > > > Any update on this one? > > > > > > > > > > > > > > > > Not really, the above suggestion to use some global counter on > > string > > > > > > > > allocations doesn't make much sense. > > > > > > > > > > > > > > I'm curious why -- even if the returned number is large, I guess we > > can > > > > just > > > > > > > rely on the percentage diff (and that's what Oilpan's GC and V8's GC > > are > > > > > doing > > > > > > > to determine when to trigger a GC). > > > > > > > > > > > > > > In your proposal, are you planning to add > > > > > > Heap::increaseExternallyAllocatedBytes > > > > > > > to a bunch of other places that allocate Strings? > > > > > > > > > > > > There is no causal connection between that number and what Strings > were > > > > > > allocated by Oilpan objects; what if some other code in the renderer > > > process > > > > > > allocated a lot of string, do you want that to trigger Oilpan GCs? > > > > > > > > > > This is a good point --- and my answer is yes. > > > > > > > > > > We had a similar discussion in V8 before. Previously the > > > AdjustAmountOfMemory > > > > > API was intended to be used to report the amount of memory charged on V8 > > > > (i.e., > > > > > the amount of memory that can be collected by triggering V8 GC). > However, > > > > given > > > > > the complexity of the lifetime relationship between Blink and V8, it's > not > > > > clear > > > > > to distinguish what is charged on V8 and not charged on V8 (e.g., V8 > > wrapper > > > > > ==(Persistent)==> DOM object ==(RefPtr)==> object ==(OwnPtr)==> big > object > > > > <--- > > > > > Should we report the memory of this big object?). > > > > > > > > > > Thus we changed our approach and started to report the amount of memory > > > > consumed > > > > > in the embedder side, no matter if the memory is reclaimable by > triggering > > > V8 > > > > GC > > > > > or not. It's a responsibility of V8 GC not to trigger too many GCs if V8 > > GCs > > > > are > > > > > turned out to be unhelpful to decrease the memory usage. Then V8 GC > should > > > > > decrease the GC frequency. For example, V8GCController is reporting > > > > > DOMMemoryUsage to V8 even though not all the DOM memory is associated > with > > > V8 > > > > > wrappers > > > > > > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > > > > > > > > We have the same situation in Oilpan GC. It's hard to tell what objects > > are > > > > > charged on Oilpan and what objects aren't. So, just like V8, I think > it's > > > > better > > > > > to just report the amount of memory in the embedder side (i.e., > > > > PartitionAlloc). > > > > > It's a responsibility of Oilpan not to trigger too many GCs by observing > > how > > > > > much external memory has been collected by recent Oilpan's GC. > > > > > > > > BTW, there is another difference. Whereas the AdjustAmountOfMemory API > > reports > > > a > > > > diff of the allocated/deallocated memory, ParitionAlloc::xxx() reports an > > > > absolute value of the current memory usage. I don't have a strong opinion > on > > > > which API is better but would slightly prefer the latter API, because > we've > > > > caused a bunch of OOM bugs by unbalanced AdjustAmountOfMemory calls. > > > > > > If v8&oilpan people want to go that route, then having a global OOM > > notification > > > service makes some sense -- it notifying interested parties when > > PartitionAlloc > > > has reached certain ominous levels, or the derivative of such is alarming. > The > > > accounting here isn't needed then, just the ability to flag an urgent GC. > > > > It seems jochen@ is implementing the urgent GC API for V8 (according to the > > discussion in oilpan-reviews@). I hope such an urgent GC API for Oilpan would > be > > enough to handle our current OOM issues at the moment. > > > > That said, the urgent GC API is just the last resort and we'll need an > > accounting API to coordinate memory allocators more modestly. However, we'll > > need more discussions to decide how the accounting API should look like :) > > As agreed off-review, we need to make headway on this issue. Here's what I > propose doing initially (i.e., summarizing what's in this CL right now): > > - Provide the ability to externally flag urgent Oilpan GCs, as intended used by > some allocator-overarching component. This being the "urgent GC API". > - Add the ability to have Oilpan objects register their external allocations, > and re-register those when alive&being traced. That way we have a mechanism we > can address specific OOM issues faced right now & short term (due to Text nodes > or whatever else comes up.) This accounting mechanism relies on being able to > flag urgent GCs. > > Getting the local Oilpan policy "right" for when it makes to perform an urgent > GCs due to external allocations is something to iterate on. I won't rehash > arguments on merits of doing Oilpan-specific allocation accounting or not. The > accounting mechanism is low overhead in terms of codebase footprint and > complexity; to my mind. > > (Uploaded a new patchset that adjust limits and comments a bit; it avoids the > Windows 32-bit OOMs on the two perf tests we know of as being problematic.) Thanks for being persistent! I think the patch set 7 looks good. In particular, I like the APIs in that we don't need to "balance" the accounting values (c.f., we need to balance the accounting values in V8's AdjustAmountOfMemory API). Let me take a closer look tomorrow. Things we discussed offline: It seems that we need more discussions (with wider teams) to design good APIs to prevent OOM over all components in a renderer process (e.g., V8, Oilpan, PartitionAlloc, GPU, Skia etc). Since it will take more time to design such APIs, we decided to fix the out-standing OOMs by introducing some short-term APIs. The patch set 7 looks like a good solution in that sense.
Mostly looks good! https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:47: // If the external string kept by a Text node exceed this threshold length, exceeds https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { Does it really make sense to have the threshold? Previously we were using a similar threshold to report memory used by NodeList to V8. However, it turned out that it can end up mis-estimating the amount of memory used by NodeLists and it actually caused OOM (when a ton of NodeLists each of which is smaller than the threshold are allocated). So we eliminated the threshold from the NodeList. https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h File Source/core/dom/Text.h (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... Source/core/dom/Text.h:64: #if ENABLE(OILPAN) You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the contents of a trace method. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2833: atomicAdd(&s_externallyAllocatedBytes, static_cast<long>(delta)); You can use the return value of atomicAdd, instead of looking up externallyAllocatedBytes in 2846 again. Then you can write: size_t externalBytesSinceGC = atomicAdd(&s_externallyAllocatedBytes, static_cast<long>(delta)); if (LIKELY(externalBytesSinceGC < 100 * 1024 * 1024)) return; and thus reduce the cost of this method (I think Heap::increaseExternallyAllocatedBytes must be light-weight). You can inline this part if necessary. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2846: size_t externalBytesSinceGC = externallyAllocatedBytes(); externalBytesSinceLastGC https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.h:1010: static bool isGCUrgentlyRequested() { return acquireLoad(&s_requestedUrgentGC); } isUrgentGCRequested ? https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isGCUrgentlyRequested() || !isSweepingScheduled()) As commented earlier, I guess we should just call completeSweep() if an urgent GC is scheduled. Note that scheduleGCIfNeeded can be called by outOfLineAllocate, where it's not guaranteed that sweeping is complete. So I still think that something like: > if (Heap::isGCUrgentlyRequested()) > completeSweep(); > if (isSweepingInProgress()) > return; is what we want. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:708: if (shouldForceConservativeGC()) { Slightly better: if (Heap::isGCUrgentlyRequested()) { completeSweep(); Heap::clearUrgentGC(); Heap::collectGarbage(HeapPointersOnStack, GCWithSweep); return; } if (shouldForceConservativeGC()) { Heap::collectGarbage(ThreadState::HeapPointersOnStack, ThreadState::GCWithoutSweep); } else if (shouldSchedulePreciseGC()) ...
On 2015/02/19 23:47:44, haraken wrote: > Mostly looks good! > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp > File Source/core/dom/Text.cpp (right): > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > Source/core/dom/Text.cpp:47: // If the external string kept by a Text node > exceed this threshold length, > > exceeds > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { > > Does it really make sense to have the threshold? > > Previously we were using a similar threshold to report memory used by NodeList > to V8. However, it turned out that it can end up mis-estimating the amount of > memory used by NodeLists and it actually caused OOM (when a ton of NodeLists > each of which is smaller than the threshold are allocated). So we eliminated the > threshold from the NodeList. > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h > File Source/core/dom/Text.h (right): > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... > Source/core/dom/Text.h:64: #if ENABLE(OILPAN) > > You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the contents of > a trace method. > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:2833: atomicAdd(&s_externallyAllocatedBytes, > static_cast<long>(delta)); > > You can use the return value of atomicAdd, instead of looking up > externallyAllocatedBytes in 2846 again. > > Then you can write: > > size_t externalBytesSinceGC = atomicAdd(&s_externallyAllocatedBytes, > static_cast<long>(delta)); > if (LIKELY(externalBytesSinceGC < 100 * 1024 * 1024)) > return; > > and thus reduce the cost of this method (I think > Heap::increaseExternallyAllocatedBytes must be light-weight). You can inline > this part if necessary. > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:2846: size_t externalBytesSinceGC = > externallyAllocatedBytes(); > > externalBytesSinceLastGC > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > File Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > Source/platform/heap/Heap.h:1010: static bool isGCUrgentlyRequested() { return > acquireLoad(&s_requestedUrgentGC); } > > isUrgentGCRequested ? > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > File Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > Source/platform/heap/ThreadState.cpp:701: if (!Heap::isGCUrgentlyRequested() || > !isSweepingScheduled()) > > As commented earlier, I guess we should just call completeSweep() if an urgent > GC is scheduled. > > Note that scheduleGCIfNeeded can be called by outOfLineAllocate, where it's not > guaranteed that sweeping is complete. > > So I still think that something like: > > > if (Heap::isGCUrgentlyRequested()) > > completeSweep(); > > if (isSweepingInProgress()) > > return; > > is what we want. > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > Source/platform/heap/ThreadState.cpp:708: if (shouldForceConservativeGC()) { > > Slightly better: > > if (Heap::isGCUrgentlyRequested()) { > completeSweep(); > Heap::clearUrgentGC(); > Heap::collectGarbage(HeapPointersOnStack, GCWithSweep); > return; > } > if (shouldForceConservativeGC()) { > Heap::collectGarbage(ThreadState::HeapPointersOnStack, > ThreadState::GCWithoutSweep); > } else if (shouldSchedulePreciseGC()) > ... BTW, I'm curious how V8 is aware of the string memory allocated by Text::create(). In other words, I'm curious why dom-modify.html is not causing OOM in non-oilpan.
https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { On 2015/02/19 23:47:43, haraken wrote: > > Does it really make sense to have the threshold? > > Previously we were using a similar threshold to report memory used by NodeList > to V8. However, it turned out that it can end up mis-estimating the amount of > memory used by NodeLists and it actually caused OOM (when a ton of NodeLists > each of which is smaller than the threshold are allocated). So we eliminated the > threshold from the NodeList. > I added it to prevent having e.g. all smaller Text nodes that the parser generates being accounted for - it seemed like needless overhead & wanting to strike a balance in that regard. So it makes sense from that POV, but it is somewhat hokey overall, hence the accompanying comment. https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h File Source/core/dom/Text.h (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... Source/core/dom/Text.h:64: #if ENABLE(OILPAN) On 2015/02/19 23:47:43, haraken wrote: > > You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the contents of > a trace method. I know :) Just didn't want to increase the vtbl of this rather common object in a non-Oilpan setting at this stage of the project. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2833: atomicAdd(&s_externallyAllocatedBytes, static_cast<long>(delta)); On 2015/02/19 23:47:43, haraken wrote: > > You can use the return value of atomicAdd, instead of looking up > externallyAllocatedBytes in 2846 again. > > Then you can write: > > size_t externalBytesSinceGC = atomicAdd(&s_externallyAllocatedBytes, > static_cast<long>(delta)); > if (LIKELY(externalBytesSinceGC < 100 * 1024 * 1024)) > return; > > and thus reduce the cost of this method (I think > Heap::increaseExternallyAllocatedBytes must be light-weight). You can inline > this part if necessary. Inlined and tidied. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.h:1010: static bool isGCUrgentlyRequested() { return acquireLoad(&s_requestedUrgentGC); } On 2015/02/19 23:47:44, haraken wrote: > > isUrgentGCRequested ? It would permutedly express the exact same, but why not. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isGCUrgentlyRequested() || !isSweepingScheduled()) On 2015/02/19 23:47:44, haraken wrote: > > As commented earlier, I guess we should just call completeSweep() if an urgent > GC is scheduled. > > Note that scheduleGCIfNeeded can be called by outOfLineAllocate, where it's not > guaranteed that sweeping is complete. > > So I still think that something like: > > > if (Heap::isGCUrgentlyRequested()) > > completeSweep(); > > if (isSweepingInProgress()) > > return; > > is what we want. This will already happen when the collectGarbage() for the conservative GC goes ahead just below, as previously explained. https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:708: if (shouldForceConservativeGC()) { On 2015/02/19 23:47:44, haraken wrote: > > Slightly better: > > if (Heap::isGCUrgentlyRequested()) { > completeSweep(); > Heap::clearUrgentGC(); > Heap::collectGarbage(HeapPointersOnStack, GCWithSweep); > return; > } > if (shouldForceConservativeGC()) { > Heap::collectGarbage(ThreadState::HeapPointersOnStack, > ThreadState::GCWithoutSweep); > } else if (shouldSchedulePreciseGC()) > ... See above; redundant.
(Will take a final look at this today.)
Here is the final round of comments. https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { On 2015/02/20 09:33:48, sof wrote: > On 2015/02/19 23:47:43, haraken wrote: > > > > Does it really make sense to have the threshold? > > > > Previously we were using a similar threshold to report memory used by NodeList > > to V8. However, it turned out that it can end up mis-estimating the amount of > > memory used by NodeLists and it actually caused OOM (when a ton of NodeLists > > each of which is smaller than the threshold are allocated). So we eliminated > the > > threshold from the NodeList. > > > > I added it to prevent having e.g. all smaller Text nodes that the parser > generates being accounted for - it seemed like needless overhead & wanting to > strike a balance in that regard. > > So it makes sense from that POV, but it is somewhat hokey overall, hence the > accompanying comment. Mostly convinced but let me ask one more question: Is it really faster to have the if branch rather than unconditionally dispatching Heap::increaseExternallyAllocatedBytes? https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h File Source/core/dom/Text.h (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... Source/core/dom/Text.h:64: #if ENABLE(OILPAN) On 2015/02/20 09:33:49, sof wrote: > On 2015/02/19 23:47:43, haraken wrote: > > > > You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the contents > of > > a trace method. > > I know :) Just didn't want to increase the vtbl of this rather common object in > a non-Oilpan setting at this stage of the project. The Text object already has virtual methods, doesn't it? https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isGCUrgentlyRequested() || !isSweepingScheduled()) On 2015/02/20 09:33:49, sof wrote: > On 2015/02/19 23:47:44, haraken wrote: > > > > As commented earlier, I guess we should just call completeSweep() if an urgent > > GC is scheduled. > > > > Note that scheduleGCIfNeeded can be called by outOfLineAllocate, where it's > not > > guaranteed that sweeping is complete. > > > > So I still think that something like: > > > > > if (Heap::isGCUrgentlyRequested()) > > > completeSweep(); > > > if (isSweepingInProgress()) > > > return; > > > > is what we want. > > This will already happen when the collectGarbage() for the conservative GC goes > ahead just below, as previously explained. Makes sense. https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.h:1456: if (UNLIKELY(isUrgentGCRequested())) Can we move this check down to line 1469? For performance it seems better to check LIKELY(externalBytesAllocatedSinceLastGC < ...) first. https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) Do we want to have the 'isSweepingScheduled()' check? https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:711: Heap::clearUrgentGC(); Would it be better to move clearUrgentGC() into Heap::collectGarbage()? Otherwise, the following scenario can happen: 1) An urgent GC is scheduled 2) Heap::collectGarbage() runs for some reason. 3) shouldGCIfNeeded() is called. 4) An urgent GC is invoked.
https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { On 2015/02/23 08:42:27, haraken wrote: > On 2015/02/20 09:33:48, sof wrote: > > On 2015/02/19 23:47:43, haraken wrote: > > > > > > Does it really make sense to have the threshold? > > > > > > Previously we were using a similar threshold to report memory used by > NodeList > > > to V8. However, it turned out that it can end up mis-estimating the amount > of > > > memory used by NodeLists and it actually caused OOM (when a ton of NodeLists > > > each of which is smaller than the threshold are allocated). So we eliminated > > the > > > threshold from the NodeList. > > > > > > > I added it to prevent having e.g. all smaller Text nodes that the parser > > generates being accounted for - it seemed like needless overhead & wanting to > > strike a balance in that regard. > > > > So it makes sense from that POV, but it is somewhat hokey overall, hence the > > accompanying comment. > > Mostly convinced but let me ask one more question: Is it really faster to have > the if branch rather than unconditionally dispatching > Heap::increaseExternallyAllocatedBytes? The latter would entail an atomicAdd() which will require a lock-prefixed add instruction on (e.g.) x86. Which will bring about a multicore-lock and a cache flush. "Hundreds of cycles". If we want to keep it simple®ular just take that overhead per Text node allocation, then that's an option. Another option is to try to arrange so that only script-induced Text node allocations are accounted for (I haven't investigated if this is possible.)
https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { On 2015/02/23 09:13:37, sof wrote: > On 2015/02/23 08:42:27, haraken wrote: > > On 2015/02/20 09:33:48, sof wrote: > > > On 2015/02/19 23:47:43, haraken wrote: > > > > > > > > Does it really make sense to have the threshold? > > > > > > > > Previously we were using a similar threshold to report memory used by > > NodeList > > > > to V8. However, it turned out that it can end up mis-estimating the amount > > of > > > > memory used by NodeLists and it actually caused OOM (when a ton of > NodeLists > > > > each of which is smaller than the threshold are allocated). So we > eliminated > > > the > > > > threshold from the NodeList. > > > > > > > > > > I added it to prevent having e.g. all smaller Text nodes that the parser > > > generates being accounted for - it seemed like needless overhead & wanting > to > > > strike a balance in that regard. > > > > > > So it makes sense from that POV, but it is somewhat hokey overall, hence the > > > accompanying comment. > > > > Mostly convinced but let me ask one more question: Is it really faster to have > > the if branch rather than unconditionally dispatching > > Heap::increaseExternallyAllocatedBytes? > > The latter would entail an atomicAdd() which will require a lock-prefixed add > instruction on (e.g.) x86. Which will bring about a multicore-lock and a cache > flush. "Hundreds of cycles". > > If we want to keep it simple®ular just take that overhead per Text node > allocation, then that's an option. Another option is to try to arrange so that > only script-induced Text node allocations are accounted for (I haven't > investigated if this is possible.) Thanks, the current code looks reasonable to me. I'm not sure if we want to add this kind of threshold code to each file we call Heap::increaseExternallyAllocatedBytes, but we can worry about the problem when we hit the problem in the future.
On 2015/02/23 09:26:34, haraken wrote: > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp > File Source/core/dom/Text.cpp (right): > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { > On 2015/02/23 09:13:37, sof wrote: > > On 2015/02/23 08:42:27, haraken wrote: > > > On 2015/02/20 09:33:48, sof wrote: > > > > On 2015/02/19 23:47:43, haraken wrote: > > > > > > > > > > Does it really make sense to have the threshold? > > > > > > > > > > Previously we were using a similar threshold to report memory used by > > > NodeList > > > > > to V8. However, it turned out that it can end up mis-estimating the > amount > > > of > > > > > memory used by NodeLists and it actually caused OOM (when a ton of > > NodeLists > > > > > each of which is smaller than the threshold are allocated). So we > > eliminated > > > > the > > > > > threshold from the NodeList. > > > > > > > > > > > > > I added it to prevent having e.g. all smaller Text nodes that the parser > > > > generates being accounted for - it seemed like needless overhead & wanting > > to > > > > strike a balance in that regard. > > > > > > > > So it makes sense from that POV, but it is somewhat hokey overall, hence > the > > > > accompanying comment. > > > > > > Mostly convinced but let me ask one more question: Is it really faster to > have > > > the if branch rather than unconditionally dispatching > > > Heap::increaseExternallyAllocatedBytes? > > > > The latter would entail an atomicAdd() which will require a lock-prefixed add > > instruction on (e.g.) x86. Which will bring about a multicore-lock and a cache > > flush. "Hundreds of cycles". > > > > If we want to keep it simple®ular just take that overhead per Text node > > allocation, then that's an option. Another option is to try to arrange so that > > only script-induced Text node allocations are accounted for (I haven't > > investigated if this is possible.) > > Thanks, the current code looks reasonable to me. > > I'm not sure if we want to add this kind of threshold code to each file we call > Heap::increaseExternallyAllocatedBytes, but we can worry about the problem when > we hit the problem in the future. Heavy objects with considerable external allocations of their own will probably not have many instances, so I would expect these to unconditionally register their external allocations.
https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h File Source/core/dom/Text.h (right): https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... Source/core/dom/Text.h:64: #if ENABLE(OILPAN) On 2015/02/23 08:42:27, haraken wrote: > On 2015/02/20 09:33:49, sof wrote: > > On 2015/02/19 23:47:43, haraken wrote: > > > > > > You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the > contents > > of > > > a trace method. > > > > I know :) Just didn't want to increase the vtbl of this rather common object > in > > a non-Oilpan setting at this stage of the project. > > The Text object already has virtual methods, doesn't it? It does, that wasn't the concern, but if it would alter the size&layout of the Text vtable, non-Oilpan. I've now verified that it doesn't, the trace slot is merely overwritten with another unused method, hence I've dropped the use of the #if here. https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.h:1456: if (UNLIKELY(isUrgentGCRequested())) On 2015/02/23 08:42:27, haraken wrote: > > Can we move this check down to line 1469? For performance it seems better to > check LIKELY(externalBytesAllocatedSinceLastGC < ...) first. Done. https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) On 2015/02/23 08:42:27, haraken wrote: > > Do we want to have the 'isSweepingScheduled()' check? I think we do as otherwise we would allow nested GCs? https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:711: Heap::clearUrgentGC(); On 2015/02/23 08:42:27, haraken wrote: > > Would it be better to move clearUrgentGC() into Heap::collectGarbage()? > Otherwise, the following scenario can happen: > > 1) An urgent GC is scheduled > 2) Heap::collectGarbage() runs for some reason. > 3) shouldGCIfNeeded() is called. > 4) An urgent GC is invoked. That makes good sense to do for the cases where urgent GCs are externally requested. And it is actually already done via resetHeapCounters(), so the clearing here shouldn't be done (and will be redundant, typically.)
LGTM https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) On 2015/02/23 12:26:13, sof wrote: > On 2015/02/23 08:42:27, haraken wrote: > > > > Do we want to have the 'isSweepingScheduled()' check? > > I think we do as otherwise we would allow nested GCs? How can it lead to nested GCs? Given that we clear the urgent GC flag at the beginning of a GC, Heap::isUrgentGCRequested() will return false if we reach here during the GC. https://codereview.chromium.org/875503003/diff/160001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/160001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:61: void registerExternalAllocation(size_t length, RegistrationMode mode) Nit: I'd slightly prefer having two methods for this instead of adding an enum. increaseExternallyAllocatedBytesIfNeeded(length); increaseExternallyAllocatedBytesAliveIfNeeded(length);
https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) On 2015/02/23 14:59:07, haraken wrote: > On 2015/02/23 12:26:13, sof wrote: > > On 2015/02/23 08:42:27, haraken wrote: > > > > > > Do we want to have the 'isSweepingScheduled()' check? > > > > I think we do as otherwise we would allow nested GCs? > > How can it lead to nested GCs? Given that we clear the urgent GC flag at the > beginning of a GC, Heap::isUrgentGCRequested() will return false if we reach > here during the GC. If you're in the Sweeping state and you don't perform this check & the urgent-gc has been set by some external agent, you'll fall into doing a nested conservative GC below.
https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) On 2015/02/23 15:03:40, sof wrote: > On 2015/02/23 14:59:07, haraken wrote: > > On 2015/02/23 12:26:13, sof wrote: > > > On 2015/02/23 08:42:27, haraken wrote: > > > > > > > > Do we want to have the 'isSweepingScheduled()' check? > > > > > > I think we do as otherwise we would allow nested GCs? > > > > How can it lead to nested GCs? Given that we clear the urgent GC flag at the > > beginning of a GC, Heap::isUrgentGCRequested() will return false if we reach > > here during the GC. > > If you're in the Sweeping state and you don't perform this check & the urgent-gc > has been set by some external agent, you'll fall into doing a nested > conservative GC below. Thanks, makes sense. It seems more straightforward (and robust) to add some logic to Heap::collectGarbage to prevent nested GCs. (e.g., Add a check to prevent GCScope from getting nested.)
https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:701: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) On 2015/02/23 15:06:22, haraken wrote: > On 2015/02/23 15:03:40, sof wrote: > > On 2015/02/23 14:59:07, haraken wrote: > > > On 2015/02/23 12:26:13, sof wrote: > > > > On 2015/02/23 08:42:27, haraken wrote: > > > > > > > > > > Do we want to have the 'isSweepingScheduled()' check? > > > > > > > > I think we do as otherwise we would allow nested GCs? > > > > > > How can it lead to nested GCs? Given that we clear the urgent GC flag at the > > > beginning of a GC, Heap::isUrgentGCRequested() will return false if we reach > > > here during the GC. > > > > If you're in the Sweeping state and you don't perform this check & the > urgent-gc > > has been set by some external agent, you'll fall into doing a nested > > conservative GC below. > > Thanks, makes sense. > > It seems more straightforward (and robust) to add some logic to > Heap::collectGarbage to prevent nested GCs. (e.g., Add a check to prevent > GCScope from getting nested.) Having that "is-allowed" check in an already conditional scheduling operation (scheduleIfNeeded()), seems reasonable to me. i.e., like before/right now.
New patchsets have been uploaded after l-g-t-m from haraken@chromium.org
https://codereview.chromium.org/875503003/diff/160001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/875503003/diff/160001/Source/core/dom/Text.cp... Source/core/dom/Text.cpp:61: void registerExternalAllocation(size_t length, RegistrationMode mode) On 2015/02/23 14:59:08, haraken wrote: > > Nit: I'd slightly prefer having two methods for this instead of adding an enum. > > increaseExternallyAllocatedBytesIfNeeded(length); > increaseExternallyAllocatedBytesAliveIfNeeded(length); Done.
https://codereview.chromium.org/875503003/diff/180001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/180001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:705: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) Sorry to bug you a lot of times on this... I reconsidered but am not yet sure why we need the '|| !isSweepingScheduled()' check. Even if we don't have the check, Heap::collectGarbage() will make the heap consistent (by makeConsitentForSweeping) before starting the next GC, so the nested GC won't cause any issue. If we interpret Heap::isUrgentGCRequested() as a signal to indicate that "Please trigger a conservative GC right now even if the current sweeping is not yet complete", it seems more straightforward just to trigger a conservative GC without any additional check. (The reason I'm skeptical about the check is that the check means that we treat Sweeping and SweepingAndPreciseGCScheduled differently. There should be no difference between the two states in a case where an urgent GC is requested (we should just trigger an urgent GC immediately), so I think we should just remove the check.)
Confirmed in a local Windows build that the latest PS cures the perf test OOMs; let's try this. https://codereview.chromium.org/875503003/diff/180001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/875503003/diff/180001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:705: if (!Heap::isUrgentGCRequested() || !isSweepingScheduled()) On 2015/02/23 16:40:29, haraken wrote: > > Sorry to bug you a lot of times on this... I reconsidered but am not yet sure > why we need the '|| !isSweepingScheduled()' check. > > Even if we don't have the check, Heap::collectGarbage() will make the heap > consistent (by makeConsitentForSweeping) before starting the next GC, so the > nested GC won't cause any issue. > > If we interpret Heap::isUrgentGCRequested() as a signal to indicate that "Please > trigger a conservative GC right now even if the current sweeping is not yet > complete", it seems more straightforward just to trigger a conservative GC > without any additional check. > > (The reason I'm skeptical about the check is that the check means that we treat > Sweeping and SweepingAndPreciseGCScheduled differently. There should be no > difference between the two states in a case where an urgent GC is requested (we > should just trigger an urgent GC immediately), so I think we should just remove > the check.) Yes, we don't want to make a distinction between the "pure" sweeping state and the sweeping-and-scheduled-gc ones here. I'm getting these GC states mixed up, somehow. Removed the check; sorry about all these iterations (fatigue setting in, i suppose.)
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/875503003/#ps200001 (title: "more minor stuff")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/875503003/200001
The CQ bit was unchecked by sigbjornf@opera.com
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/875503003/200001
LGTM
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190702
Message was sent while issue was closed.
On 2015/02/19 23:55:38, haraken wrote: > On 2015/02/19 23:47:44, haraken wrote: > > Mostly looks good! > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp > > File Source/core/dom/Text.cpp (right): > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > > Source/core/dom/Text.cpp:47: // If the external string kept by a Text node > > exceed this threshold length, > > > > exceeds > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > > Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { > > > > Does it really make sense to have the threshold? > > > > Previously we were using a similar threshold to report memory used by NodeList > > to V8. However, it turned out that it can end up mis-estimating the amount of > > memory used by NodeLists and it actually caused OOM (when a ton of NodeLists > > each of which is smaller than the threshold are allocated). So we eliminated > the > > threshold from the NodeList. > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h > > File Source/core/dom/Text.h (right): > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... > > Source/core/dom/Text.h:64: #if ENABLE(OILPAN) > > > > You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the contents > of > > a trace method. > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > File Source/platform/heap/Heap.cpp (right): > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:2833: atomicAdd(&s_externallyAllocatedBytes, > > static_cast<long>(delta)); > > > > You can use the return value of atomicAdd, instead of looking up > > externallyAllocatedBytes in 2846 again. > > > > Then you can write: > > > > size_t externalBytesSinceGC = atomicAdd(&s_externallyAllocatedBytes, > > static_cast<long>(delta)); > > if (LIKELY(externalBytesSinceGC < 100 * 1024 * 1024)) > > return; > > > > and thus reduce the cost of this method (I think > > Heap::increaseExternallyAllocatedBytes must be light-weight). You can inline > > this part if necessary. > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:2846: size_t externalBytesSinceGC = > > externallyAllocatedBytes(); > > > > externalBytesSinceLastGC > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > File Source/platform/heap/Heap.h (right): > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > Source/platform/heap/Heap.h:1010: static bool isGCUrgentlyRequested() { return > > acquireLoad(&s_requestedUrgentGC); } > > > > isUrgentGCRequested ? > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > > File Source/platform/heap/ThreadState.cpp (right): > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > > Source/platform/heap/ThreadState.cpp:701: if (!Heap::isGCUrgentlyRequested() > || > > !isSweepingScheduled()) > > > > As commented earlier, I guess we should just call completeSweep() if an urgent > > GC is scheduled. > > > > Note that scheduleGCIfNeeded can be called by outOfLineAllocate, where it's > not > > guaranteed that sweeping is complete. > > > > So I still think that something like: > > > > > if (Heap::isGCUrgentlyRequested()) > > > completeSweep(); > > > if (isSweepingInProgress()) > > > return; > > > > is what we want. > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > > Source/platform/heap/ThreadState.cpp:708: if (shouldForceConservativeGC()) { > > > > Slightly better: > > > > if (Heap::isGCUrgentlyRequested()) { > > completeSweep(); > > Heap::clearUrgentGC(); > > Heap::collectGarbage(HeapPointersOnStack, GCWithSweep); > > return; > > } > > if (shouldForceConservativeGC()) { > > Heap::collectGarbage(ThreadState::HeapPointersOnStack, > > ThreadState::GCWithoutSweep); > > } else if (shouldSchedulePreciseGC()) > > ... > > BTW, I'm curious how V8 is aware of the string memory allocated by > Text::create(). In other words, I'm curious why dom-modify.html is not causing > OOM in non-oilpan. If you look at textarea-dom.html, it creates Text nodes in replaceChildrenWithText() that have a ref-count of 1. As the loop runs, each of these will be replaced & finalized promptly, preventing a heap build up of Text nodes (with ever greater strings attached.)
Message was sent while issue was closed.
On 2015/02/25 09:56:53, sof wrote: > On 2015/02/19 23:55:38, haraken wrote: > > On 2015/02/19 23:47:44, haraken wrote: > > > Mostly looks good! > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cpp > > > File Source/core/dom/Text.cpp (right): > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > > > Source/core/dom/Text.cpp:47: // If the external string kept by a Text node > > > exceed this threshold length, > > > > > > exceeds > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.cp... > > > Source/core/dom/Text.cpp:63: if (length > stringLengthThreshold) { > > > > > > Does it really make sense to have the threshold? > > > > > > Previously we were using a similar threshold to report memory used by > NodeList > > > to V8. However, it turned out that it can end up mis-estimating the amount > of > > > memory used by NodeLists and it actually caused OOM (when a ton of NodeLists > > > each of which is smaller than the threshold are allocated). So we eliminated > > the > > > threshold from the NodeList. > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h > > > File Source/core/dom/Text.h (right): > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/core/dom/Text.h#... > > > Source/core/dom/Text.h:64: #if ENABLE(OILPAN) > > > > > > You can remove ENABLE(OILPAN). We normally add ENABLE(OILPAN) to the > contents > > of > > > a trace method. > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > > Source/platform/heap/Heap.cpp:2833: atomicAdd(&s_externallyAllocatedBytes, > > > static_cast<long>(delta)); > > > > > > You can use the return value of atomicAdd, instead of looking up > > > externallyAllocatedBytes in 2846 again. > > > > > > Then you can write: > > > > > > size_t externalBytesSinceGC = atomicAdd(&s_externallyAllocatedBytes, > > > static_cast<long>(delta)); > > > if (LIKELY(externalBytesSinceGC < 100 * 1024 * 1024)) > > > return; > > > > > > and thus reduce the cost of this method (I think > > > Heap::increaseExternallyAllocatedBytes must be light-weight). You can inline > > > this part if necessary. > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > > Source/platform/heap/Heap.cpp:2846: size_t externalBytesSinceGC = > > > externallyAllocatedBytes(); > > > > > > externalBytesSinceLastGC > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > > File Source/platform/heap/Heap.h (right): > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/He... > > > Source/platform/heap/Heap.h:1010: static bool isGCUrgentlyRequested() { > return > > > acquireLoad(&s_requestedUrgentGC); } > > > > > > isUrgentGCRequested ? > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > > > File Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > > > Source/platform/heap/ThreadState.cpp:701: if (!Heap::isGCUrgentlyRequested() > > || > > > !isSweepingScheduled()) > > > > > > As commented earlier, I guess we should just call completeSweep() if an > urgent > > > GC is scheduled. > > > > > > Note that scheduleGCIfNeeded can be called by outOfLineAllocate, where it's > > not > > > guaranteed that sweeping is complete. > > > > > > So I still think that something like: > > > > > > > if (Heap::isGCUrgentlyRequested()) > > > > completeSweep(); > > > > if (isSweepingInProgress()) > > > > return; > > > > > > is what we want. > > > > > > > > > https://codereview.chromium.org/875503003/diff/120001/Source/platform/heap/Th... > > > Source/platform/heap/ThreadState.cpp:708: if (shouldForceConservativeGC()) { > > > > > > Slightly better: > > > > > > if (Heap::isGCUrgentlyRequested()) { > > > completeSweep(); > > > Heap::clearUrgentGC(); > > > Heap::collectGarbage(HeapPointersOnStack, GCWithSweep); > > > return; > > > } > > > if (shouldForceConservativeGC()) { > > > Heap::collectGarbage(ThreadState::HeapPointersOnStack, > > > ThreadState::GCWithoutSweep); > > > } else if (shouldSchedulePreciseGC()) > > > ... > > > > BTW, I'm curious how V8 is aware of the string memory allocated by > > Text::create(). In other words, I'm curious why dom-modify.html is not causing > > OOM in non-oilpan. > > If you look at textarea-dom.html, it creates Text nodes in > replaceChildrenWithText() that have a ref-count of 1. > > As the loop runs, each of these will be replaced & finalized promptly, > preventing a heap build up of Text nodes (with ever greater strings attached.) ah, V8 minor GCs drop the persistent handles to the Text Nodes, which promptly collects the strings... Makes sense :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
