Closed
Bug 1376546
Opened 7 years ago
Closed 7 years ago
Add instrumentation to browser-chrome tests to measure the usage of XUL, XBL, HTML and SVG
Categories
(Testing :: Mochitest, enhancement, P5)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(4 files, 1 obsolete file)
No description provided.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Comment on attachment 8883038 [details] Bug 1376546: Add instrumentation to browser-chrome tests to output the total set of elements in use in tests. try: -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc -t none Probably more of a feedback pass here as I don't think we need to check this in, mainly just to get a second set of eyes on the code so we can be a bit more confident in the numbers that it generates. The changes to browser-test cause us to upload an xulinstrument.json file from each browser-chrome test run. xulcount.js is a node script that takes as arguments any number of the xulinstrument.json files and combines them to generate the stats we're interested in.
Attachment #8883038 -
Flags: review?(bgrinstead) → feedback?(bgrinstead)
Assignee | ||
Comment 3•7 years ago
|
||
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Blocks: war-on-xbl
Comment 5•7 years ago
|
||
I made a version that makes it easier to run since it forces all the b-c tests into one chunk. Leads to a push like https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e9ea150889251328677fd9bb7f2f4c0c02c9f63 so you can grab a single xulintrument.txt like https://public-artifacts.taskcluster.net/JWUqJETHRW6Wpb3yipO0Ww/0/public/test_info//xulinstrument.txt. Trying to get a version that does the xulcount script in browser-test so we can have an artifact that shows the summary directly. The identityPopupContentHosts error is still there - did you apply a fix locally to work around this or has there always been that failure?
Flags: needinfo?(dtownsend)
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
See the change at the top of browser-test.js where it opens a new tab and clicks on the identity box. That was fixing it for me the last time I rand numbers.
Flags: needinfo?(dtownsend)
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=361bc9f3f5cc759d15f379dcfc29033cb271ffc8
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8883038 [details] Bug 1376546: Add instrumentation to browser-chrome tests to output the total set of elements in use in tests. try: -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc -t none https://reviewboard.mozilla.org/r/153704/#review189904 Clearing review - I've attached an updated version to the bug
Attachment #8883038 -
Flags: review?(bgrinstead)
Updated•7 years ago
|
Attachment #8883038 -
Attachment is obsolete: true
Comment 12•7 years ago
|
||
I don't think we need to land this, but will use it to keep track of usage over time
Priority: -- → P5
Comment 13•7 years ago
|
||
The latest version is saving out a separate xulsummary.txt in browser-test.js so we don't need a separate node script to analyze the data. Leads to output like https://public-artifacts.taskcluster.net/TknE8AbZQd6AMvWV3j8h9A/0/public/test_info//xulsummary.txt from https://treeherder.mozilla.org/#/jobs?repo=try&revision=361bc9f3f5cc759d15f379dcfc29033cb271ffc8&selectedJob=133861399
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
After chatting with Matt, I think we could make it a lot easier to collect this data over time like so: * Land the change to browser-test.js but behind an environment variable so that this doesn't run in normal mochitest-browser runs * Add a new job in mochitest.yml similar to "mochitest-browser-chrome" that runs all the b-c tests in one chunk and make it tier 3 * Add an entry in .cron.yml that causes the new job to run at specified times * Use the taskcluster API to fetch the latest runs with the new job to gather the artifact that has the relevant numbers, using code similar to https://github.com/mnoorenberghe/mozscreenshots/blob/bcedcf74221cd681a219c7aec68af05c3840dfbf/mozscreenshots/fetch_screenshots.py#L130
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
Joel, I'm not sure if I'm doing this right, so please be critical. I'm trying to make a job that I can run periodically (a couple of times a month) as a way to collect some data about how much and what kind of DOM we create. I don't want it to run when mochitests are requested (i.e. `-u mochitests`), only when I manually trigger it. Sidenote: if there's a way to automatically run this on an interval (i.e. the 1st and 15th of every month), that'd be even better.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
Oh also: this test run is perma-orange, but it still works for what we need it for. I suspect that traversing the DOM is somehow changing the XBL construction ordering that breaks a few tests. So we may need to flag this as tier 3 (although as long as it doesn't run with "mochitests" or on m-c it may not matter).
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8924401 [details] Bug 1376546 - Set up browser instrumentation as a new taskcluster job; https://reviewboard.mozilla.org/r/195568/#review201158 I am not clear how you actually are doing a different style of browser-chrome run. As for your question about running periodically- there is the ability to run a job on a schedule, using taskcluster:cron: http://searchfox.org/mozilla-central/source/.cron.yml that will run daily (probably on m-c) and if you make it tier-3 or hide the job in treeherder, you can get data from it regularly. For something that runs less frequently, you will probably have to push to try manually. In your code make sure to mark it as tier-3 ::: taskcluster/ci/test/mochitest.yml:131 (Diff revision 2) > + treeherder-symbol: tc-M(inst) > + loopback-video: true > + max-run-time: 14400 > + mozharness: > + mochitest-flavor: browser > + allow-software-gl-layers: false add a tier section here: http://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest.yml#53 you might want to put run-on-projects to include 'try': http://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest.yml#75 if you ignore run-on-projects, then you specifically have to select the job in try syntax to run, that might be what you want to do for now. ::: testing/mozharness/configs/unittests/linux_unittest.py:202 (Diff revision 2) > "browser-chrome-clipboard": ["--flavor=browser", "--subsuite=clipboard"], > "browser-chrome-chunked": ["--flavor=browser", "--chunk-by-runtime"], > "browser-chrome-addons": ["--flavor=browser", "--chunk-by-runtime", "--tag=addons"], > "browser-chrome-coverage": ["--flavor=browser", "--chunk-by-runtime", "--timeout=1200"], > "browser-chrome-screenshots": ["--flavor=browser", "--subsuite=screenshots"], > + "browser-chrome-instrumentation": ["--flavor=browser"], I would expect an extra flag like --instrumentation, or something like that. ::: testing/mozharness/scripts/desktop_unittest.py:706 (Diff revision 2) > 'abs_res_dir': abs_res_dir, > } > options_list = [] > - env = {} > + env = { > + 'TEST_SUITE': suite > + } I don't understand this specific change? is this how you trigger the instrumented test run?
Attachment #8924401 -
Flags: review?(jmaher) → review-
Comment 24•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #23) > Comment on attachment 8924401 [details] > Bug 1376546 - Set up browser instrumentation as a new taskcluster job; > > https://reviewboard.mozilla.org/r/195568/#review201158 > > I am not clear how you actually are doing a different style of > browser-chrome run. > > ::: testing/mozharness/configs/unittests/linux_unittest.py:202 > (Diff revision 2) > > "browser-chrome-clipboard": ["--flavor=browser", "--subsuite=clipboard"], > > "browser-chrome-chunked": ["--flavor=browser", "--chunk-by-runtime"], > > "browser-chrome-addons": ["--flavor=browser", "--chunk-by-runtime", "--tag=addons"], > > "browser-chrome-coverage": ["--flavor=browser", "--chunk-by-runtime", "--timeout=1200"], > > "browser-chrome-screenshots": ["--flavor=browser", "--subsuite=screenshots"], > > + "browser-chrome-instrumentation": ["--flavor=browser"], > > I would expect an extra flag like --instrumentation, or something like that. The reason there's not extra flag here is I want to run the whole b-c suite in a single chunk, not a different set of tests (see below). > ::: testing/mozharness/scripts/desktop_unittest.py:706 > (Diff revision 2) > > 'abs_res_dir': abs_res_dir, > > } > > options_list = [] > > - env = {} > > + env = { > > + 'TEST_SUITE': suite > > + } > > I don't understand this specific change? is this how you trigger the > instrumented test run? Yes, it's used in the first patch in this series as a way to toggle the behavior in browser-test.js if `ENV["TEST_SUITE"] == "browser-chrome-instrumentation"`
Comment 25•7 years ago
|
||
ni? to see if there's a better way to do what I want based on response in Comment 24
Flags: needinfo?(jmaher)
Comment 26•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #24) > (In reply to Joel Maher ( :jmaher) (UTC-5) from comment #23) > > I would expect an extra flag like --instrumentation, or something like that. > > The reason there's not extra flag here is I want to run the whole b-c suite > in a single chunk, not a different set of tests (see below). IOW what I want is to run the entire b-c suite in one chunk with some environment variable set that lets us know to do extra instrumentation.
Comment 27•7 years ago
|
||
as a note, this will take a long time, on debug we run 4.5-5 hours for runtime of all linux64 debug browser-chrome tests, we will need to modify taskcluster/mozharness to let the job run that long. For linux64 opt it is 3.5 hours. it is reasonable to toggle features with an environment variable- other options are a commandline flag that sets a pref or different url param on the in-browser harness. If we need to set the ENV variable to make the browser do something in the backend, then this approach is fine. I am concerned with running this in a single chunk, is there a need for that vs running it in the existing 7 or 8 chunks?
Flags: needinfo?(jmaher)
Comment 28•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #27) > as a note, this will take a long time, on debug we run 4.5-5 hours for > runtime of all linux64 debug browser-chrome tests, we will need to modify > taskcluster/mozharness to let the job run that long. For linux64 opt it is > 3.5 hours. I don't need this to run in debug builds, and we could pick a single platform to run this on instead of all 3 (whichever is fastest). It looks like on Windows 10 it's 103 minutes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34a73cbe0dfd89c76bca8e58895393e51d351001&selectedJob=141451689 > it is reasonable to toggle features with an environment variable- other > options are a commandline flag that sets a pref or different url param on > the in-browser harness. If we need to set the ENV variable to make the > browser do something in the backend, then this approach is fine. Yeah, that's exactly why we need this env variable - and I didn't see another one that looked like it would work. > I am concerned with running this in a single chunk, is there a need for that vs > running it in the existing 7 or 8 chunks? Yes, otherwise we need to keep state between the chunks to make sure we don't count duplicate elements (which is possible but it requires extra work to process the results and make them usable). An alternative is to not land this patch and have me apply it and push to try every so often. I'd prefer to land it so that I can 'add new jobs' to existing builds instead of needing to remember to do the push on certain days - but it wouldn't be too bad either way.
Comment 29•7 years ago
|
||
ok, lets target windows10-opt and make sure it can run on try and mozilla-central- on demand only though. How about you verify it doesn't run on try with a default |./mach try -b o -p all -u all -t none| push, but does show up as tier-3 on try with |./mach try -b o -p win64 -u all -t none|. Once this is proven to do that, we can land it and sort out the options for tweaking it to be m-c only.
Comment 30•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #29) > ok, lets target windows10-opt and make sure it can run on try and > mozilla-central- on demand only though. How about you verify it doesn't run > on try with a default |./mach try -b o -p all -u all -t none| push, but does > show up as tier-3 on try with |./mach try -b o -p win64 -u all -t none|. > > Once this is proven to do that, we can land it and sort out the options for > tweaking it to be m-c only. Alright, here are 3 pushes: 1) `./mach try fuzzy and search` for 'instrumentation' (shows 'test-windows10-64/opt-browser-instrumentation-e10s'): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ff52d09f20607c185221f3f9a6fd4c888926259&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded 2) `./mach try -b o -p all -u all -t none`: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a52391f64c712407e392cea62f26e2b1675f9b7b&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded 3) `./mach try -b o -p win64 -u all -t none`: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1242d6cd1971775da51aa62b619d61a367b438d&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded (1) *does* run the instrumentation job (as expected) and (2)/(3) do not. In your comment you make it sound as if it should be running in (3), but I don't think it should be - it should only run when you manually specify the test suite, right?
Flags: needinfo?(jmaher)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•7 years ago
|
||
Published the push from Comment 30 to mozreview. Do you think I should revert the changes to these? testing/mozharness/configs/unittests/linux_unittest.py testing/mozharness/configs/unittests/mac_unittest.py testing/mozharness/configs/unittests/win_unittest.py OR testing/mozharness/configs/unittests/win_taskcluster_unittest.py (not sure which is needed for win10-opt)
Comment 34•7 years ago
|
||
I like the idea of it only running in mach try fuzzy; this seems like the best option!
Flags: needinfo?(jmaher)
Comment 35•7 years ago
|
||
It also allows for "Add New Jobs" on Win10 opt builds (tier 3), so I can trigger it manually every so often on m-c builds to get the data
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8924401 [details] Bug 1376546 - Set up browser instrumentation as a new taskcluster job; https://reviewboard.mozilla.org/r/195568/#review202370 overall this seems good- I think we need the <platform>_unittest.py changes as well.
Attachment #8924401 -
Flags: review?(jmaher) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8912927 [details] Bug 1376546 - Add instrumentation to browser-chrome tests to output the total set of elements in use in tests; https://reviewboard.mozilla.org/r/184238/#review202552
Attachment #8912927 -
Flags: review?(bgrinstead) → review+
Comment 41•7 years ago
|
||
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d1bca2413eb Add instrumentation to browser-chrome tests to output the total set of elements in use in tests;r=bgrins https://hg.mozilla.org/integration/autoland/rev/088804dba005 Set up browser instrumentation as a new taskcluster job;r=jmaher
Comment 42•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2d1bca2413eb https://hg.mozilla.org/mozilla-central/rev/088804dba005
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 43•7 years ago
|
||
this seems to be running on inbound, central, autoland- could we fix this to run on try and central only?
Comment 44•7 years ago
|
||
:bgrins, I see we wanted to land this, then figure out where it ran- I believe we need to adjust this to only have [mozilla-central, try] in the config: https://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest.yml#123 similar to: https://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest.yml#76
Flags: needinfo?(bgrinstead)
Comment 45•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #44) > :bgrins, I see we wanted to land this, then figure out where it ran- I > believe we need to adjust this to only have [mozilla-central, try] in the > config: > https://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest. > yml#123 > > similar to: > https://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest. > yml#76 After discussing this, we decided to have it run only on nightlies in Bug 1417676
Flags: needinfo?(bgrinstead)
Updated•7 years ago
|
Component: BrowserTest → Mochitest
You need to log in
before you can comment on or make changes to this bug.
Description
•