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)

Product:
Testing
Component:
Mochitest
Version:
Version 3
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(4 files, 1 obsolete file)

Latest results
11.49 KB, text/plain
Details
Bug 1376546 - Add instrumentation to browser-chrome tests to output the total set of elements in use in tests;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
results-linux-opt-2017-09-27.txt
11.05 KB, text/plain
Details
Bug 1376546 - Set up browser instrumentation as a new taskcluster job;
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
      No description provided.
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)
Attached file Latest results — 

    
    

    
      
  

    
    

    
  
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)

    
    

    
  
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 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)

        Attachment #8883038 -
        Attachment is obsolete: true

    
    

    
  
I don't think we need to land this, but will use it to keep track of usage over time
Priority: -- → P5

    
    

    
  
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

    
    

    
      
  

    
    

    
  
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

    
    

    
      
  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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 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-

    
    

    
  
(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"`

    
    

    
  
ni? to see if there's a better way to do what I want based on response in Comment 24
Flags: needinfo?(jmaher)

    
    

    
  
(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.

    
    

    
  
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)

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(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)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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)

    
    

    
  
I like the idea of it only running in mach try fuzzy; this seems like the best option!
Flags: needinfo?(jmaher)

    
    

    
  
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 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+

    
    

    
  
Syntax for running on try: `./mach try fuzzy -q "browser-instrumentation"`

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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+

    
    

    
  
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

    
    

    
  
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

    
    

    
  
this seems to be running on inbound, central, autoland- could we fix this to run on try and central only?

    
    

    
  
: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)
Depends on: 1417676

    
    

    
  
(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)
Depends on: 1426509
Component: BrowserTest → Mochitest

    
  
Depends on: 1443328

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: