Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by haraken Modified:
4 years, 11 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: We shouldn't schedule a GC before we complete sweeping
There is no reason to schedule a GC before we complete sweeping.
If we schedule a GC before completing sweeping, it is possible that
the next GC starts running before the unmarked objects get collected
(in fact this is frequently happening in memory.top_7_stress).
This CL moves scheduleGCIfNeeded() to after completeSweep().
One concern of this change would be that we might end up with delaying
scheduling a GC, but that cannot happen because completeSweep() is guaranteed
to run before we allocate any new page.
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196390
Patch Set 1 #
Messages
Total messages: 16 (2 generated)
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org, sigbjornf@opera.com
PTAL If I'm not missing something, I think this is a correct change. I don't remember a reason I put scheduleGCIfNeeded() before completeSweep().
Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)?
On 2015/06/02 09:10:32, sof wrote: > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? Yes, it can. However, as far as I consider, I cannot come up with a scenario where we want to trigger a conservative GC before allocating a new page (if I'm not missing something).
On 2015/06/02 09:20:59, haraken wrote: > On 2015/06/02 09:10:32, sof wrote: > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > Yes, it can. However, as far as I consider, I cannot come up with a scenario > where we want to trigger a conservative GC before allocating a new page (if I'm > not missing something). But this change doesn't delay the check until after a new page allocation either? This is rather delicate, so I think you need to think that through carefully - the description doesn't account for conservative GC implications. For instance, might this delay a conservative GC too long.
On 2015/06/02 09:31:09, sof wrote: > On 2015/06/02 09:20:59, haraken wrote: > > On 2015/06/02 09:10:32, sof wrote: > > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > > > Yes, it can. However, as far as I consider, I cannot come up with a scenario > > where we want to trigger a conservative GC before allocating a new page (if > I'm > > not missing something). > > But this change doesn't delay the check until after a new page allocation > either? This is rather delicate, so I think you need to think that through > carefully - the description doesn't account for conservative GC implications. > > For instance, might this delay a conservative GC too long. Yes, this change is delicate and doing something non-trivial (but solving an actual problem we're observing in memory.top_7_stress). Regarding a conservative GC: After this CL, a conservative GC is not scheduled until we finish completeSweep. However, a conservative GC is scheduled before a new page allocation. In other words, this CL delays a conservative GC to some extent but the conservative GC is guaranteed to be scheduled before allocating a new page. Thus the key is whether there is a scenario where we should trigger a conservative GC without allocating a new page. The fact that a new page is not allocated implies that there is no peak memory increase since the latest GC (in common cases we can expect memory decrease because the lazy sweeping sweeps dead objects incrementally), which means that a conservative GC wouldn't be needed in that case. (Of course, we can invent an artificial scenario where the delay of a conservative GC becomes problematic; e.g., Oilpan allocates little but the allocated object allocates a big memory in PartitionAlloc.) Would it make sense?
On 2015/06/02 11:30:54, haraken wrote: > On 2015/06/02 09:31:09, sof wrote: > > On 2015/06/02 09:20:59, haraken wrote: > > > On 2015/06/02 09:10:32, sof wrote: > > > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > > > > > Yes, it can. However, as far as I consider, I cannot come up with a scenario > > > where we want to trigger a conservative GC before allocating a new page (if > > I'm > > > not missing something). > > > > But this change doesn't delay the check until after a new page allocation > > either? This is rather delicate, so I think you need to think that through > > carefully - the description doesn't account for conservative GC implications. > > > > For instance, might this delay a conservative GC too long. > > Yes, this change is delicate and doing something non-trivial (but solving an > actual problem we're observing in memory.top_7_stress). > > Regarding a conservative GC: > > After this CL, a conservative GC is not scheduled until we finish completeSweep. > However, a conservative GC is scheduled before a new page allocation. In other > words, this CL delays a conservative GC to some extent but the conservative GC > is guaranteed to be scheduled before allocating a new page. Thus the key is > whether there is a scenario where we should trigger a conservative GC without > allocating a new page. The fact that a new page is not allocated implies that > there is no peak memory increase since the latest GC (in common cases we can > expect memory decrease because the lazy sweeping sweeps dead objects > incrementally), which means that a conservative GC wouldn't be needed in that > case. (Of course, we can invent an artificial scenario where the delay of a > conservative GC becomes problematic; e.g., Oilpan allocates little but the > allocated object allocates a big memory in PartitionAlloc.) > > Would it make sense? Thanks, the repositioning doesn't allocate extra before the conservative GC check is made. The unknown here is if we have a fragmented heap and are under memory pressure, the conservative GC is delayed until some later out-of-line allocation. Do we know that the near-OOM tests (blink_perf.dom) aren't adversely affected?
On 2015/06/02 12:17:30, sof wrote: > On 2015/06/02 11:30:54, haraken wrote: > > On 2015/06/02 09:31:09, sof wrote: > > > On 2015/06/02 09:20:59, haraken wrote: > > > > On 2015/06/02 09:10:32, sof wrote: > > > > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > > > > > > > Yes, it can. However, as far as I consider, I cannot come up with a > scenario > > > > where we want to trigger a conservative GC before allocating a new page > (if > > > I'm > > > > not missing something). > > > > > > But this change doesn't delay the check until after a new page allocation > > > either? This is rather delicate, so I think you need to think that through > > > carefully - the description doesn't account for conservative GC > implications. > > > > > > For instance, might this delay a conservative GC too long. > > > > Yes, this change is delicate and doing something non-trivial (but solving an > > actual problem we're observing in memory.top_7_stress). > > > > Regarding a conservative GC: > > > > After this CL, a conservative GC is not scheduled until we finish > completeSweep. > > However, a conservative GC is scheduled before a new page allocation. In other > > words, this CL delays a conservative GC to some extent but the conservative GC > > is guaranteed to be scheduled before allocating a new page. Thus the key is > > whether there is a scenario where we should trigger a conservative GC without > > allocating a new page. The fact that a new page is not allocated implies that > > there is no peak memory increase since the latest GC (in common cases we can > > expect memory decrease because the lazy sweeping sweeps dead objects > > incrementally), which means that a conservative GC wouldn't be needed in that > > case. (Of course, we can invent an artificial scenario where the delay of a > > conservative GC becomes problematic; e.g., Oilpan allocates little but the > > allocated object allocates a big memory in PartitionAlloc.) > > > > Would it make sense? > > Thanks, the repositioning doesn't allocate extra before the conservative GC > check is made. > > The unknown here is if we have a fragmented heap and are under memory pressure, > the conservative GC is delayed until some later out-of-line allocation. Do we > know that the near-OOM tests (blink_perf.dom) aren't adversely affected? s/allocate extra/allocates extra Oilpan heap/
On 2015/06/02 12:17:30, sof wrote: > On 2015/06/02 11:30:54, haraken wrote: > > On 2015/06/02 09:31:09, sof wrote: > > > On 2015/06/02 09:20:59, haraken wrote: > > > > On 2015/06/02 09:10:32, sof wrote: > > > > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > > > > > > > Yes, it can. However, as far as I consider, I cannot come up with a > scenario > > > > where we want to trigger a conservative GC before allocating a new page > (if > > > I'm > > > > not missing something). > > > > > > But this change doesn't delay the check until after a new page allocation > > > either? This is rather delicate, so I think you need to think that through > > > carefully - the description doesn't account for conservative GC > implications. > > > > > > For instance, might this delay a conservative GC too long. > > > > Yes, this change is delicate and doing something non-trivial (but solving an > > actual problem we're observing in memory.top_7_stress). > > > > Regarding a conservative GC: > > > > After this CL, a conservative GC is not scheduled until we finish > completeSweep. > > However, a conservative GC is scheduled before a new page allocation. In other > > words, this CL delays a conservative GC to some extent but the conservative GC > > is guaranteed to be scheduled before allocating a new page. Thus the key is > > whether there is a scenario where we should trigger a conservative GC without > > allocating a new page. The fact that a new page is not allocated implies that > > there is no peak memory increase since the latest GC (in common cases we can > > expect memory decrease because the lazy sweeping sweeps dead objects > > incrementally), which means that a conservative GC wouldn't be needed in that > > case. (Of course, we can invent an artificial scenario where the delay of a > > conservative GC becomes problematic; e.g., Oilpan allocates little but the > > allocated object allocates a big memory in PartitionAlloc.) > > > > Would it make sense? > > Thanks, the repositioning doesn't allocate extra before the conservative GC > check is made. > > The unknown here is if we have a fragmented heap and are under memory pressure, > the conservative GC is delayed until some later out-of-line allocation. Do we > know that the near-OOM tests (blink_perf.dom) aren't adversely affected? I'll check it tomorrow. However, in theory it will rarely happen since a conservative GC is anyway not invoked until we hit 400% increase in the memory usage.
On 2015/06/02 13:34:45, haraken wrote: > On 2015/06/02 12:17:30, sof wrote: > > On 2015/06/02 11:30:54, haraken wrote: > > > On 2015/06/02 09:31:09, sof wrote: > > > > On 2015/06/02 09:20:59, haraken wrote: > > > > > On 2015/06/02 09:10:32, sof wrote: > > > > > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > > > > > > > > > Yes, it can. However, as far as I consider, I cannot come up with a > > scenario > > > > > where we want to trigger a conservative GC before allocating a new page > > (if > > > > I'm > > > > > not missing something). > > > > > > > > But this change doesn't delay the check until after a new page allocation > > > > either? This is rather delicate, so I think you need to think that through > > > > carefully - the description doesn't account for conservative GC > > implications. > > > > > > > > For instance, might this delay a conservative GC too long. > > > > > > Yes, this change is delicate and doing something non-trivial (but solving an > > > actual problem we're observing in memory.top_7_stress). > > > > > > Regarding a conservative GC: > > > > > > After this CL, a conservative GC is not scheduled until we finish > > completeSweep. > > > However, a conservative GC is scheduled before a new page allocation. In > other > > > words, this CL delays a conservative GC to some extent but the conservative > GC > > > is guaranteed to be scheduled before allocating a new page. Thus the key is > > > whether there is a scenario where we should trigger a conservative GC > without > > > allocating a new page. The fact that a new page is not allocated implies > that > > > there is no peak memory increase since the latest GC (in common cases we can > > > expect memory decrease because the lazy sweeping sweeps dead objects > > > incrementally), which means that a conservative GC wouldn't be needed in > that > > > case. (Of course, we can invent an artificial scenario where the delay of a > > > conservative GC becomes problematic; e.g., Oilpan allocates little but the > > > allocated object allocates a big memory in PartitionAlloc.) > > > > > > Would it make sense? > > > > Thanks, the repositioning doesn't allocate extra before the conservative GC > > check is made. > > > > The unknown here is if we have a fragmented heap and are under memory > pressure, > > the conservative GC is delayed until some later out-of-line allocation. Do we > > know that the near-OOM tests (blink_perf.dom) aren't adversely affected? > > I'll check it tomorrow. > > However, in theory it will rarely happen since a conservative GC is anyway not > invoked until we hit 400% increase in the memory usage. ok, thanks - i'm sorry for pushing back so much on changes in this (notorious) area..
On 2015/06/02 13:51:03, sof wrote: > On 2015/06/02 13:34:45, haraken wrote: > > On 2015/06/02 12:17:30, sof wrote: > > > On 2015/06/02 11:30:54, haraken wrote: > > > > On 2015/06/02 09:31:09, sof wrote: > > > > > On 2015/06/02 09:20:59, haraken wrote: > > > > > > On 2015/06/02 09:10:32, sof wrote: > > > > > > > Doesnt scheduleGCIfNeeded() trigger a conservative GC (if needed)? > > > > > > > > > > > > Yes, it can. However, as far as I consider, I cannot come up with a > > > scenario > > > > > > where we want to trigger a conservative GC before allocating a new > page > > > (if > > > > > I'm > > > > > > not missing something). > > > > > > > > > > But this change doesn't delay the check until after a new page > allocation > > > > > either? This is rather delicate, so I think you need to think that > through > > > > > carefully - the description doesn't account for conservative GC > > > implications. > > > > > > > > > > For instance, might this delay a conservative GC too long. > > > > > > > > Yes, this change is delicate and doing something non-trivial (but solving > an > > > > actual problem we're observing in memory.top_7_stress). > > > > > > > > Regarding a conservative GC: > > > > > > > > After this CL, a conservative GC is not scheduled until we finish > > > completeSweep. > > > > However, a conservative GC is scheduled before a new page allocation. In > > other > > > > words, this CL delays a conservative GC to some extent but the > conservative > > GC > > > > is guaranteed to be scheduled before allocating a new page. Thus the key > is > > > > whether there is a scenario where we should trigger a conservative GC > > without > > > > allocating a new page. The fact that a new page is not allocated implies > > that > > > > there is no peak memory increase since the latest GC (in common cases we > can > > > > expect memory decrease because the lazy sweeping sweeps dead objects > > > > incrementally), which means that a conservative GC wouldn't be needed in > > that > > > > case. (Of course, we can invent an artificial scenario where the delay of > a > > > > conservative GC becomes problematic; e.g., Oilpan allocates little but the > > > > allocated object allocates a big memory in PartitionAlloc.) > > > > > > > > Would it make sense? > > > > > > Thanks, the repositioning doesn't allocate extra before the conservative GC > > > check is made. > > > > > > The unknown here is if we have a fragmented heap and are under memory > > pressure, > > > the conservative GC is delayed until some later out-of-line allocation. Do > we > > > know that the near-OOM tests (blink_perf.dom) aren't adversely affected? > > > > I'll check it tomorrow. > > > > However, in theory it will rarely happen since a conservative GC is anyway not > > invoked until we hit 400% increase in the memory usage. > > ok, thanks - i'm sorry for pushing back so much on changes in this (notorious) > area.. I verified that this CL doesn't change the timing of conservative GCs and performance of textarea-dom.html and LargeDistributionWithoutLayout.html
On 2015/06/03 04:41:43, haraken wrote: ... > > I verified that this CL doesn't change the timing of conservative GCs and > performance of textarea-dom.html and LargeDistributionWithoutLayout.html Locally verified that win32 results on dom-modify.html(*) and textarea-dom.html are ok. With that data in place, lgtm. * - dom-modify.html has regressed "a bit" wrt OOM on trunk; i.e., i can see it OOM every couple of runs.
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/1158683003/1
> * - dom-modify.html has regressed "a bit" wrt OOM on trunk; i.e., i can see it > OOM every couple of runs. Thanks for reporting. That sounds like a problem. I hope https://codereview.chromium.org/1156863002/ will fix some OOMs but I'm not sure if the OOM of dom-modify.html is related to this.
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=196390 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
