Pass main thread responsiveness threshold via Finch

Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:
--enable-features="MainThreadBusyScrollIntervention"
--force-fieldtrials=MainThreadResponsivenessScrollIntervention/Enabled150

BUG=599609
TEST=MainThreadEventQueueInitializationTest, RendererSchedulerImplTest, manual testing of passing via commandline.

Review-Url: https://codereview.chromium.org/2581243004
Cr-Commit-Position: refs/heads/master@{#449276}
Committed: https://chromium.googlesource.com/chromium/src/+/0c4dc23c2c1a24df3f57347c0765b4872730c632

[tdresser, 2016-12-16 19:51:37]

Description was changed from

==========
Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:

--enable-features="MainThreadBusyScrollIntervention<Foo" \
  --force-fieldtrials=Foo/Group1 \
  --force-fieldtrial-params=Foo.Group1:threshold/200

BUG=599609
TEST=RendererSchedulerImplTest, manual testing of passing via commandline.
==========

to

==========
Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:

--enable-features="MainThreadBusyScrollIntervention<Foo" \
  --force-fieldtrials=Foo/Group1 \
  --force-fieldtrial-params=Foo.Group1:threshold/200

BUG=599609
TEST=RendererSchedulerImplTest, manual testing of passing via commandline.
==========

[tdresser, 2016-12-16 19:51:38]

tdresser@chromium.org changed reviewers:
+ jwd@chromium.org

[tdresser, 2016-12-16 19:53:28]

tdresser@chromium.org changed reviewers:
+ jochen@chromium.org

[tdresser, 2016-12-16 19:53:28]

+jwd for variations dependency
+jochen@ for the rest

+Sami & Alex FYI.

[tdresser, 2016-12-16 19:53:34]

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

[commit-bot: I haz the power, 2016-12-16 19:54:17]

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

[dtapuska, 2016-12-16 19:59:45]

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

[dtapuska, 2016-12-16 19:59:46]

lgtm % nit

https://codereview.chromium.org/2581243004/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2581243004/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:90:
enable_non_blocking_due_to_main_thread_responsiveness_flag_ = false;
What about negative values? And StringToInt failing for overflow and underflow;
probably want it disabled in those cases no?

I'd expect this to be

if (!StringToInt(..) || threshold_ms <= 0)

[commit-bot: I haz the power, 2016-12-16 20:01:27]

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

[commit-bot: I haz the power, 2016-12-16 20:01:27]

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

[tdresser, 2016-12-16 20:12:08]

https://codereview.chromium.org/2581243004/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2581243004/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:90:
enable_non_blocking_due_to_main_thread_responsiveness_flag_ = false;
On 2016/12/16 19:59:46, dtapuska wrote:
> What about negative values? And StringToInt failing for overflow and
underflow;
> probably want it disabled in those cases no?
> 
> I'd expect this to be
> 
> if (!StringToInt(..) || threshold_ms <= 0)

StringToInt saturates for overflow and underflow, which I think is fine. Added
handling for negative numbers.

[tdresser, 2016-12-16 20:12:27]

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

[commit-bot: I haz the power, 2016-12-16 20:13:39]

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-12-16 22:38:27]

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

[commit-bot: I haz the power, 2016-12-16 22:38:28]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-19 11:52:42]

https://codereview.chromium.org/2581243004/diff/20001/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/2581243004/diff/20001/content/renderer/BUILD....
content/renderer/BUILD.gn:407: "//components/variations",
variations depend on prefs, but we don't have prefs available in the renderer,
right?

if there's a part of variations that is independend of prefs, can we factor that
out, so the renderer only depends on that?

[tdresser, 2016-12-19 13:25:39]

tdresser@chromium.org changed reviewers:
+ rkaplow@chromium.org

[tdresser, 2016-12-19 13:25:40]

