Oilpan: We shouldn't trigger too many conservative GCs when detaching iframes

Oilpan: We shouldn't trigger too many conservative GCs when detaching iframes

This is a follow-up fix for https://codereview.chromium.org/1353283002/.
The original CL was not nice in a sense that we force a conservative GC
at every 5 iframe detachment. If we do this, we end up with triggering
too many conservative GCs when detaching a frame that has a lot of iframes.
To avoid the scenario, this CL changes the timing where we trigger the conservative GC.

BUG=474470

[haraken, 2015-09-21 09:56:53]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2015-09-21 09:56:54]

PTAL

[sof, 2015-09-21 10:06:17]

thanks, started some oilpan bots.

Moving the check for page navigation GCs to the destruction of frames makes
sense (on trunk it is sort of positioned as a check during attach/loading.)

[haraken, 2015-09-21 10:41:47]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org

[haraken, 2015-09-21 10:41:48]

On 2015/09/21 10:06:17, sof wrote:
> thanks, started some oilpan bots.
> 
> Moving the check for page navigation GCs to the destruction of frames makes
> sense (on trunk it is sort of positioned as a check during attach/loading.)

Yeah, I'll defer this part to keishi@.

[sof, 2015-09-21 12:29:26]

On 2015/09/21 10:41:48, haraken wrote:
> On 2015/09/21 10:06:17, sof wrote:
> > thanks, started some oilpan bots.
> > 
> > Moving the check for page navigation GCs to the destruction of frames makes
> > sense (on trunk it is sort of positioned as a check during attach/loading.)
> 
> Yeah, I'll defer this part to keishi@.

ok, if performance checks out, then let's try adopting this one instead.

[haraken, 2015-09-21 12:38:08]

On 2015/09/21 12:29:26, sof wrote:
> On 2015/09/21 10:41:48, haraken wrote:
> > On 2015/09/21 10:06:17, sof wrote:
> > > thanks, started some oilpan bots.
> > > 
> > > Moving the check for page navigation GCs to the destruction of frames
makes
> > > sense (on trunk it is sort of positioned as a check during
attach/loading.)
> > 
> > Yeah, I'll defer this part to keishi@.
> 
> ok, if performance checks out, then let's try adopting this one instead.

What you're proposing is to land this CL and then move the
schedulePageNavigationGCIfNeeded() (currently written in
FrameLoader::prepareForCommit) to WindowProxy::disposeContext, isn't it?

[sof, 2015-09-21 13:14:09]

On 2015/09/21 12:38:08, haraken wrote:
> On 2015/09/21 12:29:26, sof wrote:
> > On 2015/09/21 10:41:48, haraken wrote:
> > > On 2015/09/21 10:06:17, sof wrote:
> > > > thanks, started some oilpan bots.
> > > > 
> > > > Moving the check for page navigation GCs to the destruction of frames
> makes
> > > > sense (on trunk it is sort of positioned as a check during
> attach/loading.)
> > > 
> > > Yeah, I'll defer this part to keishi@.
> > 
> > ok, if performance checks out, then let's try adopting this one instead.
> 
> What you're proposing is to land this CL and then move the
> schedulePageNavigationGCIfNeeded() (currently written in
> FrameLoader::prepareForCommit) to WindowProxy::disposeContext, isn't it?

Perhaps, but I do think the arrangement here makes sense, i.e., count the number
of disposes (not frame creation/loads), but "pay" for a sequence of those upon
the next frame creation.

[haraken, 2015-09-21 13:42:33]

