Closed Bug 1383146 Opened 6 years ago Closed 6 years ago

add new talos test to q1s that measures STYLO_THREADS=1


(Testing :: Talos, enhancement)

Not set


(firefox56 fixed)

Tracking Status
firefox56 --- fixed


(Reporter: jmaher, Assigned: jmaher)


(Blocks 1 open bug)


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


(3 files, 1 obsolete file)

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?
: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]
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)
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/
@@ +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"],
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]:

Attachment #8890117 - Flags: review?(rwood) → review+
Attached patch support --stylo-threads in talos (obsolete) — Splinter Review
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:
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/
@@ +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/
@@ +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/
@@ +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-
thanks for the issues yesterday!
Attachment #8890475 - Attachment is obsolete: true
Attachment #8890871 - Flags: review?(rwood)
Attachment #8890871 - Flags: review?(rwood) → review+
Pushed by
add new talos test to q1s that measures STYLO_THREADS=1. r=rwood
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.