1502116 - web-tooling subtests should be 'score'

Status: VERIFIED FIXED

(Testing :: General, enhancement)

Description

[Armen [:armenzg]]

In bug 1502036 we discovered that web-tooling subtests are marked as *ms* "lower is better" (by *not* specifying "lower_is_better: false") [1]

However, we need it to be a *score* "higher is better".

This is similar to bug 1486789.
This blocks bug 1502036.
The metadata in Perfherder will be updated in bug 1502073.

[1]
https://treeherder.mozilla.org/api/project/mozilla-central/performance/signatures/?parent_signature=620f90feca6c0916ca8e593737eb6bf6d0301f68

Comment 1

[Armen [:armenzg]]

Adapted from bug 1502036:
> Looking at the graphs for webtooling [1] we see a peculiar oddity. 
> 
> In the case of web-tooling, the aggregate results are being reported as a __Score__,
> where higher-is-better, and each subtest is being reported as __execution time in ms__,
> where lower-is-better.
> 
> However, if you look at the score and subtests, comparing Chrome and SpiderMonkey,
> they disagree about what is better: Chrome shows a lower (worse) score than
> SpiderMonkey in the aggregate score, but for every subtest shows lower (better)
> execution time, which should imply a higher score.

From the logs we can see that the subscores are also scores [2] as mgaudet points out.
From PERFHERDER_DATA only the main score has __"lowerIsBetter": false__ while the subtests don't have that property (in the absence of it we default to lower is better).

We need to change the __subtests__ of web-tooling to report as higher is better.

I think the line that needs editing is this one:
https://searchfox.org/mozilla-central/source/testing/jsshell/benchmark.py#294
> self.suite['subtests'].append({'name': test_name, 'value': mean})
I think we just need to change it to:
> self.suite['subtests'].append({
>     'lower_is_better': false,
>     'name': test_name,
>     'value': mean,
> })

I will give it a try. I don't remember the last time I pushed to Try :)

