Measure Stylo memory usage using AWSY tests

RESOLVED FIXED in Firefox 57

Status

()

P5
normal
RESOLVED FIXED
a year ago
5 days ago

People

(Reporter: cpeterson, Assigned: bc)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57 fixed)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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]

Updated

a year ago
Whiteboard: [MemShrink] → [MemShrink:P1]

Comment 1

a year ago
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)
(Reporter)

Comment 2

a year ago
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.

Comment 3

a year ago
(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.

Comment 4

a year ago
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
(Assignee)

Comment 6

a year ago
You want more than just Linux, right? I have an idea on how to proceed. Give me till tomorrow to work it out.

Comment 7

a year ago
(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
(Assignee)

Comment 8

a year ago
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)
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
Created attachment 8890919 [details] [diff] [review]
bug-1378526-awsy-stylo.patch
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
(Assignee)

Comment 16

a year ago
(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?
(Assignee)

Updated

a year ago
See Also: → bug 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!
(Assignee)

Comment 18

a year ago
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)
(Assignee)

Updated

a year ago
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.
(Assignee)

Comment 24

a year ago
Created attachment 8892293 [details] [diff] [review]
bug-1378526-awsy-stylo-v3.patch

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+

Comment 27

a year ago
Pushed by bclary@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa83b1463e2b
Measure Stylo memory usage using AWSY tests, r=erahm, kmoir.

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa83b1463e2b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 29

a year ago
https://hg.mozilla.org/projects/date/rev/fa83b1463e2b061ed6ca9a92faf61776f1376df7
Bug 1378526 - Measure Stylo memory usage using AWSY tests, r=erahm, kmoir.
Too late for 56. Mass won't fix for 56.
status-firefox56: affected → wontfix

Updated

5 days ago
See Also: → bug 1500512
You need to log in before you can comment on or make changes to this bug.