Closed Bug 1378526 Opened 7 years ago Closed 7 years ago

Measure Stylo memory usage using AWSY tests

Categories

(Core :: CSS Parsing and Computation, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: bc)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 file, 1 obsolete file)

Stylo support is currently built, but pref'd off, in Nightly builds for Linux64, Win32, and Win64. We'd like to watch for memory usage regressions caused by Stylo.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]
Bob where'd we end up on this? I think you were going to do some try runs, but we also talked about having kmoir help out.
Flags: needinfo?(bob)
Stylo support is currently built (pref'd off) in regular Nightly builds for Linux64, Win32, Win64, and Mac. Stylo can be enabled by a test runner by setting the environment variable STYLO_FORCE_ENABLE=1.
(In reply to Chris Peterson [:cpeterson] from comment #2)
> Stylo support is currently built (pref'd off) in regular Nightly builds for
> Linux64, Win32, Win64, and Mac. Stylo can be enabled by a test runner by
> setting the environment variable STYLO_FORCE_ENABLE=1.

I assumed we'd just add awsy to list of tests for the 'Linux 64 Stylo Opt' platform and then when other OS's are supported we'd enable them as well.
So probably in test-platforms.yml [1]. I'm not sure if we'd need to make any other changes than that.

[1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/taskcluster/ci/test/test-platforms.yml#92-96
You want more than just Linux, right? I have an idea on how to proceed. Give me till tomorrow to work it out.
(In reply to Bob Clary [:bc:] from comment #6)
> You want more than just Linux, right? I have an idea on how to proceed. Give
> me till tomorrow to work it out.

Sounds good. The try run failed due to awsy_script.py choking on the '--enable-stylo' arg. I guess we can just copy and paste what other scripts do [1].

[1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/testing/mozharness/scripts/web_platform_tests.py#65-76
Test run here:
https://treeherder.allizom.org/#/jobs?repo=try&author=bclary@mozilla.com&fromchange=0eb78f6b5c8b8b8828ef724713bebf86cb6911d3

It doesn't use the 'stylo' platform but runs on regular builds and hopefully sets the environment variable to enable stylo. They are still running and I haven't checked the results yet. I'll take a look later when it's finished.
Flags: needinfo?(bob)
Looks like it actually worked:
https://treeherder.allizom.org/logviewer.html#?job_id=111938110&repo=try&lineNumber=1284
[task 2017-07-27T14:45:37.677546Z] 14:45:37     INFO -  'STYLO_FORCE_ENABLED': '1',

I'll submit the patch for review/feedback.
Attached patch bug-1378526-awsy-stylo.patch (obsolete) — Splinter Review
Attachment #8890919 - Flags: review?(kmoir)
Attachment #8890919 - Flags: review?(erahm)
Comment on attachment 8890919 [details] [diff] [review]
bug-1378526-awsy-stylo.patch

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

Overall looks good, but the perf data needs to be attributed to new platforms -- linux64-stylo not linux64 -- so that we can compare against our baseline. I'm not sure if there's a reasonable way to do that.

I wonder if we'd be better off defining stylo platforms for win and osx that just run awsy?

::: taskcluster/ci/test/tests.yml
@@ +44,5 @@
> +                    - awsy/macosx_config.py
> +                default:
> +                    - awsy/linux_config.py
> +        extra-options:
> +            - --enable-stylo

Nice, that's super simple.

::: testing/mozharness/scripts/awsy_script.py
@@ +40,5 @@
> +        [["--enable-stylo"],
> +        {"action": "store_true",
> +         "dest": "enable_stylo",
> +         "default": False,
> +         "help": "Run tests with Stylo enabled.",

Should we also support sequential? TBH I'm not sure I understand the difference.
Attachment #8890919 - Flags: review?(erahm) → feedback+
Will, do you know a way of "faking" the platform attribution for a PERFHERDER_DATA blob? In this case we want to run awsy with stylo enabled and have it attributed to <platform>-awsy so that we don't mix data with non-stylo runs.
Flags: needinfo?(wlachance)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #13)
> Will, do you know a way of "faking" the platform attribution for a
> PERFHERDER_DATA blob? In this case we want to run awsy with stylo enabled
> and have it attributed to <platform>-awsy so that we don't mix data with
> non-stylo runs.

Can you not specify 'stylo' as an extraOption in the perfherder json submitted, much as we did/do for e10s and talos?

https://github.com/mozilla/treeherder/blob/master/schemas/performance-artifact.json#L81
Flags: needinfo?(wlachance)
(In reply to William Lachance (:wlach) (use needinfo!) from comment #14)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #13)
> > Will, do you know a way of "faking" the platform attribution for a
> > PERFHERDER_DATA blob? In this case we want to run awsy with stylo enabled
> > and have it attributed to <platform>-awsy so that we don't mix data with
> > non-stylo runs.
> 
> Can you not specify 'stylo' as an extraOption in the perfherder json
> submitted, much as we did/do for e10s and talos?
> 
> https://github.com/mozilla/treeherder/blob/master/schemas/performance-
> artifact.json#L81

Perfect, so that's just updating our perf script [1] to include extraOption. I guess we'd have to pipe that through.

[1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/testing/awsy/awsy/process_perf_data.py#62-67
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #12)
> 
> Should we also support sequential? TBH I'm not sure I understand the
> difference.

bug 1318187 comment 2

sequential STYLO_THREADS=1

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #15)
> 
> Perfect, so that's just updating our perf script [1] to include extraOption.
> I guess we'd have to pipe that through.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> ad093e98f42338effe2e2513e26c3a311dd96422/testing/awsy/awsy/process_perf_data.
> py#62-67

So we would basically add "extraOptions": ["stylo"],

That would mean I can keep the platform assignments as they are?
See Also: → 1385027
(In reply to Bob Clary [:bc:] from comment #16)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #12)
> > 
> > Should we also support sequential? TBH I'm not sure I understand the
> > difference.
> 
> bug 1318187 comment 2
> 
> sequential STYLO_THREADS=1
> 
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #15)
> > 
> > Perfect, so that's just updating our perf script [1] to include extraOption.
> > I guess we'd have to pipe that through.
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > ad093e98f42338effe2e2513e26c3a311dd96422/testing/awsy/awsy/process_perf_data.
> > py#62-67
> 
> So we would basically add "extraOptions": ["stylo"],
> 
> That would mean I can keep the platform assignments as they are?

Yep, no need to add new style platforms!
Kim, do you have any suggestions on how to handle the platform issues? If adding stylo to  the extraOptions will give erham the ability to compare non-stylo vs. stylo even on the same platform, it seems that the approach I took to add to the existing platforms would work ok.
Flags: needinfo?(kmoir)
Attachment #8890919 - Flags: review?(kmoir)
regarding comment 12, 

sequential tests just don't have this added
test['mozharness'].setdefault('extra-options', [])\
                             .append('--parallel-stylo-traversal')

with respect to the other question in comment 18

See how this was done in bug 1374748,

https://hg.mozilla.org/integration/autoland/rev/6c77cacaf82d
Flags: needinfo?(kmoir)
It sounded like bholly would like both parallel and sequential so we should add that.

The not terribly important debate here is whether we should just add AWSY to the list of tests run on the stylo(-sequential) test platforms (so only run on Linux for now until Windows and Mac are added) or if we should just add a new test type 'awsy-e10s-stylo' and 'awsy-e10s-stylo-sequential' that we add to "normal" test platforms (linux64, win, osx, etc).

I was inclined to just add it to the stylo test platforms so that we only start testing when those platforms are deemed ready (so someone from the stylo team adds them) and they get turned off properly when we eventually turn those test platforms off. I don't care too much and nobody has chimed in saying Bob's plan is a bad idea so lets just do that.
(In reply to Bob Clary [:bc:] from comment #19)
> with extraOptions:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a2ae385d1a96f26c3f6f1be76999787e0f1da81d&exclusion_pro
> file=false&group_state=expanded&filter-searchStr=sy
> 
> erahm: How does this look?

This looks good, we just need to add support for sequential as well.
Sorry for the churn, given bug 1380053 (stylo tests on mac) just landed and bug 1385027 (stylo tests on win) is pending until they fix some things I'd say just add awsy to those platforms. Enabling and crashing a bunch on windows would probably make the sheriffs sad.

They also seem to have deemed sequential only worth testing on Linux.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c52319b670c4ea0a0115ea486762431af61a371&exclusion_profile=false

Still waiting on OSX...
Assignee: nobody → bob
Attachment #8890919 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8892293 - Flags: review?(kmoir)
Attachment #8892293 - Flags: review?(erahm)
Comment on attachment 8892293 [details] [diff] [review]
bug-1378526-awsy-stylo-v3.patch

The osx results are delayed due to bug 1386264.
Attachment #8892293 - Flags: review?(kmoir) → review+
Comment on attachment 8892293 [details] [diff] [review]
bug-1378526-awsy-stylo-v3.patch

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

Looks good, thanks Bob!
Attachment #8892293 - Flags: review?(erahm) → review+
Pushed by bclary@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa83b1463e2b
Measure Stylo memory usage using AWSY tests, r=erahm, kmoir.
https://hg.mozilla.org/mozilla-central/rev/fa83b1463e2b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Too late for 56. Mass won't fix for 56.
See Also: → 1500512
You need to log in before you can comment on or make changes to this bug.