scheduler: Detect load RAIL mode

scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

As a safeguard against cases where FMP is not detected, we terminate the
load stage if we see user input. The assumption is that if the user is interacting
with the page, there is likely meaningful content on the screen at that point.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/e5da9cc2e06c351293a55b4f3c3fc9d39f1e2bfc
Cr-Commit-Position: refs/heads/master@{#435616}

[Sami, 2016-10-06 17:58:33]

Description was changed from

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

BUG=613520
==========

to

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Sami, 2016-10-06 17:58:38]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[Sami, 2016-10-06 17:59:24]

Description was changed from

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Sami, 2016-10-06 17:59:24]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org

[commit-bot: I haz the power, 2016-10-06 17:59:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-06 18:01:14]

PTAL. As discussed a slight deficiency here is that we only track navigations on
a per-renderer level, so two tabs sharing a renderer may interfere (e.g., one
tab finishing the load sooner will also kick the other tab out of loading mode).
I don't think this should cause any actual problems however.

[commit-bot: I haz the power, 2016-10-06 18:39:17]

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

[commit-bot: I haz the power, 2016-10-06 18:39:18]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Sami, 2016-10-07 10:00:03]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 10:00:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 12:48:55]

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

[commit-bot: I haz the power, 2016-10-07 12:48:56]

Dry run: This issue passed the CQ dry run.

[ulan, 2016-10-10 18:47:40]

On 2016/10/06 18:01:14, Sami (slow -- traveling) wrote:
> PTAL. As discussed a slight deficiency here is that we only track navigations
on
> a per-renderer level, so two tabs sharing a renderer may interfere (e.g., one
> tab finishing the load sooner will also kick the other tab out of loading
mode).
> I don't think this should cause any actual problems however.

Scrolling a mobile facebook website triggers many load signals (logcat output
below). Is this expected?
Command line to reproduce:  tools/perf/run_benchmark 
v8.mobile_infinite_scroll_tbmv2 --story-filter=facebook
--browser=android-chromium --device=android
--extra-browser-args="--js-flags='--trace-rail'"

Desktop version sends only one load signal at the beginning.

10-10 14:41:20.673  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:20.673  8470  8483 I v8      : RAIL mode: LOAD
10-10 14:41:22.924  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:22.925  8470  8483 I v8      : RAIL mode: ANIMATION
10-10 14:41:23.089  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:23.089  8470  8483 I v8      : RAIL mode: LOAD
10-10 14:41:32.006  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:32.007  8470  8483 I v8      : RAIL mode: ANIMATION
10-10 14:41:32.320  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:32.321  8470  8483 I v8      : RAIL mode: RESPONSE
10-10 14:41:33.718  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:33.718  8470  8483 I v8      : RAIL mode: LOAD
10-10 14:41:33.723  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:33.723  8470  8483 I v8      : RAIL mode: RESPONSE
10-10 14:41:46.724  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:46.725  8470  8483 I v8      : RAIL mode: LOAD
10-10 14:41:46.742  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:46.742  8470  8483 I v8      : RAIL mode: RESPONSE
10-10 14:41:50.166  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:50.166  8470  8483 I v8      : RAIL mode: LOAD
10-10 14:41:50.182  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:50.183  8470  8483 I v8      : RAIL mode: RESPONSE
10-10 14:41:51.917  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:51.917  8470  8483 I v8      : RAIL mode: LOAD
10-10 14:41:51.931  8470  8483 I v8      : [8470:0xeefc2000] 
10-10 14:41:51.931  8470  8483 I v8      : RAIL mode: RESPONSE

[Sami, 2016-10-11 05:23:17]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 05:23:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-11 05:24:21]

On 2016/10/10 18:47:40, ulan wrote:
> On 2016/10/06 18:01:14, Sami (slow -- traveling) wrote:
> > PTAL. As discussed a slight deficiency here is that we only track
navigations
> on
> > a per-renderer level, so two tabs sharing a renderer may interfere (e.g.,
one
> > tab finishing the load sooner will also kick the other tab out of loading
> mode).
> > I don't think this should cause any actual problems however.
> 
> Scrolling a mobile facebook website triggers many load signals (logcat output
> below). Is this expected?