On 2015/09/21 13:14:09, sof wrote:
> On 2015/09/21 12:38:08, haraken wrote:
> > On 2015/09/21 12:29:26, sof wrote:
> > > On 2015/09/21 10:41:48, haraken wrote:
> > > > On 2015/09/21 10:06:17, sof wrote:
> > > > > thanks, started some oilpan bots.
> > > > > 
> > > > > Moving the check for page navigation GCs to the destruction of frames
> > makes
> > > > > sense (on trunk it is sort of positioned as a check during
> > attach/loading.)
> > > > 
> > > > Yeah, I'll defer this part to keishi@.
> > > 
> > > ok, if performance checks out, then let's try adopting this one instead.
> > 
> > What you're proposing is to land this CL and then move the
> > schedulePageNavigationGCIfNeeded() (currently written in
> > FrameLoader::prepareForCommit) to WindowProxy::disposeContext, isn't it?
> 
> Perhaps, but I do think the arrangement here makes sense, i.e., count the
number
> of disposes (not frame creation/loads), but "pay" for a sequence of those upon
> the next frame creation.

Like PS3?

I'll ask keishi-san to experiment with Speedometer and see if PS3's performance
is ok.

[sof, 2015-09-21 13:56:40]

On 2015/09/21 13:42:33, haraken wrote:
> On 2015/09/21 13:14:09, sof wrote:
> > On 2015/09/21 12:38:08, haraken wrote:
> > > On 2015/09/21 12:29:26, sof wrote:
> > > > On 2015/09/21 10:41:48, haraken wrote:
> > > > > On 2015/09/21 10:06:17, sof wrote:
> > > > > > thanks, started some oilpan bots.
> > > > > > 
> > > > > > Moving the check for page navigation GCs to the destruction of
frames
> > > makes
> > > > > > sense (on trunk it is sort of positioned as a check during
> > > attach/loading.)
> > > > > 
> > > > > Yeah, I'll defer this part to keishi@.
> > > > 
> > > > ok, if performance checks out, then let's try adopting this one instead.
> > > 
> > > What you're proposing is to land this CL and then move the
> > > schedulePageNavigationGCIfNeeded() (currently written in
> > > FrameLoader::prepareForCommit) to WindowProxy::disposeContext, isn't it?
> > 
> > Perhaps, but I do think the arrangement here makes sense, i.e., count the
> number
> > of disposes (not frame creation/loads), but "pay" for a sequence of those
upon
> > the next frame creation.
> 
> Like PS3?
> 
> I'll ask keishi-san to experiment with Speedometer and see if PS3's
performance
> is ok.

No, I was referring to ps#1. If you want to situate the schedule*GC() call like
in ps#3, then would having it in clearForNavigation() be too far up?

[haraken, 2015-09-21 14:06:04]

On 2015/09/21 13:56:40, sof wrote:
> On 2015/09/21 13:42:33, haraken wrote:
> > On 2015/09/21 13:14:09, sof wrote:
> > > On 2015/09/21 12:38:08, haraken wrote:
> > > > On 2015/09/21 12:29:26, sof wrote:
> > > > > On 2015/09/21 10:41:48, haraken wrote:
> > > > > > On 2015/09/21 10:06:17, sof wrote:
> > > > > > > thanks, started some oilpan bots.
> > > > > > > 
> > > > > > > Moving the check for page navigation GCs to the destruction of
> frames
> > > > makes
> > > > > > > sense (on trunk it is sort of positioned as a check during
> > > > attach/loading.)
> > > > > > 
> > > > > > Yeah, I'll defer this part to keishi@.
> > > > > 
> > > > > ok, if performance checks out, then let's try adopting this one
instead.
> > > > 
> > > > What you're proposing is to land this CL and then move the
> > > > schedulePageNavigationGCIfNeeded() (currently written in
> > > > FrameLoader::prepareForCommit) to WindowProxy::disposeContext, isn't it?
> > > 
> > > Perhaps, but I do think the arrangement here makes sense, i.e., count the
> > number
> > > of disposes (not frame creation/loads), but "pay" for a sequence of those
> upon
> > > the next frame creation.
> > 
> > Like PS3?
> > 
> > I'll ask keishi-san to experiment with Speedometer and see if PS3's
> performance
> > is ok.
> 
> No, I was referring to ps#1. If you want to situate the schedule*GC() call
like
> in ps#3, then would having it in clearForNavigation() be too far up?