There appears to be some code for syncing field trial data to the renderer
(https://cs.chromium.org/chromium/src/chrome/renderer/chrome_render_thread_obs...),
but I'm not sure how to verify the behavior of this locally, and it doesn't look
like it supports params.

+rkaplow, should the field trial syncer handle this case? Can I test it locally?

[Sami, 2016-12-19 16:05:26]

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

[Sami, 2016-12-19 16:05:26]

https://codereview.chromium.org/2581243004/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2581243004/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:85: std::string threshold =
variations::GetVariationParamValueByFeature(
Are you sure this works? The comment here says this only works for the browser
process at the moment:

https://cs.chromium.org/chromium/src/components/variations/variations_associa...

We had to introduce a new WebSetting[1] for something similar because of this.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSetting...

[jwd, 2016-12-19 16:48:52]

On 2016/12/19 16:05:26, Sami wrote:
>
https://codereview.chromium.org/2581243004/diff/20001/content/renderer/input/...
> File content/renderer/input/main_thread_event_queue.cc (right):
> 
>
https://codereview.chromium.org/2581243004/diff/20001/content/renderer/input/...
> content/renderer/input/main_thread_event_queue.cc:85: std::string threshold =
> variations::GetVariationParamValueByFeature(
> Are you sure this works? The comment here says this only works for the browser
> process at the moment:
> 
>
https://cs.chromium.org/chromium/src/components/variations/variations_associa...
> 
> We had to introduce a new WebSetting[1] for something similar because of this.
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSetting...

Yeah, I'd be surprised if this works. Currently variations data/features are
communicated to renderers through command line args, and we don't do include
parameters because it's too much. We are working on a shared memory based
solution, but I wasn't aware it was ready to go yet, and I'm fairly sure it's
not all platforms yet.

[tdresser, 2016-12-19 17:49:40]

Thanks folks. What's the best way to test passing parameters to the Renderer? Do
I need to set up a local finch instance?

[jwd, 2016-12-19 19:46:56]

On 2016/12/19 17:49:40, tdresser wrote:
> Thanks folks. What's the best way to test passing parameters to the Renderer?
Do
> I need to set up a local finch instance?

Yup, that would work, you can find info at go/chrome-metrics in the Testing in
Chromium section.

To get around the parameter thing, people have encoded data in the group name,
then just queried the active group for a trial, and parsed it.

[rkaplow, 2017-01-09 21:50:15]

rkaplow@chromium.org changed reviewers:
- rkaplow@chromium.org

[tdresser, 2017-02-08 17:06:47]

Description was changed from

==========
Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:

--enable-features="MainThreadBusyScrollIntervention<Foo" \
  --force-fieldtrials=Foo/Group1 \
  --force-fieldtrial-params=Foo.Group1:threshold/200

BUG=599609
TEST=RendererSchedulerImplTest, manual testing of passing via commandline.
==========

to

==========
Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:
--enable-features="MainThreadBusyScrollIntervention"
--force-fieldtrials=MainThreadResponsivenessScrollIntervention/Enabled150

BUG=599609
TEST=MainThreadEventQueueInitializationTest, RendererSchedulerImplTest, manual
testing of passing via commandline.
==========

[tdresser, 2017-02-08 17:07:27]

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

[commit-bot: I haz the power, 2017-02-08 17:07:47]

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

[tdresser, 2017-02-08 17:57:50]

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

[commit-bot: I haz the power, 2017-02-08 17:58:23]

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

[tdresser, 2017-02-08 17:58:51]

tdresser@chromium.org changed reviewers:
- jwd@chromium.org

[tdresser, 2017-02-08 17:58:52]

Sorry for the delay here.

Jochen, PTAL.

-jwd, as there's no variations dependency anymore.

https://codereview.chromium.org/2581243004/diff/20001/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/2581243004/diff/20001/content/renderer/BUILD....
content/renderer/BUILD.gn:407: "//components/variations",
On 2016/12/19 11:52:42, jochen wrote:
> variations depend on prefs, but we don't have prefs available in the renderer,
> right?
> 
> if there's a part of variations that is independend of prefs, can we factor
that
> out, so the renderer only depends on that?

I've removed this dependency.

[commit-bot: I haz the power, 2017-02-08 19:48:44]

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

[commit-bot: I haz the power, 2017-02-08 19:48:45]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-02-09 13:18:39]

lgtm

[tdresser, 2017-02-09 13:20:15]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2017-02-09 13:20:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2581243004/#ps80001
(title: "Use EXPECT_FALSE/TRUE - avoids compile error.")

[commit-bot: I haz the power, 2017-02-09 13:20:27]

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, 2017-02-09 13:24:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486646415133720,
"parent_rev": "f5e5811f11d2d7b8d232f07dd5ac0bdbe6822ddf", "commit_rev":
"0c4dc23c2c1a24df3f57347c0765b4872730c632"}

[commit-bot: I haz the power, 2017-02-09 13:25:34]

Description was changed from

==========
Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:
--enable-features="MainThreadBusyScrollIntervention"
--force-fieldtrials=MainThreadResponsivenessScrollIntervention/Enabled150

BUG=599609
TEST=MainThreadEventQueueInitializationTest, RendererSchedulerImplTest, manual
testing of passing via commandline.
==========

to

==========
Pass main thread responsiveness threshold via Finch

This will allow us to tune when a page is considered unresponsive
for the main thread responsiveness intervention.

To test, use:
--enable-features="MainThreadBusyScrollIntervention"
--force-fieldtrials=MainThreadResponsivenessScrollIntervention/Enabled150

BUG=599609
TEST=MainThreadEventQueueInitializationTest, RendererSchedulerImplTest, manual
testing of passing via commandline.

Review-Url: https://codereview.chromium.org/2581243004
Cr-Commit-Position: refs/heads/master@{#449276}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c4dc23c2c1a24df3f57347c0765...
==========

[commit-bot: I haz the power, 2017-02-09 13:25:35]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0c4dc23c2c1a24df3f57347c0765...