Interesting, I think this may be a bug with how we detect top-level navigations.
Could you try the latest patch to see if this goes away? (I'm travelling so I
can't test this easily for the next two weeks.)

[commit-bot: I haz the power, 2016-10-11 05:34:02]

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

[commit-bot: I haz the power, 2016-10-11 05:34:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Sami, 2016-10-11 05:37:08]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 05:37:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 07:15:58]

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

[commit-bot: I haz the power, 2016-10-11 07:15:58]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-10-11 10:32:50]

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/web_scheduler_impl.h
(left):

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/web_scheduler_impl.h:49: void
onNavigationStarted() override {}
Hmm looks like we never got round to using that.  Do you remember what we were
intending to do?

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:912:
// priorities.
This is probably sensible for now. Presumably we'd want to throttle compositing
somehow, although I note the current throttling is likely too harsh, rather
limiting compositing to say 50% of CPU seems reasonable?

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1105:
if (AnyThread().waiting_for_meaningful_paint)
So until first meaningful paint, if the user isn't trying to scroll, we'll enter
loading policy.  Is that the right thing to do? Should we abandon loading policy
after the user has interacted, or is this OK?

What if a desktop user presses a key? Currently we don't notice that and we'd
stay in loading policy.

[Sami, 2016-10-12 04:21:41]

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/web_scheduler_impl.h
(left):

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/web_scheduler_impl.h:49: void
onNavigationStarted() override {}
On 2016/10/11 10:32:50, alex clarke wrote:
> Hmm looks like we never got round to using that.  Do you remember what we were
> intending to do?

IIRC we speculatively added this for doing load time scheduling before we
realized those need to be hooked up from the content layer instead. Doesn't seem
like we need to keep these around anymore.

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:912:
// priorities.
On 2016/10/11 10:32:50, alex clarke wrote:
> This is probably sensible for now. Presumably we'd want to throttle
compositing
> somehow, although I note the current throttling is likely too harsh, rather
> limiting compositing to say 50% of CPU seems reasonable?

Yeah, something like that might be interesting. Although, if nothing else is
competing for CPU during loading I think we might still want to let rendering
happen at full speed. Added this as a reminder in the comment here.

https://codereview.chromium.org/2397753006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1105:
if (AnyThread().waiting_for_meaningful_paint)
On 2016/10/11 10:32:50, alex clarke wrote:
> So until first meaningful paint, if the user isn't trying to scroll, we'll
enter
> loading policy.  Is that the right thing to do? Should we abandon loading
policy
> after the user has interacted, or is this OK?
>
> What if a desktop user presses a key? Currently we don't notice that and we'd
> stay in loading policy.

Right, this implementation of 'L' is very greedy in the sense that it can remain
in loading mode for much longer than necessary (possibly forever). For now I
think this is fine because v8 takes precautions against it.

I think the way to fix this in the long term is to make the first meaningful
paint signal more reliable instead of trying to double-guess things here in the
scheduler. If the user presses a key or tries to otherwise interact with the
page before FMP, we should be able to assume that loading is still in progress
because there's nothing meaningful on the screen yet.

What we can and should do (and already do in this patch) is to let gestures take
us temporarily into other use cases so that things work nicely even if FMP is
wrong. Once we start doing actual prioritization I suspect we'll need bit more
of a safety net for pages where FMP isn't realiable (e.g., someone mentioned the
facebook chat widget which is basically always loading).

[Sami, 2016-10-12 06:34:44]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[Sami, 2016-10-12 06:34:53]

skyostil@chromium.org changed reviewers:
+ kouhei@chromium.org

[commit-bot: I haz the power, 2016-10-12 06:34:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-12 06:35:11]

+kouhei@, here's the patch I was talking about.

[kouhei (in TOK), 2016-10-12 06:36:45]

kouhei@chromium.org changed reviewers:
+ ksakamoto@chromium.org

[kouhei (in TOK), 2016-10-12 06:36:46]

+ksakamoto

[commit-bot: I haz the power, 2016-10-12 08:08:30]

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