Ah, understood. Then can we just land PS1? Given that the page navigation GC is
orthogonal to the iframe destruction GC, this CL won't change behavior of the
page navigation GCs.

[sof, 2015-09-21 14:10:14]

On 2015/09/21 14:06:04, haraken wrote:
> On 2015/09/21 13:56:40, sof wrote:
> > On 2015/09/21 13:42:33, haraken wrote:
> > > On 2015/09/21 13:14:09, sof wrote:
> > > > On 2015/09/21 12:38:08, haraken wrote:
> > > > > On 2015/09/21 12:29:26, sof wrote:
> > > > > > On 2015/09/21 10:41:48, haraken wrote:
> > > > > > > On 2015/09/21 10:06:17, sof wrote:
> > > > > > > > thanks, started some oilpan bots.
> > > > > > > > 
> > > > > > > > Moving the check for page navigation GCs to the destruction of
> > frames
> > > > > makes
> > > > > > > > sense (on trunk it is sort of positioned as a check during
> > > > > attach/loading.)
> > > > > > > 
> > > > > > > Yeah, I'll defer this part to keishi@.
> > > > > > 
> > > > > > ok, if performance checks out, then let's try adopting this one
> instead.
> > > > > 
> > > > > What you're proposing is to land this CL and then move the
> > > > > schedulePageNavigationGCIfNeeded() (currently written in
> > > > > FrameLoader::prepareForCommit) to WindowProxy::disposeContext, isn't
it?
> > > > 
> > > > Perhaps, but I do think the arrangement here makes sense, i.e., count
the
> > > number
> > > > of disposes (not frame creation/loads), but "pay" for a sequence of
those
> > upon
> > > > the next frame creation.
> > > 
> > > Like PS3?
> > > 
> > > I'll ask keishi-san to experiment with Speedometer and see if PS3's
> > performance
> > > is ok.
> > 
> > No, I was referring to ps#1. If you want to situate the schedule*GC() call
> like
> > in ps#3, then would having it in clearForNavigation() be too far up?
> 
> Ah, understood. Then can we just land PS1? Given that the page navigation GC
is
> orthogonal to the iframe destruction GC, this CL won't change behavior of the
> page navigation GCs.

I'm fine with landing ps1 and iterate on that one based on keishi's assessment &
perf findings later this week or next.

[sof, 2015-09-21 14:12:34]

actually it was ps#2 i looked at. lgtm to land it.

[haraken, 2015-09-21 14:14:44]

On 2015/09/21 14:12:34, sof wrote:
> actually it was ps#2 i looked at. lgtm to land it.

Thanks, reuploaded PS2.

[haraken, 2015-09-21 14:14:49]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-09-21 14:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1358813002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-09-21 14:14:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358813002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358813002/60001

[sof, 2015-09-21 14:36:14]

lgtm ps#4

[commit-bot: I haz the power, 2015-09-21 15:57:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-21 15:57:46]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2015-09-21 18:19:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-09-21 18:20:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358813002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358813002/60001

[commit-bot: I haz the power, 2015-09-21 20:31:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-21 20:31:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Russell (switch to Gerrit), 2015-09-22 01:50:51]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-09-22 01:50:52]

Please hold off landing this until the crashes in http://crbug.com/534524 have
been diagnosed and fixed.

[Ken Russell (switch to Gerrit), 2015-09-22 01:52:26]

On 2015/09/21 20:31:32, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Crashed with an assertion failure:

