Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by haraken Modified:
4 years, 10 months ago CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Don't reset heap counters in a snapshot GC
The snapshot GC is expected to have no side effect.
It shouldn't reset the heap counters before starting a GC.
BUG=490087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197483
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #Messages
Total messages: 22 (7 generated)
haraken@chromium.org changed reviewers: + keishi@chromium.org, oilpan-reviews@chromium.org, primiano@chromium.org, ssid@chromium.org
ssid@: I'm sorry about the delay... Would you check if this CL will solve the problem that marked object size becomes 0 after taking a snapshot etc?
https://codereview.chromium.org/1188993004/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1188993004/diff/1/Source/platform/heap/Heap.c... Source/platform/heap/Heap.cpp:2022: if (gcType == ThreadState::TakeSnapshot) Do you mean != ? With !=, The allocatedObjectSize seems to be showing sensible values. The total live_size calculated from each heap page (both normal and large) is less than Heap::markedObjectSize. Is this expected or I am calculating it wrong? The markedObjectSize keeps increasing with each trace GC! I think this needs to be reset. I am changing the total_allocated_size displayed in tracing to be allocatedObjectSize + markedObjectSize. Makes sense? Also estimatedLiveObjectSize > allocatedSpace. I think I am missing some bit mask ? Also, the GC with snapshot is failing when the page has large object allocations, filed a bug (crbug.com/501771)
https://codereview.chromium.org/1188993004/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1188993004/diff/1/Source/platform/heap/Heap.c... Source/platform/heap/Heap.cpp:2022: if (gcType == ThreadState::TakeSnapshot) On 2015/06/18 12:01:07, ssid wrote: > Do you mean != ? > With !=, Nice catch! > The allocatedObjectSize seems to be showing sensible values. > The total live_size calculated from each heap page (both normal and large) is > less than Heap::markedObjectSize. Is this expected or I am calculating it wrong? How are you calculating the "total live_size"? > The markedObjectSize keeps increasing with each trace GC! I think this needs to > be reset. I think this is expected. Note that the markedObjectSize is counted up in the lazy sweeping phase. It is not counted up in the marking phase. So, as the lazy sweeping sweeps more objects, the markedObjectSize increases. (The reason we don't count up the markedObjectSize in the marking phase is that we want to reduce the pause time of the marking phase as much as possible. However, we should reconsider this -- we should investigate if it really affects the pause time.) > I am changing the total_allocated_size displayed in tracing to be > allocatedObjectSize + markedObjectSize. Makes sense? Makes sense. > Also estimatedLiveObjectSize > allocatedSpace. I think I am missing some bit > mask ? This is OK, because estimatedLiveObjectSize takes care of the size of objects allocated in PartitionAlloc. ...Yeah, this is nasty. Now yutak is trying to make the estimatedLiveObjectSize saner, so let's just wait for his awesome work :) I'm sorry it's causing your headache. Just to clarify: allocatedSpace: The total amount of the memory Oilpan has allocated. allocatedObjectSize: The size of objects Oilpan has allocated since the last GC. markedObjectSize: The size of marked objects in the last GC. This value is finalized when the lazy sweeping completes. estimatedLiveObjectSize: The estimated live object size in both Oilpan and PartitionAlloc. Currently the estimation looks broken, and yutak is looking into the issue. > Also, the GC with snapshot is failing when the page has large object > allocations, filed a bug (crbug.com/501771) Sorry again. I'll fix it.
> > The allocatedObjectSize seems to be showing sensible values. > > The total live_size calculated from each heap page (both normal and large) is > > less than Heap::markedObjectSize. Is this expected or I am calculating it > wrong? > > How are you calculating the "total live_size"? > In each heap, in each object, if the object isMarked(), then the is counted as live. The way it is currently calculated in snapshot() functions. > > The markedObjectSize keeps increasing with each trace GC! I think this needs > to > > be reset. > > I think this is expected. Note that the markedObjectSize is counted up in the > lazy sweeping phase. It is not counted up in the marking phase. So, as the lazy > sweeping sweeps more objects, the markedObjectSize increases. > > (The reason we don't count up the markedObjectSize in the marking phase is that > we want to reduce the pause time of the marking phase as much as possible. > However, we should reconsider this -- we should investigate if it really affects > the pause time.) > I wasn't clear. But the markedObjectSize keep multiplying every time the dump happens. For example, at the first dump, marked size is 2Mb and allocatedSpace is 3Mb, the second dump marked size goes to 3.5, ...in 10th dump it goes up to 20mb or more. The allocatedSpace still remains 3Mb.
On 2015/06/18 18:59:38, ssid wrote: > > > The allocatedObjectSize seems to be showing sensible values. > > > The total live_size calculated from each heap page (both normal and large) > is > > > less than Heap::markedObjectSize. Is this expected or I am calculating it > > wrong? > > > > How are you calculating the "total live_size"? > > > > In each heap, in each object, if the object isMarked(), then the is counted as > live. The way it is currently calculated in snapshot() functions. > > > > The markedObjectSize keeps increasing with each trace GC! I think this needs > > to > > > be reset. > > > > I think this is expected. Note that the markedObjectSize is counted up in the > > lazy sweeping phase. It is not counted up in the marking phase. So, as the > lazy > > sweeping sweeps more objects, the markedObjectSize increases. > > > > (The reason we don't count up the markedObjectSize in the marking phase is > that > > we want to reduce the pause time of the marking phase as much as possible. > > However, we should reconsider this -- we should investigate if it really > affects > > the pause time.) > > > > I wasn't clear. But the markedObjectSize keep multiplying every time the dump > happens. For example, at the first dump, marked size is 2Mb and allocatedSpace > is 3Mb, the second dump marked size goes to 3.5, ...in 10th dump it goes up to > 20mb or more. The allocatedSpace still remains 3Mb. Thanks, I think I fixed the issue in the latest patch set :)
keishi-san: PTAL if ssid is happy with this.
On 2015/06/18 19:04:33, haraken wrote: > keishi-san: PTAL if ssid is happy with this. Great, thanks LGTM
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1188993004/20001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Stampty stamp, LGMT
Damn, let's try again with no typos. Stampty stamp, LGTM
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1188993004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59773)
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/1188993004/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1188993004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197483 |
