723571 - pageloader shouldnt calculate statistics

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

Pageloader calculates the "median" (well median of all besides the max
value), the mean, max, and min.  Really, pageloader should just output
the raw data and let upstream tools (talos, graphserver) do what they
want with the data.

Comment 1

[Jeff Hammel]

We will have to change the talos results parser at the same time, since currently we assume the median, mean, max, min for datapoints: http://hg.mozilla.org/build/talos/file/4f9e229d378e/talos/results.py#l7

Comment 2

[Jeff Hammel]

Currently pageloader calculates variance and standard deviation.  As far as I know these aren't used anywhere.  Can anyone confirm this?

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Jeff Hammel [:jhammel] from comment #2)
> Currently pageloader calculates variance and standard deviation.  As far as
> I know these aren't used anywhere.  Can anyone confirm this?

It's not exposed in the UI, although rniwa@webkit has very recently added support (we currently have it disabled).

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> (In reply to Jeff Hammel [:jhammel] from comment #2)
> > Currently pageloader calculates variance and standard deviation.  As far as
> > I know these aren't used anywhere.  Can anyone confirm this?
> 
> It's not exposed in the UI, although rniwa@webkit has very recently added
> support (we currently have it disabled).

I don't think graphserver has support for it currently anyway.

Comment 5

[Jeff Hammel]

Noticing an unused format, jsfull:

http://hg.mozilla.org/build/talos/file/51075a756118/talos/pageloader/chrome/report.js#l206

I guess I'll disable it since it doesn't actually do anything

Comment 6

[Jeff Hammel]

js format is pretty useless if we don't compute statistics:

http://hg.mozilla.org/build/talos/file/51075a756118/talos/pageloader/chrome/report.js#l196

Comment 7

[Jeff Hammel]

Attachment: remove pageloader statistics

Comment 8

[Jeff Hammel]

attachment 603855 [details] [diff] [review] tested locally on android with ts and tsvg

Comment 9

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a4a0f4f66462

Comment 10

[Mozilla RelEng Bot]

Try run for a4a0f4f66462 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a4a0f4f66462
Results (out of 73 total builds):
    success: 71
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a4a0f4f66462

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 603855 [details] [diff] [review]
remove pageloader statistics

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

overall this is good stuff!

::: talos/pageloader/chrome/report.js
@@ +95,5 @@
> +      if (left) {
> +        str = " " + str;
> +      } else {
> +        str += " ";
> +      }

why did you change this?  There are a lot of other if clauses without brackets.

::: tests/test_results.py
@@ +21,5 @@
> +|8;hixie-004.xml;5050;5054;5053;5054;5055
> +|9;hixie-005.xml;4568;4569;4562;4545;4567
> +|10;hixie-006.xml;5090;5165;5054;5015;5077
> +|11;hixie-007.xml;1628;1623;1623;1617;1622
> +"""

can we have tests that have 1 result, 2 results, 5 results, 20 results instead of the same 5?

Comment 12

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #11)
> Comment on attachment 603855 [details] [diff] [review]
> remove pageloader statistics
> 
> Review of attachment 603855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall this is good stuff!
> 
> ::: talos/pageloader/chrome/report.js
> @@ +95,5 @@
> > +      if (left) {
> > +        str = " " + str;
> > +      } else {
> > +        str += " ";
> > +      }
> 
> why did you change this?  There are a lot of other if clauses without
> brackets.

I mostly changed for the indentation which uses tabs instead of spaces (there are other cases of bad indentation in the file that I didn't clean up).  Since it has nothing to do with the change, i can revert this if desired.

> ::: tests/test_results.py
> @@ +21,5 @@
> > +|8;hixie-004.xml;5050;5054;5053;5054;5055
> > +|9;hixie-005.xml;4568;4569;4562;4545;4567
> > +|10;hixie-006.xml;5090;5165;5054;5015;5077
> > +|11;hixie-007.xml;1628;1623;1623;1617;1622
> > +"""
> 
> can we have tests that have 1 result, 2 results, 5 results, 20 results
> instead of the same 5?

We can, though I'd prefer to take that is a follow-up.  No one has written tests besides me and I'm guessing no one runs tests besides me.  The tests are mostly a smoke-screen and I would prefer to wait on adding more checks here until its easier to do so.

Comment 13

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/2968b025488d

Comment 14

[Jeff Hammel]

Forgot to add the test as part of the push; will do so now

Comment 15

[Jeff Hammel]

pushed follow up: http://hg.mozilla.org/build/talos/rev/3b4d65573036

Comment 16

[Jeff Hammel]

forgot this:

http://mxr.mozilla.org/build/source/talos/talos/run_tests.py#132

Comment 17

[Jeff Hammel]

I'm not sure what to do about the consolidated results: http://hg.mozilla.org/build/talos/rev/52063311813e

Do we need these in Talos?  More generally, do we need send_to_csv at all?

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

I believe send_to_csv is used for local runs of talos, now with results_url to a file, we don't have a need for this.  We do other things in this function like the amo results, so lets not lose track of that.

Comment 19

[Jeff Hammel]

Attachment: fix send_to_csv

fixes send_to_csv; we no longer calculate the average of modified medians for tpformat results, though

The whole result outputting should be cleaned up quite a bit; however, I'd prefer to do this after configuration gets a bit saner

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 605486 [details] [diff] [review]
fix send_to_csv

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

it will work for now, I still would like to remove this function.

Comment 21

[Jeff Hammel]

tested locally on android with tsvg and ts

Comment 22

[Mozilla RelEng Bot]

Try run for c3e8e3f833a3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c3e8e3f833a3
Results (out of 72 total builds):
    success: 70
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c3e8e3f833a3

Comment 23

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/b666faec6abe