13:19:48.964 5313 worker/7
imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html crashed,
(stderr lines):
13:19:48.964 5313   ASSERTION FAILED: m_lastRemainingAllocationSize ==
remainingAllocationSize()
13:19:48.964 5313  
../../third_party/WebKit/Source/platform/heap/HeapPage.cpp(768) : void
blink::NormalPageHeap::updateRemainingAllocationSize()
13:19:48.964 5313   1   0x1cf1894
13:19:48.964 5313   2   0x1cfac9f
13:19:48.964 5313   3   0x1cfae83
13:19:48.964 5313   4   0x1cee1a8
13:19:48.964 5313   5   0x1cee528
13:19:48.964 5313   6   0x137e48c
13:19:48.964 5313   7   0x137dd70
13:19:48.964 5313   8   0x137dc22
13:19:48.965 5313   9   0x133ebf6
13:19:48.965 5313   10  0x133eb64
13:19:48.965 5313   11  0x133fe9f
13:19:48.965 5313   12  0x21f4ab9
13:19:48.965 5313   13  0x22effbc
13:19:48.965 5313   14  0x22efd82
13:19:48.965 5313   15  0x22ee7e0
13:19:48.965 5313   16  0x22ee4cb
13:19:48.965 5313   17  0x22f0acf
13:19:48.965 5313   18  0x22f0fae
13:19:48.965 5313   19  0x2303134
13:19:48.965 5313   20  0x22fff92
13:19:48.965 5313   21  0x12ededb
13:19:48.965 5313   22  0x12edfd3
13:19:48.965 5313   23  0x35a277f
13:19:48.965 5313   24  0x35a229b
13:19:48.965 5313   25  0x321f996
13:19:48.965 5313   26  0x6ea5b7
13:19:48.965 5313   27  0x6ea4f8
13:19:48.965 5313   28  0x2a58bfd
13:19:48.965 5313   29  0x707b64
13:19:48.965 5313   30  0x4aaf13
13:19:48.965 5313   31  0x2b89a0e
13:19:48.965 5313  
[16652:16705:0921/131948:2889639518:WARNING:crash_handler_host_linux.cc(290)]
Could not translate tid - assuming crashing thread is thread group leader;
syscall_supported=1
13:19:48.972 5229 [30419/37627]
imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html failed
unexpectedly (renderer crashed [pid=16721])
13:19:48.966 5313 worker/7 killing primary driver
13:19:48.967 5313 worker/7 killing secondary driver
13:19:48.967 5313 worker/7
imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html failed:
13:19:48.967 5313 worker/7  renderer crashed [pid=16721]

[sof, 2015-09-22 15:40:57]

On 2015/09/22 01:52:26, Ken Russell wrote:
> On 2015/09/21 20:31:32, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Crashed with an assertion failure:
> 
> 13:19:48.964 5313 worker/7
> imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html
crashed,
> (stderr lines):
> 13:19:48.964 5313   ASSERTION FAILED: m_lastRemainingAllocationSize ==
> remainingAllocationSize()
>
...
> 13:19:48.967 5313 worker/7  renderer crashed [pid=16721]

Thanks, handled by http://crbug.com/534423 . A bug brought out in the open by
https://codereview.chromium.org/1353283002 , but not due to it per se.

[haraken, 2015-09-23 03:20:51]

On 2015/09/22 15:40:57, sof wrote:
> On 2015/09/22 01:52:26, Ken Russell wrote:
> > On 2015/09/21 20:31:32, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > Crashed with an assertion failure:
> > 
> > 13:19:48.964 5313 worker/7
> > imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html
> crashed,
> > (stderr lines):
> > 13:19:48.964 5313   ASSERTION FAILED: m_lastRemainingAllocationSize ==
> > remainingAllocationSize()
> >
> ...
> > 13:19:48.967 5313 worker/7  renderer crashed [pid=16721]
> 
> Thanks, handled by http://crbug.com/534423 . A bug brought out in the open by
> https://codereview.chromium.org/1353283002 , but not due to it per se.

