Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1670463002:
[Oilpan] Unify memory usage reporters of Oilpan (Closed)
|
Created:
4 years, 10 months ago by peria Modified:
4 years, 10 months ago CC:
Mads Ager (chromium), ajuma+watch-canvas_chromium.org, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jbroman, Justin Novosad, kinuko+watch, kouhei+heap_chromium.org, oilpan-reviews, pdr+graphicswatchlist_chromium.org, rwlbuis, Raymond Toy, Stephen Chennney, sof, vmpstr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnify memory usage reporters of Oilpan into ThreadState.
Currently Blink reports its memory usages to V8 in several places,
and it is very complicated to manage and its coverage is limited.
As a result, V8 can fail to trigger GCs even though Blink is consuming
a lot of memory and waiting for V8 GCs, and it leads to OOM.
This CL removes all individual reports for Oilpan, and makes
ThreadState to handle total memory usage of Oilpan on its thread.
BUG=584118
Committed: https://crrev.com/62bfa3d4cbdfdfbfd7356e15e34c2aaa0036be74
Cr-Commit-Position: refs/heads/master@{#377273}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Work for some comments #Patch Set 3 : Watch out only Oilpan heap usages #
Total comments: 4
Patch Set 4 : Remove MemoryUsageProvider #Patch Set 5 : Clean up #
Total comments: 20
Patch Set 6 : #
Total comments: 5
Patch Set 7 : Drop accessors #Patch Set 8 : Remove atomic operations #
Total comments: 8
Patch Set 9 : Fix nits #
Messages
Total messages: 50 (19 generated)
Description was changed from ========== Create MemoryUsageProvider to report additional memory amount to V8 BUG=584118 ========== to ========== Create MemoryUsageProvider class MemoryUsageProvider is expected to report memory usage of Blink (PartitionAlloc + Oilpan's heap) to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ==========
Description was changed from ========== Create MemoryUsageProvider class MemoryUsageProvider is expected to report memory usage of Blink (PartitionAlloc + Oilpan's heap) to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ========== to ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usage to V8 in some methods/functions, but the coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. MemoryUsageProvider is expected to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and to report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ==========
Description was changed from ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usage to V8 in some methods/functions, but the coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. MemoryUsageProvider is expected to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and to report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ========== to ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usage to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. MemoryUsageProvider is expected to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and to report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ==========
Description was changed from ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usage to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. MemoryUsageProvider is expected to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and to report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ========== to ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usages to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. MemoryUsageProvider is expected to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and to report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ==========
Description was changed from ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usages to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. MemoryUsageProvider is expected to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and to report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ========== to ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usages to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. This CL removes all individual reportings and creates MemoryUsageProvider to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ==========
Description was changed from ========== Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usages to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. This CL removes all individual reportings and creates MemoryUsageProvider to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and report it to V8 periodically. PS1 is an experimental implementation, and not for review. BUG=584118 ========== to ========== * experimental implementation, not for review now * Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usages to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. This CL removes all individual reportings and creates MemoryUsageProvider to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and report it to V8 periodically. BUG=584118 ==========
peria@chromium.org changed reviewers: + haraken@chromium.org
haraken@ made an experimental change.
https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapPage.cpp:705: MemoryUsageProvider::instance()->adjustAmountOfAllocatedMemory(); Currently this is the only entry point to report memory usages. It means Blink reports almost nothing about deallocations now.
haraken@chromium.org changed reviewers: + hpayer@chromium.org, jochen@chromium.org
It looks like this CL does not address all AdjustAmountOfMemory() in the code base; e.g., the ones in SerializedScriptValue.cpp, V8StringResource.cpp etc. To avoid double-reporting Blink's memory, we must remove all hand-written AdjustAmountOfMemory() when landing this CL. One big issue is that PartitionAlloc is shared among threads (and thus multiple V8 isolates). If you report (size of PartitionAlloc's heap) + (size of Oilpan's heap for thread X) to V8 isolate of the thread X, you'll end up double-reporting the size of PartitionAlloc's heap to many V8 isolates. As long as PartitionAlloc's heap is shared among threads, I cannot come up with a simple solution to the issue. One idea would be: - The main thread reports (size of PartitionAlloc's heap) + (size of Oilpan's heap of the main thread). - A worker thread reports AdjustAmountOfMemory + (size of Oilpan's heap of the worker thread). This means that we need to keep using AdjustAmountOfMemory for large objects allocated by workers, but I think the places would be pretty limited (e.g., ArrayBuffers). We'll need to keep a couple of AdjustAmountOfMemory() for workers but can remove the remaining AjustAmountOfMemory(). I'm open for ideas :) https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/StaticNodeList.h (left): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/StaticNodeList.h:77: v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(nodeList->AllocationSize()); Can we remove the AllocationSize method from StaticNodeList.h? https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/MemoryUsageProvider.cpp (right): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MemoryUsageProvider.cpp:39: int64_t oilpanHeapSize = Heap::allocatedSpace(); Heap::allocatedSpace() is the size of mmapped regions. If you want to get the size of allocated objects, you should use Heap::allocatedObjectSize() + Heap::markedObjectSize(). Also consider using ThreadState::totalMemorySize(). (Note: Before keishi@ finishes implementing the per-thread heap, ThreadState::totalMemorySize() returns the size of the entire Oilpan's heap, not the size of the thread's heap.) https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/MemoryUsageProvider.h (right): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MemoryUsageProvider.h:15: } Nit: We normally just include <v8.h>. https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MemoryUsageProvider.h:23: static MemoryUsageProvider* instance(); Instead of making this class a singleton, it might be better to make V8PerIsolateData own the MemoryUsageProvider. V8PerIsolateData : v8::Isolate = 1 : 1. https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (left): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:29: adjustExternalMemoryAllocated(info.getSafeSize(info.minRowBytes())); You cannot remove this because this explains Skia's memory. In long term, we might want to report all Skia's allocations and then remove AdjustAmountOfMemory() for Skia's memory but that's out of the scope of this CL.
On 2016/02/04 12:00:29, haraken wrote: > It looks like this CL does not address all AdjustAmountOfMemory() in the code > base; e.g., the ones in SerializedScriptValue.cpp, V8StringResource.cpp etc. To > avoid double-reporting Blink's memory, we must remove all hand-written > AdjustAmountOfMemory() when landing this CL. > > One big issue is that PartitionAlloc is shared among threads (and thus multiple > V8 isolates). If you report (size of PartitionAlloc's heap) + (size of Oilpan's > heap for thread X) to V8 isolate of the thread X, you'll end up double-reporting > the size of PartitionAlloc's heap to many V8 isolates. > > As long as PartitionAlloc's heap is shared among threads, I cannot come up with > a simple solution to the issue. One idea would be: > > - The main thread reports (size of PartitionAlloc's heap) + (size of Oilpan's > heap of the main thread). > > - A worker thread reports AdjustAmountOfMemory + (size of Oilpan's heap of the > worker thread). This means that we need to keep using AdjustAmountOfMemory for > large objects allocated by workers, but I think the places would be pretty > limited (e.g., ArrayBuffers). We'll need to keep a couple of > AdjustAmountOfMemory() for workers but can remove the remaining > AjustAmountOfMemory(). > > I'm open for ideas :) Hannes, Jochen: Do you have any idea on this?
On 2016/02/05 at 09:57:31, haraken wrote: > On 2016/02/04 12:00:29, haraken wrote: > > It looks like this CL does not address all AdjustAmountOfMemory() in the code > > base; e.g., the ones in SerializedScriptValue.cpp, V8StringResource.cpp etc. To > > avoid double-reporting Blink's memory, we must remove all hand-written > > AdjustAmountOfMemory() when landing this CL. > > > > One big issue is that PartitionAlloc is shared among threads (and thus multiple > > V8 isolates). If you report (size of PartitionAlloc's heap) + (size of Oilpan's > > heap for thread X) to V8 isolate of the thread X, you'll end up double-reporting > > the size of PartitionAlloc's heap to many V8 isolates. > > > > As long as PartitionAlloc's heap is shared among threads, I cannot come up with > > a simple solution to the issue. One idea would be: > > > > - The main thread reports (size of PartitionAlloc's heap) + (size of Oilpan's > > heap of the main thread). > > > > - A worker thread reports AdjustAmountOfMemory + (size of Oilpan's heap of the > > worker thread). This means that we need to keep using AdjustAmountOfMemory for > > large objects allocated by workers, but I think the places would be pretty > > limited (e.g., ArrayBuffers). We'll need to keep a couple of > > AdjustAmountOfMemory() for workers but can remove the remaining > > AjustAmountOfMemory(). > > > > I'm open for ideas :) > > Hannes, Jochen: Do you have any idea on this? I'm concerned that e.g. a worker that does a lot of allocation churn will have a negative impact on an well-behaved main thread which will end up doing unnecessary GCs. I'm not convinced to blanket report all memory is the right way to go, there should be at least a theoretical connect between the memory and the V8 instance we report the memory to.
e.g. in the partitionalloc case where array buffers are shared across threads, we still adjust the amount of externally owned memory only on the thread the array buffer is actually held
On 2016/02/05 15:00:35, jochen (OOO) wrote: > e.g. in the partitionalloc case where array buffers are shared across threads, > we still adjust the amount of externally owned memory only on the thread the > array buffer is actually held Yeah. The problem we want to solve is: - V8 isn't aware of memory in embedders and doesn't trigger GCs. The renderer hits OOM. Maybe a right solution to this problem is to improve the TabManager so that it can properly send moderate/critical pressure notifications to the renderer? Then V8 can just listen to the notifications and trigger GCs as necessary. (The tab-discarding team is improving the TabManager that way.) On the other hand, what do you think about the idea of reporting the entire memory of Oilpan to V8? Oilpan's heap is isolated per thread and most of the contents are DOM objects that would be indirectly related to V8. (Regarding PartitionAlloc, we can just keep AdjustAmountOfMemory.)
can you point me at a specific example of where this is the case? Usually, that happens when somebody adds a new piece of code that allocates tons of memory and doesn't tell anybody about it. I also don't see how the tab manager would help here: even if we'd trigger a GC, the website might just allocate more new stuff again. If we want to reduce the memory usage effectively, we need to communicate a dynamic limit to each renderer. I know that Opera has something like this implemented (see the talk they gave at BlinkOn3), and I still have this fancy document lying around that describes something similar: https://docs.google.com/document/d/1G5d4HfkCbiJrm74aDgeLPIwUo76adTJIPCxkAllqu...
On 2016/02/05 15:22:51, jochen (OOO) wrote: > can you point me at a specific example of where this is the case? > > Usually, that happens when somebody adds a new piece of code that allocates tons > of memory and doesn't tell anybody about it. For example, https://code.google.com/p/chromium/issues/detail?id=581674. Previously there were OOMs caused by Blink not reporting the memory used by NodeList, XHR etc. I'm a bit confused. I don't know what we should do about PartitionAlloc, but about Oilpan, Hannes and I discussed that we want to experiment with reporting Oilpan's memory to V8 and see if it's a better approach than manually adding AdjustAmountOfMemory every time a new OOM bug is filed. That's why we started this experiment. If that is not in your plan, we can stop here. > > I also don't see how the tab manager would help here: even if we'd trigger a GC, > the website might just allocate more new stuff again. If we want to reduce the > memory usage effectively, we need to communicate a dynamic limit to each > renderer. > > I know that Opera has something like this implemented (see the talk they gave at > BlinkOn3), and I still have this fancy document lying around that describes > something similar: > https://docs.google.com/document/d/1G5d4HfkCbiJrm74aDgeLPIwUo76adTJIPCxkAllqu...
Patchset #3 (id:40001) has been deleted
Discussed w/ haraken@ and we decided to report Oilpan memory usage from MemoryUsageProvider and keep other adjustAmountOfExternalAllocatedMemory() as-is. (e.g. Skia and PartitionAlloc) https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/StaticNodeList.h (left): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/StaticNodeList.h:77: v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(nodeList->AllocationSize()); On 2016/02/04 12:00:29, haraken wrote: > > Can we remove the AllocationSize method from StaticNodeList.h? Done. https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/MemoryUsageProvider.cpp (right): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MemoryUsageProvider.cpp:39: int64_t oilpanHeapSize = Heap::allocatedSpace(); On 2016/02/04 12:00:29, haraken wrote: > > Heap::allocatedSpace() is the size of mmapped regions. If you want to get the > size of allocated objects, you should use Heap::allocatedObjectSize() + > Heap::markedObjectSize(). > > Also consider using ThreadState::totalMemorySize(). (Note: Before keishi@ > finishes implementing the per-thread heap, ThreadState::totalMemorySize() > returns the size of the entire Oilpan's heap, not the size of the thread's > heap.) Done with Heap methods, as we discussed offline. https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/MemoryUsageProvider.h (right): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MemoryUsageProvider.h:15: } On 2016/02/04 12:00:29, haraken wrote: > > Nit: We normally just include <v8.h>. It is not visible from platform/heap/HeapPage.h, which includes this header file. https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MemoryUsageProvider.h:23: static MemoryUsageProvider* instance(); On 2016/02/04 12:00:29, haraken wrote: > > Instead of making this class a singleton, it might be better to make > V8PerIsolateData own the MemoryUsageProvider. > > V8PerIsolateData : v8::Isolate = 1 : 1. This method is expected to return thread specific instances. https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (left): https://codereview.chromium.org/1670463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:29: adjustExternalMemoryAllocated(info.getSafeSize(info.minRowBytes())); On 2016/02/04 12:00:29, haraken wrote: > > You cannot remove this because this explains Skia's memory. In long term, we > might want to report all Skia's allocations and then remove > AdjustAmountOfMemory() for Skia's memory but that's out of the scope of this CL. > Done.
Description was changed from ========== * experimental implementation, not for review now * Unify memory usage reporters in Blink into a new class, MemoryUsageProvider. Currently, Blink reports its memory usages to V8 in several places. It is very complicated to manage and its coverage is limited. As a result, V8 cannot trigger GCs when Blink is consuming a lot of memory and waiting for V8 GCs, leading to OOM. This CL removes all individual reportings and creates MemoryUsageProvider to know total memory usage of Blink (PartitionAlloc + Oilpan's heap) and report it to V8 periodically. BUG=584118 ========== to ========== * experimental implementation, not for review now * Unify memory usage reporters of Oilpan into a new class, MemoryUsageProvider. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports, and creates MemoryUsageProvider to handle total memory usage of Oilpan. BUG=584118 ==========
https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (left): https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:847: v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(externallyAllocatedMemory - m_externallyAllocatedMemory); This AdjustAmountOfMemory is explaining GPU's memory, so we shouldn't remove it in this CL, right? https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/MemoryUsageProvider.cpp (right): https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/MemoryUsageProvider.cpp:40: int64_t currentTotalSize = Heap::allocatedObjectSize() + Heap::markedObjectSize(); This is not what we want to report. Heap::allocatedObjectSize()+Heap::markedObjectSize() is the size of the entire Oilpan's heap. What we want to do here is to make each ThreadState report the size of Oilpan's heap of the thread. So I'd suggest: 1) Introduce ThreadState::perThreadAllocatedObjectSize(), which keeps track of the size of objects that got allocated by the thread since the last GC. 2) Introduce ThreadState::perThreadMarkedObjectSize(), which keeps track of the size of objects that got marked by the thread in the last GC. 3) Call AdjustAmountOfExternalAllocatedMemory() in ThreadState::scheduleGCIfNeeded() after the 'isSweepingInProgress()' check. The reason we want to call AdjustAmountOfExternalAllocatedMemory() after the check is that ThreadState::perThreadMarkedObjectSize() does not return a correct size until the thread finishes sweeping. Note: - If you implement 1) - 3), you won't need MemoryUsageProvider. - You can use ThreadState::m_isolate to get the isolate.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Description was changed from ========== * experimental implementation, not for review now * Unify memory usage reporters of Oilpan into a new class, MemoryUsageProvider. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports, and creates MemoryUsageProvider to handle total memory usage of Oilpan. BUG=584118 ========== to ========== Unify memory usage reporters of Oilpan into a new class, MemoryUsageProvider. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports, and creates MemoryUsageProvider to handle total memory usage of Oilpan. BUG=584118 ==========
https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (left): https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:847: v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(externallyAllocatedMemory - m_externallyAllocatedMemory); On 2016/02/08 11:17:20, haraken wrote: > > This AdjustAmountOfMemory is explaining GPU's memory, so we shouldn't remove it > in this CL, right? You're right. reverted. https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/MemoryUsageProvider.cpp (right): https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/MemoryUsageProvider.cpp:40: int64_t currentTotalSize = Heap::allocatedObjectSize() + Heap::markedObjectSize(); On 2016/02/08 11:17:20, haraken wrote: > > This is not what we want to report. > Heap::allocatedObjectSize()+Heap::markedObjectSize() is the size of the entire > Oilpan's heap. > > What we want to do here is to make each ThreadState report the size of Oilpan's > heap of the thread. So I'd suggest: > > 1) Introduce ThreadState::perThreadAllocatedObjectSize(), which keeps track of > the size of objects that got allocated by the thread since the last GC. > > 2) Introduce ThreadState::perThreadMarkedObjectSize(), which keeps track of the > size of objects that got marked by the thread in the last GC. > > 3) Call AdjustAmountOfExternalAllocatedMemory() in > ThreadState::scheduleGCIfNeeded() after the 'isSweepingInProgress()' check. The > reason we want to call AdjustAmountOfExternalAllocatedMemory() after the check > is that ThreadState::perThreadMarkedObjectSize() does not return a correct size > until the thread finishes sweeping. > > Note: > > - If you implement 1) - 3), you won't need MemoryUsageProvider. > > - You can use ThreadState::m_isolate to get the isolate. Done.
On 2016/02/09 06:39:59, peria wrote: > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (left): > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:847: > v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(externallyAllocatedMemory > - m_externallyAllocatedMemory); > On 2016/02/08 11:17:20, haraken wrote: > > > > This AdjustAmountOfMemory is explaining GPU's memory, so we shouldn't remove > it > > in this CL, right? > > You're right. reverted. > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/MemoryUsageProvider.cpp (right): > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/MemoryUsageProvider.cpp:40: int64_t > currentTotalSize = Heap::allocatedObjectSize() + Heap::markedObjectSize(); > On 2016/02/08 11:17:20, haraken wrote: > > > > This is not what we want to report. > > Heap::allocatedObjectSize()+Heap::markedObjectSize() is the size of the entire > > Oilpan's heap. > > > > What we want to do here is to make each ThreadState report the size of > Oilpan's > > heap of the thread. So I'd suggest: > > > > 1) Introduce ThreadState::perThreadAllocatedObjectSize(), which keeps track of > > the size of objects that got allocated by the thread since the last GC. > > > > 2) Introduce ThreadState::perThreadMarkedObjectSize(), which keeps track of > the > > size of objects that got marked by the thread in the last GC. > > > > 3) Call AdjustAmountOfExternalAllocatedMemory() in > > ThreadState::scheduleGCIfNeeded() after the 'isSweepingInProgress()' check. > The > > reason we want to call AdjustAmountOfExternalAllocatedMemory() after the check > > is that ThreadState::perThreadMarkedObjectSize() does not return a correct > size > > until the thread finishes sweeping. > > > > Note: > > > > - If you implement 1) - 3), you won't need MemoryUsageProvider. > > > > - You can use ThreadState::m_isolate to get the isolate. > > Done. From a V8 perspective there are two types of external memory: 1) Blink memory that is referenced by V8. This memory contributes to the whole transitive closure. Performing a V8 garbage collection may help to release that memory. We are currently reporting (more or less) this memory as external memory to V8. This type is considered in the V8 heap growing strategy to schedule garbage collections. 2) Blink memory that is not referenced by V8. A V8 garbage collection will not help to release that memory. However, if V8 holds on to a big heap and a component in Blink needs more memory, performing a V8 garbage collection (reducing V8 heap size and also releasing type 1) objects) may help to reduce memory footprint and prevent/delay OOM. Type 2) may result in garbage collections that do not free up any memory in V8/Blink. If we know that V8 and Blink memory is already collected and compacted, we do not want to force more garbage collections in V8. Overall, I think we should report these two types of external memory to V8 separately. V8 can then make a good call and decide if garbage collection may be worth it or not.
On 2016/02/09 07:56:23, Hannes Payer wrote: > On 2016/02/09 06:39:59, peria wrote: > > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (left): > > > > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:847: > > > v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(externallyAllocatedMemory > > - m_externallyAllocatedMemory); > > On 2016/02/08 11:17:20, haraken wrote: > > > > > > This AdjustAmountOfMemory is explaining GPU's memory, so we shouldn't remove > > it > > > in this CL, right? > > > > You're right. reverted. > > > > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/MemoryUsageProvider.cpp (right): > > > > > https://codereview.chromium.org/1670463002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/MemoryUsageProvider.cpp:40: int64_t > > currentTotalSize = Heap::allocatedObjectSize() + Heap::markedObjectSize(); > > On 2016/02/08 11:17:20, haraken wrote: > > > > > > This is not what we want to report. > > > Heap::allocatedObjectSize()+Heap::markedObjectSize() is the size of the > entire > > > Oilpan's heap. > > > > > > What we want to do here is to make each ThreadState report the size of > > Oilpan's > > > heap of the thread. So I'd suggest: > > > > > > 1) Introduce ThreadState::perThreadAllocatedObjectSize(), which keeps track > of > > > the size of objects that got allocated by the thread since the last GC. > > > > > > 2) Introduce ThreadState::perThreadMarkedObjectSize(), which keeps track of > > the > > > size of objects that got marked by the thread in the last GC. > > > > > > 3) Call AdjustAmountOfExternalAllocatedMemory() in > > > ThreadState::scheduleGCIfNeeded() after the 'isSweepingInProgress()' check. > > The > > > reason we want to call AdjustAmountOfExternalAllocatedMemory() after the > check > > > is that ThreadState::perThreadMarkedObjectSize() does not return a correct > > size > > > until the thread finishes sweeping. > > > > > > Note: > > > > > > - If you implement 1) - 3), you won't need MemoryUsageProvider. > > > > > > - You can use ThreadState::m_isolate to get the isolate. > > > > Done. > > From a V8 perspective there are two types of external memory: > 1) Blink memory that is referenced by V8. This memory contributes to the whole > transitive closure. Performing a V8 garbage collection may help to release that > memory. We are currently reporting (more or less) this memory as external memory > to V8. This type is considered in the V8 heap growing strategy to schedule > garbage collections. > 2) Blink memory that is not referenced by V8. A V8 garbage collection will not > help to release that memory. However, if V8 holds on to a big heap and a > component in Blink needs more memory, performing a V8 garbage collection > (reducing V8 heap size and also releasing type 1) objects) may help to reduce > memory footprint and prevent/delay OOM. > > Type 2) may result in garbage collections that do not free up any memory in > V8/Blink. If we know that V8 and Blink memory is already collected and > compacted, we do not want to force more garbage collections in V8. Overall, I > think we should report these two types of external memory to V8 separately. V8 > can then make a good call and decide if garbage collection may be worth it or > not. Thanks Hannes! Totally agreed. I think the current AdjustAmountOfMemory API is designed for type 1). So I think our goal in this CL is to report memory of type 1) in a better way (Oilpan enabled us to report the memory in a better way). The tricky part is how to determine what memory is the one referenced by V8 and what memory is not. As far as I understand: - Most of the contents in Oilpan's heap are directly or indirectly referenced by V8. In other words, most memory of Oilpan's heap is type 1). (Oilpan's GC is already triggered with this assumption.) - PartitionAlloc contains a lot of memory not referenced by V8 (e.g., static strings). In other words, PartitionAlloc contains lot of memory of type 2). Therefore my suggestion is to start reporting memory of only Oilpan's per-thread heap with the AdjustAmountOfMemory API, not including memory of PartitionAlloc. Does this make sense?
Description was changed from ========== Unify memory usage reporters of Oilpan into a new class, MemoryUsageProvider. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports, and creates MemoryUsageProvider to handle total memory usage of Oilpan. BUG=584118 ========== to ========== Unify memory usage reporters of Oilpan into ThreadState. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports for Oilpan, and makes ThreadState to handle total memory usage of Oilpan on its thread. BUG=584118 ==========
LGTM if Hannes is fine with this change. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:699: state->resetHeapSizes(); resetHeapSizes => resetHeapCounters (for consistency with Heap::resetHeapCounters) https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1160: ThreadState::current()->increaseMarkedObjectSize(markedObjectSize); heapForNormalPage()->threadState()-> https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1185: ThreadState::current()->increaseMarkedObjectSize(markedObjectSize); heapForNormalPage()->threadState()-> https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1452: ThreadState::current()->increaseMarkedObjectSize(size()); heap()->threadState()-> https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1460: ThreadState::current()->increaseMarkedObjectSize(size()); heap()->threadState()-> https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:707: adjustAmountOfMemory(); adjustAmountOfMemory => reportMemoryToV8 ? https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1267: size_t currentHeapSize = heapSize(); size_t currentHeapSize = m_allocatedObjectSize + m_markedObjectSize; https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1270: m_prevReportedSize = currentHeapSize; m_prevReportedSize => m_reportedMemoryToV8 ? https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1279: size_t ThreadState::heapSize() This method won't be needed. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:527: size_t markedObjectSize() { return acquireLoad(&m_markedObjectSize); } acquireLoad shouldn't be needed because m_allocatedObjectSize and m_markedObjectSize are accessed only by this thread. Also these getters won't be needed. You can just read m_allocatedObjectSize and m_markedObjectSize.
Yes, I think that makes sense. Partitionalloc should use another API call. Jochen, any comments?
Thank you for your comments, Hannes and Haraken. I'll wait for Jochen's review. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:699: state->resetHeapSizes(); On 2016/02/12 09:30:54, haraken wrote: > > resetHeapSizes => resetHeapCounters (for consistency with > Heap::resetHeapCounters) Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1160: ThreadState::current()->increaseMarkedObjectSize(markedObjectSize); On 2016/02/12 09:30:54, haraken wrote: > > heapForNormalPage()->threadState()-> Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1185: ThreadState::current()->increaseMarkedObjectSize(markedObjectSize); On 2016/02/12 09:30:54, haraken wrote: > > heapForNormalPage()->threadState()-> Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1452: ThreadState::current()->increaseMarkedObjectSize(size()); On 2016/02/12 09:30:54, haraken wrote: > > heap()->threadState()-> Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1460: ThreadState::current()->increaseMarkedObjectSize(size()); On 2016/02/12 09:30:54, haraken wrote: > > heap()->threadState()-> Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:707: adjustAmountOfMemory(); On 2016/02/12 09:30:54, haraken wrote: > > adjustAmountOfMemory => reportMemoryToV8 ? Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1267: size_t currentHeapSize = heapSize(); On 2016/02/12 09:30:54, haraken wrote: > > size_t currentHeapSize = m_allocatedObjectSize + m_markedObjectSize; Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1270: m_prevReportedSize = currentHeapSize; On 2016/02/12 09:30:54, haraken wrote: > > m_prevReportedSize => m_reportedMemoryToV8 ? Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1279: size_t ThreadState::heapSize() On 2016/02/12 09:30:54, haraken wrote: > > This method won't be needed. Done. https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:527: size_t markedObjectSize() { return acquireLoad(&m_markedObjectSize); } On 2016/02/12 09:30:54, haraken wrote: > > acquireLoad shouldn't be needed because m_allocatedObjectSize and > m_markedObjectSize are accessed only by this thread. > > Also these getters won't be needed. You can just read m_allocatedObjectSize and > m_markedObjectSize. Done.
https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:525: size_t heapSize(); This declaration appears to be without a definition. https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:526: size_t allocatedObjectSize() { return m_allocatedObjectSize; } These two can be const methods. And, if concurrent R/W is an issue for these, then you need to symmetrically use acquireLoad() if you're using atomicAdd() for the other half. However, I don't understand how such a race can happen -- the thread associated with the ThreadState is either parked at a safepoint or running the GC. And we don't support cross-thread heap updates (e.g., in-place expand/shrink of objects by another thread), so thread contention on these new fields doesn't appear possible.
https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:525: size_t heapSize(); On 2016/02/15 20:47:36, sof wrote: > This declaration appears to be without a definition. removed. https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:526: size_t allocatedObjectSize() { return m_allocatedObjectSize; } On 2016/02/15 20:47:36, sof wrote: > These two can be const methods. Yes and they should be, but I removed these accessors. > And, if concurrent R/W is an issue for these, then you need to symmetrically use > acquireLoad() if you're using atomicAdd() for the other half. > > However, I don't understand how such a race can happen -- the thread associated > with the ThreadState is either parked at a safepoint or running the GC. And we > don't support cross-thread heap updates (e.g., in-place expand/shrink of objects > by another thread), so thread contention on these new fields doesn't appear > possible. Agree. We don't need to care for thread race.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:526: size_t allocatedObjectSize() { return m_allocatedObjectSize; } On 2016/02/16 03:20:20, peria wrote: > On 2016/02/15 20:47:36, sof wrote: > > These two can be const methods. > Yes and they should be, but I removed these accessors. > > > > And, if concurrent R/W is an issue for these, then you need to symmetrically > use > > acquireLoad() if you're using atomicAdd() for the other half. > > > > However, I don't understand how such a race can happen -- the thread > associated > > with the ThreadState is either parked at a safepoint or running the GC. And we > > don't support cross-thread heap updates (e.g., in-place expand/shrink of > objects > > by another thread), so thread contention on these new fields doesn't appear > > possible. > Agree. We don't need to care for thread race. If so, why is the latest patchset still using atomicAdd() et al?
On 2016/02/16 06:32:56, sof wrote: > https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > https://codereview.chromium.org/1670463002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/ThreadState.h:526: size_t > allocatedObjectSize() { return m_allocatedObjectSize; } > On 2016/02/16 03:20:20, peria wrote: > > On 2016/02/15 20:47:36, sof wrote: > > > These two can be const methods. > > Yes and they should be, but I removed these accessors. > > > > > > > And, if concurrent R/W is an issue for these, then you need to symmetrically > > use > > > acquireLoad() if you're using atomicAdd() for the other half. > > > > > > However, I don't understand how such a race can happen -- the thread > > associated > > > with the ThreadState is either parked at a safepoint or running the GC. And > we > > > don't support cross-thread heap updates (e.g., in-place expand/shrink of > > objects > > > by another thread), so thread contention on these new fields doesn't appear > > > possible. > > Agree. We don't need to care for thread race. > > If so, why is the latest patchset still using atomicAdd() et al? Oops, I forgot to remove atomic operation wrappers.
https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StaticNodeList.h (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StaticNodeList.h:36: #include <v8.h> remove this? https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/CollectionItemsCache.h (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/CollectionItemsCache.h:37: #include <v8.h> removeable https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1188: heapForNormalPage()->threadState()->increaseMarkedObjectSize(markedObjectSize); (I think it would be worth it to manually CSE heapForNormalPage and bind it to a local; not for this use in particular, but uses within the loop above. Not for this CL to do, though.) https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:523: void reportMemoryToV8(); can be private (=> clearly delineating the ThreadState external API is worth it; it's already far too large.)
https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StaticNodeList.h (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StaticNodeList.h:36: #include <v8.h> On 2016/02/16 09:14:38, sof wrote: > remove this? Done. https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/CollectionItemsCache.h (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/CollectionItemsCache.h:37: #include <v8.h> On 2016/02/16 09:14:38, sof wrote: > removeable Done. https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1188: heapForNormalPage()->threadState()->increaseMarkedObjectSize(markedObjectSize); On 2016/02/16 09:14:38, sof wrote: > (I think it would be worth it to manually CSE heapForNormalPage and bind it to a > local; not for this use in particular, but uses within the loop above. Not for > this CL to do, though.) Done. https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1670463002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.h:523: void reportMemoryToV8(); On 2016/02/16 09:14:38, sof wrote: > can be private (=> clearly delineating the ThreadState external API is worth it; > it's already far too large.) Agree. Done.
lgtm, good to go.
(can somebody please explain to me what the difference between allocated and marked memory in oilpan is?) With this change, we do not report memory of type 1) and 2) separately to V8, but together, right? We still report the entire heap size to each V8, right? That's a regression over what we had before oilpan, where eg DOM memory was only reported on the main thread isolate. It's key for V8's GC to get as precise information about memory of type 1) as possible.
On 2016/02/17 10:32:10, jochen (OOO) wrote: > (can somebody please explain to me what the difference between allocated and > marked memory in oilpan is?) Allocated = The size of objects that have been allocated by the thread since the last GC. Marked = The size of objects that have survived the last GC. So Allocated + Marked is the size of the objects allocated by the thread. > > With this change, we do not report memory of type 1) and 2) separately to V8, > but together, right? > > We still report the entire heap size to each V8, right? That's a regression over > what we had before oilpan, where eg DOM memory was only reported on the main > thread isolate. It's key for V8's GC to get as precise information about memory > of type 1) as possible. This CL is trying to report the memory of type 1) as much as possible. - This CL reports the size of objects allocated by the thread. So DOM memory allocated by the main thread won't be reported to other isolates. - Most objects in Oilpan's heap are likely to be retained by JS in some senses (e.g., Strings, statically allocated objects etc aren't included in Oilpan's heap). So I think that reporting the entire size of the Oilpan's heap (of that thread) would be doing a reasonable thing. - This CL actually fixes some regression caused by Oilpan. Before Oilpan, Blink was reporting sizeof(Node objects) to V8. After Oilpan, Blink doesn't report sizeof(Node objects). After this CL, Blink starts reporting sizeof(Node objects). See https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
jochen@ PTAL
lgtm
The CQ bit was checked by peria@chromium.org
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/1670463002/#ps220001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1670463002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670463002/220001
Message was sent while issue was closed.
Description was changed from ========== Unify memory usage reporters of Oilpan into ThreadState. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports for Oilpan, and makes ThreadState to handle total memory usage of Oilpan on its thread. BUG=584118 ========== to ========== Unify memory usage reporters of Oilpan into ThreadState. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports for Oilpan, and makes ThreadState to handle total memory usage of Oilpan on its thread. BUG=584118 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Unify memory usage reporters of Oilpan into ThreadState. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports for Oilpan, and makes ThreadState to handle total memory usage of Oilpan on its thread. BUG=584118 ========== to ========== Unify memory usage reporters of Oilpan into ThreadState. Currently Blink reports its memory usages to V8 in several places, and it is very complicated to manage and its coverage is limited. As a result, V8 can fail to trigger GCs even though Blink is consuming a lot of memory and waiting for V8 GCs, and it leads to OOM. This CL removes all individual reports for Oilpan, and makes ThreadState to handle total memory usage of Oilpan on its thread. BUG=584118 Committed: https://crrev.com/62bfa3d4cbdfdfbfd7356e15e34c2aaa0036be74 Cr-Commit-Position: refs/heads/master@{#377273} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/62bfa3d4cbdfdfbfd7356e15e34c2aaa0036be74 Cr-Commit-Position: refs/heads/master@{#377273} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
