add new talos test to q1s that measures STYLO_THREADS=1

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [PI:July] [stylo])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
currently we run quantum pageload tests on regular firefox.

As of bug 1382524, we started running with stylo turned on.

We want a 3rd data point to expand on just turning stylo on and adding the environment variable STYLO_THREADS=1.


We could save some headaches and avoid the buildbot changes by running this in the same q1s job.

We would need to:
* modify talos to define the new test quantum-pageload-stylo-threads
* add a cli arg or a hack to set the STYLO_THREADS=1 environment variable
* modify testing/talos/talos.json to run this test as well
* modify taskcluster/ci/tests.yml to have a definition for this new test (I think just a reference to it is needed)
* ensure we report something useful into perfherder

Right now perfherder sees "quantum pageload opt e10s stylo", adding "threads" to the end will get long, is there a better way to describe the options to perfherder?
(Assignee)

Comment 1

a year ago
:rwood, any further thoughts here, things I might have missed, etc.
Flags: needinfo?(rwood)
The configs are a huge pain but I think it might be best to keep it separate, and have a new symbol 'q1st'. Just makes it more flexible in case we want to easily turn it off, only run it on specific platforms in the future, re-triggering only that test when investigating regressions, have it included in SETA, etc.
Flags: needinfo?(rwood)
See Bug 1383737 regarding test &  job names
Depends on: 1383737
Whiteboard: [PI:July] → [PI:July] [stylo]
(Assignee)

Comment 4

a year ago
Created attachment 8890117 [details] [diff] [review]
buildbot-config changes for tp6

this patch will:
1) add tp6-stylo-threads-e10s to be scheduled by default (note the new name)
2) add tp6-e10s and tp6-stylo-e10s as options for try (until we get that right and switch quantum-pageload off)
Assignee: nobody → jmaher
Attachment #8890117 - Flags: review?(rwood)
(Assignee)

Comment 5

a year ago
Created attachment 8890118 [details]
buildbot_configs diff: tp6_diff.txt

these are the new jobs!
Comment on attachment 8890117 [details] [diff] [review]
buildbot-config changes for tp6

Review of attachment 8890117 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla-tests/config.py
@@ +334,5 @@
> +        'options': ({}, WIN_ONLY),
> +    },
> +    'tp6-stylo-threads-e10s': {
> +        'enable_by_default': True,
> +        'suites': GRAPH_CONFIG + ['--activeTests', 'Quantum_1', '--filter', 'ignore_first:5', '--filter', 'median', '--stylo-threads'],

I just realized, how is this even working with 'Quantum_1'? That test doesn't even exist, now we use

"tests": ["quantum_pageload_google", "quantum_pageload_youtube", "quantum_pageload_amazon", "quantum_pageload_facebook"],
(Assignee)

Comment 7

a year ago
inside of buildbot these are just placeholders, the real definition is in-tree in the testing/talos/talos.json file.
Comment on attachment 8890117 [details] [diff] [review]
buildbot-config changes for tp6

Review of attachment 8890117 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, LGTM
Attachment #8890117 - Flags: review?(rwood) → review+
(Assignee)

Comment 9

a year ago
Created attachment 8890475 [details] [diff] [review]
support --stylo-threads in talos

this does a few things:
* add tp6 names for tests (the future direction)
* adds the ability for specifying a specific amount of threads via the cli variable --stylo-threads=X
* will report as tp6_<page> X_threads e10s stylo

I left this as a secondary option to require --stylo as well- not sure if we should change that, I view this as a temporary (<6 month) test.

here is a try push and I hacked up q1s to have threads:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44f8490f11f680f02c7614e72e2a3488fa6dfee
Attachment #8890475 - Flags: review?(rwood)
Comment on attachment 8890475 [details] [diff] [review]
support --stylo-threads in talos

Review of attachment 8890475 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/talos/talos/cmdline.py
@@ +173,5 @@
>              help='If given, enable Stylo via Environment variables and '
>                   'upload results with Stylo options.')
> +    add_arg('--stylo-threads', type=int,
> +            dest='stylothreads',
> +            help='If given, Run stylo with a certain number of threads')

nit: don't need capital "Run"

::: testing/talos/talos/run_tests.py
@@ +192,5 @@
>      if config['stylo']:
>          talos_results.add_extra_option('stylo')
>  
> +    # measuring the difference of a a certain thread level
> +    if 'stylothreads' in config and config['stylothreads'] > 0:

nit, bit shorter:

if config.get('stylothreads', 0) > 0

::: testing/talos/talos/ttest.py
@@ +97,5 @@
>              setup.env['STYLO_FORCE_ENABLED'] = 1
>  
> +        # During the Stylo transition, measure different number of threads
> +        if browser_config['stylothreads']:
> +            setup.env['STYLO_THREADS'] = 1

should be set to the 'stylothreads' value, not hardcoded to 1
Comment on attachment 8890475 [details] [diff] [review]
support --stylo-threads in talos

Looks awesome, one issue and couple of nits (see Comment 10)
Attachment #8890475 - Flags: review?(rwood) → review-
(Assignee)

Comment 12

a year ago
Created attachment 8890871 [details] [diff] [review]
support --stylo-threads in talos

thanks for the issues yesterday!
Attachment #8890475 - Attachment is obsolete: true
Attachment #8890871 - Flags: review?(rwood)

Updated

a year ago
Attachment #8890871 - Flags: review?(rwood) → review+

Comment 13

a year ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29efb0ac89cf
add new talos test to q1s that measures STYLO_THREADS=1. r=rwood

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29efb0ac89cf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 16

a year ago
all the plumbing is in place, we just need a small patch to turn this on.
You need to log in before you can comment on or make changes to this bug.