Thanks for handling the crash. Shall we land PS5 now? (Or should we investigate
https://code.google.com/p/chromium/issues/detail?id=534524?)

[sof, 2015-09-23 06:20:57]

On 2015/09/23 03:20:51, haraken wrote:
> On 2015/09/22 15:40:57, sof wrote:
> > On 2015/09/22 01:52:26, Ken Russell wrote:
> > > On 2015/09/21 20:31:32, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > > 
> > > Crashed with an assertion failure:
> > > 
> > > 13:19:48.964 5313 worker/7
> > > imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html
> > crashed,
> > > (stderr lines):
> > > 13:19:48.964 5313   ASSERTION FAILED: m_lastRemainingAllocationSize ==
> > > remainingAllocationSize()
> > >
> > ...
> > > 13:19:48.967 5313 worker/7  renderer crashed [pid=16721]
> > 
> > Thanks, handled by http://crbug.com/534423 . A bug brought out in the open
by
> > https://codereview.chromium.org/1353283002 , but not due to it per se.
> 
> Thanks for handling the crash. Shall we land PS5 now? (Or should we
investigate
> https://code.google.com/p/chromium/issues/detail?id=534524?)

534423 addresses inaccurate accounting of objects allocated for a (vector
backing) heap. I couldn't see any direct connection with 534524's crash stack
when considering it yesterday.

There might be an opportunity to understand&fix a latent bug with ps#4, so let's
try to isolate that a bit first to gain some confidence wrt 534524? I'll kick
off a series mac_chromium_rel_ng runs against ps#4 this morning, and see how
those fare.

[Ken Russell (switch to Gerrit), 2015-09-23 17:36:41]

On 2015/09/23 06:20:57, sof wrote:
> On 2015/09/23 03:20:51, haraken wrote:
> > On 2015/09/22 15:40:57, sof wrote:
> > > On 2015/09/22 01:52:26, Ken Russell wrote:
> > > > On 2015/09/21 20:31:32, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > > > 
> > > > Crashed with an assertion failure:
> > > > 
> > > > 13:19:48.964 5313 worker/7
> > > > imported/web-platform-tests/html/syntax/parsing/html5lib_tables01.html
> > > crashed,
> > > > (stderr lines):
> > > > 13:19:48.964 5313   ASSERTION FAILED: m_lastRemainingAllocationSize ==
> > > > remainingAllocationSize()
> > > >
> > > ...
> > > > 13:19:48.967 5313 worker/7  renderer crashed [pid=16721]
> > > 
> > > Thanks, handled by http://crbug.com/534423 . A bug brought out in the open
> by
> > > https://codereview.chromium.org/1353283002 , but not due to it per se.
> > 
> > Thanks for handling the crash. Shall we land PS5 now? (Or should we
> investigate
> > https://code.google.com/p/chromium/issues/detail?id=534524?)
> 
> 534423 addresses inaccurate accounting of objects allocated for a (vector
> backing) heap. I couldn't see any direct connection with 534524's crash stack
> when considering it yesterday.
> 
> There might be an opportunity to understand&fix a latent bug with ps#4, so
let's
> try to isolate that a bit first to gain some confidence wrt 534524? I'll kick
> off a series mac_chromium_rel_ng runs against ps#4 this morning, and see how
> those fare.

Please, please, try to reproduce crbug.com/534524 before attempting to re-land a
policy change in this area. In fact, it would be better if you could re-test
locally with the patch from https://codereview.chromium.org/1353283002 , since
that exposed the latent bug pretty reliably. The last patch introduced flaky
failures and basically caused every CL against win_chromium_rel_ng and
mac_chromium_rel_ng that caused Chrome to be built to be rejected, which would
have led to the CQ spiraling out of control. Let's get to the root cause of
http://crbug.com/534524 first.

[sof, 2015-09-26 14:27:18]

Has the performance of this one (and/or the initial version that landed) been
tested more widely, or was that put on hold until the stability issues had been
sorted out?

Given the short week it was, I assume it hasn't been; just curious.