[commit-bot: I haz the power, 2016-10-12 08:08:31]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2016-10-13 08:01:35]

https://codereview.chromium.org/2397753006/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2397753006/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:76: if
(!m_firstContentfulPaint)
Within a single paint phase, this runs after
PaintTiming::setFirstContentfulPaint(). So this won't skip FCP if it is a FMP
candidate. Is that OK?

[Sami, 2016-10-13 08:23:59]

https://codereview.chromium.org/2397753006/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2397753006/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:76: if
(!m_firstContentfulPaint)
On 2016/10/13 08:01:34, Kunihiko Sakamoto wrote:
> Within a single paint phase, this runs after
> PaintTiming::setFirstContentfulPaint(). So this won't skip FCP if it is a FMP
> candidate. Is that OK?

Good point. Perhaps the thing to do is to ignore the first FMP candidate after
FCP with the theory that it is usually the FCP itself.

[Kunihiko Sakamoto, 2016-10-13 08:38:46]

https://codereview.chromium.org/2397753006/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2397753006/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:76: if
(!m_firstContentfulPaint)
On 2016/10/13 08:23:59, Sami (slow -- traveling) wrote:
> On 2016/10/13 08:01:34, Kunihiko Sakamoto wrote:
> > Within a single paint phase, this runs after
> > PaintTiming::setFirstContentfulPaint(). So this won't skip FCP if it is a
FMP
> > candidate. Is that OK?
> 
> Good point. Perhaps the thing to do is to ignore the first FMP candidate after
> FCP with the theory that it is usually the FCP itself.

Yeah, I think patch set 9 would work.

[ulan, 2016-10-18 11:10:14]

> Interesting, I think this may be a bug with how we detect top-level
navigations.
> Could you try the latest patch to see if this goes away? (I'm travelling so I
> can't test this easily for the next two weeks.

PS9 also sends multiple LOAD signals on mobile facebook

[ulan, 2016-11-08 15:36:17]

Regarding the multiple LOAD signals when scrolling mobile facebook:

RenderFrameImpl::didCommitProvisionalLoad calls
render_thread_impl->GetRendererScheduler()->OnNavigationStarted() 
[https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rcl=1478593042&l=3633]

That function checks navigation_state->WasWithinSamePage() in some cases:
[https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rcl=1478593042&l=3562]

I wonder if the call of OnNavigationStarted should also have a guard based on
navigation_state->WasWithinSamePage()?

[Sami, 2016-11-16 14:36:28]

On 2016/11/08 15:36:17, ulan wrote:
> Regarding the multiple LOAD signals when scrolling mobile facebook:
> 
> RenderFrameImpl::didCommitProvisionalLoad calls
> render_thread_impl->GetRendererScheduler()->OnNavigationStarted() 
>
[https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rcl=1478593042&l=3633]
> 
> That function checks navigation_state->WasWithinSamePage() in some cases:
>
[https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rcl=1478593042&l=3562]
> 
> I wonder if the call of OnNavigationStarted should also have a guard based on
> navigation_state->WasWithinSamePage()?

Thanks for digging into this Ulan -- and sorry for dropping the ball on this
patch for a while. Your theory sounds plausible; I'll look into the loading
logic first thing when I get back to LON next week.

[Sami, 2016-11-24 17:40:04]

It took a while but I finally managed to reproduce the multiple loads -issue.
The problem was that sometimes the first meaningful paint detector fails to
fire, which means we'll either stay in loading mode perpetually or go in and out
of it between gestures.

As a safeguard I'm now terminating the load stage if we see input from the user.
The hypothesis is that if the user is interacting with the page, there is likely
meaningful content on the screen.

Alex, could you review third_party/WebKit/Source/platform/scheduler please?
Kouhei, can you have a look at third_party/WebKit/Source/core/paint/?

[Sami, 2016-11-24 17:41:19]

Description was changed from

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

As a safeguard against cases where FMP is not detected, we terminate the
load stage if we see user input. The assumption is that if the user is
interacting
with the page, there is likely meaningful content on the screen at that point.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[alex clarke (OOO till 29th), 2016-11-24 17:47:42]

On 2016/11/24 17:40:04, Sami wrote:
> It took a while but I finally managed to reproduce the multiple loads -issue.
> The problem was that sometimes the first meaningful paint detector fails to
> fire, which means we'll either stay in loading mode perpetually or go in and
out
> of it between gestures.
> 
> As a safeguard I'm now terminating the load stage if we see input from the
user.
> The hypothesis is that if the user is interacting with the page, there is
likely
> meaningful content on the screen.
> 
> Alex, could you review third_party/WebKit/Source/platform/scheduler please?
> Kouhei, can you have a look at third_party/WebKit/Source/core/paint/?

It might be interesting to emit a devtools event for FirstMeaningfulPaint in a
follow on patch.

[alex clarke (OOO till 29th), 2016-11-24 17:54:23]

lgtm

https://codereview.chromium.org/2397753006/diff/190001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2397753006/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1499:
UpdatePolicy();
Can we call 
  UpdatePolicyLocked(UpdateType::MAY_EARLY_OUT_IF_POLICY_UNCHANGED); inside the
lock?

[Sami, 2016-11-24 18:41:38]

Thanks!

On 2016/11/24 17:47:42, alex clarke wrote:
> It might be interesting to emit a devtools event for FirstMeaningfulPaint in a
> follow on patch.

For sure. I think there are folks working on this already.

+Kentaro, could I have your stamp for public/platform and platform/testing
please?

https://codereview.chromium.org/2397753006/diff/190001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2397753006/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1499:
UpdatePolicy();
On 2016/11/24 17:54:23, alex clarke wrote:
> Can we call 
>   UpdatePolicyLocked(UpdateType::MAY_EARLY_OUT_IF_POLICY_UNCHANGED); inside
the
> lock?

Oh, well spotted, thanks!

[Sami, 2016-11-24 18:43:04]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 18:43:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-11-24 18:45:44]

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

