1155121 - Talos output for sessionstore tests misses relevant parameters, includes bogus ones

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[:Gijs (he/him)]

From bug 1150656

(In reply to :Gijs Kruitbosch from comment #24)
> (In reply to Joel Maher (:jmaher) from comment #22)
> > session restore is not a tp test (agree that it is confusing), so instead of
> > --tpcycles, you want --cycles.
> > 
> > Let me know if that helps.  As for the difference in data you see, I am not
> > sure why that is the case :(  Maybe this is hardware dependent?
> 
> I will look at this again later today or tomorrow. Note that it is mostly
> confusing that IIRC, the actual test output point that says "10" is prefixed
> "tpcycles", not "cycles". I'll look at this again and file followup bugs
> against talos where appropriate.

I can confirm this does work to change the number of iterations, but then the output looks like e.g.:

INFO : TALOSDATA: [{"test_machine": {"platform": "x86_64", "osversion": "6.2.9200", "os": "win", "name": "qm-pxp01"}, "testrun": {"date": 1429179460, "suite": "sessionrestore", "options": {"responsiveness": false, "tpmozafterpaint": false, "tpchrome": true, "tppagecycles": 1, "tpcycles": 10, "tprender": false, "shutdown": false, "rss": false}}, "results": {"sessionrestore": [1526.0, 1440.0, 1426.0, 1450.0, 1425.0, 1446.0, 1437.0, 1449.0, 1427.0, 1440.0, 1453.0, 1455.0, 1430.0, 1435.0, 1432.0, 1451.0, 1433.0, 1439.0, 1443.0, 1442.0, 1441.0, 1437.0, 1441.0, 1448.0, 1447.0, 1435.0, 1439.0, 1440.0, 1438.0, 1439.0]}, "test_build": {"version": "40.0a1", "revision": "2f8060bb3f94", "id": "20150416095104", "branch": "", "name": "Firefox"}}]

Note in particular:

"tppagecycles": 1, "tpcycles": 10,

and 30 output values in "results" (because I passed --cycles 30).

I would expect it not to include "tpcycles" and include whatever number I set to 30 that's actually relevant.

Please feel free to adjust the summary as appropriate.

Comment 1

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

Attachment: be smarter about what we output (1.0)

this is a patch that solves this bug.  Although in solving this we will have issues with perfherder since it uses these options to create an options hash.  This will change that options hash and treat all new data as 'new' tests instead of changing the options.

There is good logic behind the intent of doing this.  But it never accounted for us actually adding/removing parameters from the list even if we never adjusted the actual values.  I think in reality we don't need the options collection hash and could simplify a lot of stuff in perfherder.

wlach, could we look at what we really use the options hash for and determine if we could retrofit the data or just remove it completely?  This might be a larger discussion to have in a couple weeks when we have more progress on our other perfherder deliverables.

Comment 2

[William Lachance (:wlach)]

We don't use any of these to create the options hash. The only options hash values are related to build: pgo, opt, cc, debug, and asan. See https://treeherder.mozilla.org/api/optioncollectionhash/

I'm pretty sure this should be fine to put in as-is.

Comment 3

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

Comment on attachment 8593313 [details] [diff] [review]
be smarter about what we output (1.0)

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

given the fact we don't see this affecting treeherder, lets review and land it!

Comment 4

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

https://hg.mozilla.org/build/talos/rev/7b0639478b68