[1]: https://arewefastyet.com/linux64/web-tooling?numDays=90
[2] 
[task 2018-11-09T18:23:34.868Z] Running Web Tooling Benchmark 0.3.2...
[task 2018-11-09T18:23:34.869Z] --------------------------------------
[task 2018-11-09T18:23:41.227Z]          acorn:  6.93 runs/sec
[task 2018-11-09T18:23:48.455Z]          babel:  6.71 runs/sec
[task 2018-11-09T18:23:55.041Z]        babylon:  6.23 runs/sec
[task 2018-11-09T18:24:03.716Z]          buble:  4.88 runs/sec
[task 2018-11-09T18:24:07.838Z]           chai: 10.13 runs/sec
[task 2018-11-09T18:24:18.674Z]   coffeescript:  3.64 runs/sec
[task 2018-11-09T18:24:31.902Z]         espree:  3.05 runs/sec
[task 2018-11-09T18:24:38.583Z]        esprima:  5.66 runs/sec
[task 2018-11-09T18:24:51.414Z]         jshint:  3.17 runs/sec
[task 2018-11-09T18:24:58.603Z]          lebab:  5.87 runs/sec
[task 2018-11-09T18:25:06.131Z]        prepack:  5.51 runs/sec
[task 2018-11-09T18:25:13.417Z]       prettier:  5.95 runs/sec
[task 2018-11-09T18:25:17.881Z]     source-map: 15.04 runs/sec
[task 2018-11-09T18:25:23.878Z]     typescript:  7.39 runs/sec
[task 2018-11-09T18:25:28.665Z]      uglify-es: 11.92 runs/sec
[task 2018-11-09T18:25:38.502Z]      uglify-js:  4.16 runs/sec
[3]
{
	"framework": {
		"name": "js-bench"
	},
	"suites": [{
		"name": "web-tooling-benchmark-sm",
		"lowerIsBetter": false,
		"value": 6.03,
		"shouldAlert": false,
		"units": "score",
		"subtests": [{
			"name": "web-tooling-benchmark-source-map",
			"value": 15.04
		}, {

Comment 2

[Armen [:armenzg]]

Attachment: patch

Could someone please try this patch on Try?

You just need to test this for the webtool jobs (sm and v8).
Thanks!

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61706e80f7406b63b9295405d001661ce653fbcd

Comment 4

[Armen [:armenzg]]

Thanks mgaudet!

The subtests now seem to have 'lower_is_better': false defined [1]
I think this is sufficient.


[1]
https://treeherder.mozilla.org/logviewer.html#?job_id=210906528&repo=try
> "subtests": [{
>   "lower_is_better": false,
>   "name": "web-tooling-benchmark-source-map",
>   "value": 13.75
> }, {

Comment 5

[Armen [:armenzg]]

Comment on attachment 9024078 [details] [diff] [review]
patch

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

Hi Ionut,
If this looks good to you, would you mind landing it for me? Thanks!

My Hg account has been disabled for lack of usage and I won't reactivate it since I won't need it in the future.

Comment 6

[Ionuț Goldan [:igoldan]]

(In reply to Armen [:armenzg] from comment #5)
> Comment on attachment 9024078 [details] [diff] [review]
> patch
> 
> Review of attachment 9024078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ionut,
> If this looks good to you, would you mind landing it for me? Thanks!
> 
> My Hg account has been disabled for lack of usage and I won't reactivate it
> since I won't need it in the future.

Could you first push this to Phabricator, using Arcanist? I'm not familiar with landing patches on mozilla-inbound directly.

Comment 7

[Armen [:armenzg]]

Attachment: Bug 1502116 - web-tooling subtests should be 'score'

Comment 8

[Armen [:armenzg]]

Let me know if there's anything else I need to do:
https://phabricator.services.mozilla.com/D11768

I don't know how to land from Phabricator; Could you please land it for me?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26360851a3ba
web-tooling subtests should be 'score' r=igoldan

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/26360851a3ba

Comment 11

[Armen [:armenzg]]

We named it wrong (lowerIsBetter vs lower_is_better).
The subtests are still reporting incorrectly as lower is better [1]

[1] https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1760198,1,11&selected=mozilla-central,1760198,403880,643400991,11
[2] https://taskcluster-artifacts.net/F1dfOqM-THe72nyfI5Bt0g/0/public/logs/live_backing.log
> {
> 	"framework": {
> 		"name": "js-bench"
> 	},
> 	"suites": [{
> 		"name": "web-tooling-benchmark-sm",
> 		"lowerIsBetter": false,
> 		"value": 6.0,
> 		"shouldAlert": false,
> 		"units": "score",
> 		"subtests": [{
> 			"lower_is_better": false,
> 			"name": "web-tooling-benchmark-source-map",
> 			"value": 13.58
> 		}, {

Comment 12

[Armen [:armenzg]]

Attachment: Bug 1502116 - web-tooling subtests to use lowerIsBetter instead of lower_is_better

Comment 13

[Armen [:armenzg]]

How do we schedule jobs on Try from Phabricator?

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

AFAIK that's not currently possible. I will submit it for you :)

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f47907bb40a44edeaf8b3ba1992e13faeec00af2

Comment 16

[Armen [:armenzg]]

Thank you!

igoldan: would you be able to review this and land it for me? thanks!

>		"lowerIsBetter": false,
>		"value": 6.11,
>		"shouldAlert": false,
>		"units": "score",
>		"subtests": [{
>			"lowerIsBetter": false,

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1cbe1578a46
web-tooling subtests to use lowerIsBetter instead of lower_is_better r=jmaher

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d1cbe1578a46

Comment 19

[Armen [:armenzg]]

mgaudet: would you mind verifying this?

I think we might have fixed this:
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1760187,1,11&series=mozilla-central,1760198,1,11&selected=mozilla-central,1760198,405116,646080268,11
https://www.dropbox.com/s/g7kfufpxval6mf9/Screenshot%202018-11-19%2015.03.10.png?dl=0
https://arewefastyet.com/linux64/web-tooling?numDays=90
https://www.dropbox.com/s/l33n0g8ebfe6130/Screenshot%202018-11-19%2015.04.10.png?dl=0

Comment 20

[Matthew Gaudet (he/him) [:mgaudet]]

Ok, this looks good.