[Sami, 2016-11-24 18:45:58]

+haraken@ for real.

[commit-bot: I haz the power, 2016-11-24 19:27:54]

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

[commit-bot: I haz the power, 2016-11-24 19:27:56]

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

[haraken, 2016-11-25 00:20:22]

Looks good but I want to have kouhei@ confirm the change.

> As a safeguard against cases where FMP is not detected, we terminate the
> load stage if we see user input. The assumption is that if the user is
interacting
> with the page, there is likely meaningful content on the screen at that point.

^^^ Where is this logic implemented in the CL?

[Sami, 2016-11-25 11:42:01]

On 2016/11/25 00:20:22, haraken wrote:
> Looks good but I want to have kouhei@ confirm the change.
> 
> > As a safeguard against cases where FMP is not detected, we terminate the
> > load stage if we see user input. The assumption is that if the user is
> interacting
> > with the page, there is likely meaningful content on the screen at that
point.
> 
> ^^^ Where is this logic implemented in the CL?

It's line 1195 of renderer_scheduler_impl.cc, and there's a corresponding test
too.

[Sami, 2016-11-29 12:46:49]

Kouhei, ping?

[kouhei (in TOK), 2016-11-30 01:30:20]

^ Source/core/paint lg
I'd like to delegate Source/core/paint review to ksakamoto@

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:75: // paint.
I wonder if we can modify FMP to be always after FCP.

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.h:103: bool
m_seenFirstMeaningfulPaintCandidate = false;
We prefer this flag/logic to be inside FirstMeaningfulPaintDetector

[kouhei (in TOK), 2016-11-30 01:36:52]

lgtm

mostly lg

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:75: // paint.
On 2016/11/30 01:30:20, kouhei wrote:
> I wonder if we can modify FMP to be always after FCP.
Please ignore the comment above.

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.h:103: bool
m_seenFirstMeaningfulPaintCandidate = false;
On 2016/11/30 01:30:20, kouhei wrote:
> We prefer this flag/logic to be inside FirstMeaningfulPaintDetector
Chatted offline w/ ksakamoto.

I think we have two options (we are fine with either one):
- FMP<FCP on very rare cases (like drawing pictures w/ CSS boxes
http://purecss3.net/doraemon/doraemon_css3.html o_O) so we can safely ignore
them.
- If you would like to keep this logic, please move this flag/logic to
FirstMeaningfulPaintDetector

[Sami, 2016-11-30 18:07:09]

Thanks!

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2397753006/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.h:103: bool
m_seenFirstMeaningfulPaintCandidate = false;
On 2016/11/30 01:36:52, kouhei wrote:
> On 2016/11/30 01:30:20, kouhei wrote:
> > We prefer this flag/logic to be inside FirstMeaningfulPaintDetector
> Chatted offline w/ ksakamoto.
> 
> I think we have two options (we are fine with either one):
> - FMP<FCP on very rare cases (like drawing pictures w/ CSS boxes
> http://purecss3.net/doraemon/doraemon_css3.html o_O) so we can safely ignore
> them.

Great example :)

> - If you would like to keep this logic, please move this flag/logic to
> FirstMeaningfulPaintDetector

I've now moved this to FirstMeaningfulPaintDetector and modified it so that we
don't mind if FMP<FCP, but we still ignore the first FMP_c so that we don't
declare success too early. Does that sound like a good compromise?

[Sami, 2016-11-30 18:08:15]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 18:08:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 19:43:58]

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

[commit-bot: I haz the power, 2016-11-30 19:43:59]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2016-12-01 02:00:09]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2016-12-01 02:00:10]

lgtm

Thanks

[kouhei (in TOK), 2016-12-01 02:00:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2397753006/#ps230001
(title: "Review comments + new test")

[commit-bot: I haz the power, 2016-12-01 02:00:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 02:08:29]

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

[commit-bot: I haz the power, 2016-12-01 02:08:33]

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

[haraken, 2016-12-01 02:12:32]

owner LGTM

[Sami, 2016-12-01 11:08:01]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-12-01 11:08:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 11:14:01]

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

[commit-bot: I haz the power, 2016-12-01 11:14:03]

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

[Sami, 2016-12-01 14:01:52]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-12-01 14:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kouhei@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2397753006/#ps250001
(title: "Rebased")

[commit-bot: I haz the power, 2016-12-01 14:02:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 15:36:47]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1480600912029600,
"parent_rev": "4b109d9d9728236e1f6555c27ad4430eb0666d30", "commit_rev":
"6ef25ffed932e7119ca33bdeb985f2bc5f509a7f"}

[commit-bot: I haz the power, 2016-12-01 15:37:09]

Description was changed from

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

As a safeguard against cases where FMP is not detected, we terminate the
load stage if we see user input. The assumption is that if the user is
interacting
with the page, there is likely meaningful content on the screen at that point.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

As a safeguard against cases where FMP is not detected, we terminate the
load stage if we see user input. The assumption is that if the user is
interacting
with the page, there is likely meaningful content on the screen at that point.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-12-01 15:37:11]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-12-01 15:38:49]

Description was changed from

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

As a safeguard against cases where FMP is not detected, we terminate the
load stage if we see user input. The assumption is that if the user is
interacting
with the page, there is likely meaningful content on the screen at that point.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
scheduler: Detect load RAIL mode

This patch implements detection for the loading RAIL mode. This mode
starts when the top-level frame navigates and lasts until we see the
first meaningful paint for the top-level frame (which is currently not
guaranteed to happen on all pages).

We don't currently use this state for scheduling but only pass the
information to v8 which can use it to adjust garbage collection
policies.

As a safeguard against cases where FMP is not detected, we terminate the
load stage if we see user input. The assumption is that if the user is
interacting
with the page, there is likely meaningful content on the screen at that point.

BUG=613520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/e5da9cc2e06c351293a55b4f3c3fc9d39f1e2bfc
Cr-Commit-Position: refs/heads/master@{#435616}
==========

[commit-bot: I haz the power, 2016-12-01 15:38:51]

Patchset 14 (id:??) landed as
https://crrev.com/e5da9cc2e06c351293a55b4f3c3fc9d39f1e2bfc
Cr-Commit-Position: refs/heads/master@{#435616}