1179292 - Write new runtime data only if sufficiently different from the existing values

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

I recently landed a script to generate runtime JSON files in bug 1173039.  However, the runtime files it generates are relatively large, and will completely change each time the script is run.

It might be better to write the files using pretty print, and to only change values that have drifted from the current value by some percentage.  This would reduce a lot of low-value churn, and also make differences easy to spot.

Comment 1

[Jonathan Griffin (:jgriffin)]

In order to provide sensible diffs, I'll also need to change the format of the runtimes JSON files.  The current files use a big dict, with non-deterministic test ordering; I'll switch to using a list, so ordering can be preserved.

Comment 2

[Jonathan Griffin (:jgriffin)]

Attachment: Write new runtime data only if changed,

This uses a new format for runtime files that preserves test ordering, so we can write only values that have changed significantly.  ActiveData is unhappy atm; I'll upload new runtimes files when that's back up.

Comment 3

[Jonathan Griffin (:jgriffin)]

Attachment: Update runtimes files,

New runtimes for all jobs in new format.

Comment 4

[Jonathan Griffin (:jgriffin)]

Attachment: Write new runtime data only if changed,

Oops, missed a file in the last patch

Comment 5

[Jonathan Griffin (:jgriffin)]

Attachment: Update runtimes files,

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631099 [details] [diff] [review]
Write new runtime data only if changed,

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

I'm a bit on the fence about this, we're trading accuracy and file size (which is a limiting factor on accuracy) for readable diffs. Readable diffs are definitely nice to have, but are they necessary? I won't stand in the way though. We can at least try it out and see how it works.

r+ with comments addressed.

::: testing/mozbase/manifestparser/manifestparser/filters.py
@@ +280,5 @@
>          # runtimes file will be assigned `default_runtime`.
>          self.runtimes = defaultdict(lambda: default_runtime)
> +        if isinstance(runtimes, list):
> +            for testcase in runtimes:
> +                self.runtimes[testcase[0]] = testcase[1]

self.runtimes.update(dict(runtimes))

::: testing/runtimes/writeruntimes.py
@@ +51,5 @@
> +            in_path = os.path.join(indir, dirname)
> +            outfilename = os.path.join(out_path, "%s.runtimes.json" % suite)
> +            infilename = os.path.join(in_path, "%s.runtimes.json" % suite)
> +            if not os.path.exists(out_path):
> +                os.mkdir(out_path)

This should be os.makedirs() in case there's more than one that needs to be created.

@@ +63,5 @@
> +                    # convert to dict for easy manipulation
> +                    runtimes = {}
> +                    for testcase_data in indata:
> +                        runtimes[testcase_data[0]] = testcase_data[1]
> +                    indata = runtimes

indata = dict(indata)

@@ +77,4 @@
>  
>              # split the durations into two groups; excluded and specified
>              ommitted = []
> +            specified = indata if indata else {}

Might as well just make indata default to {} above and not create a new variable.

@@ +89,5 @@
> +                elif duration >= threshold and test != "automation.py":
> +                    original = specified.get(test, 0)
> +                    if not original or abs(original - duration) > (original/20):
> +                        # only write new data if it's > 20% different than original
> +                        specified[test] = duration

20% seems kind of high, but we can tweak it later if needed.

@@ +115,2 @@
>              with open(outfilename, 'w') as f:
> +                f.write(json.dumps(results, indent=2))

This will increase the file sizes a bit, probably not too bad though.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631100 [details] [diff] [review]
Update runtimes files,

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

Any idea why there's whitespace after each line? Also how much bigger does this make the files?

Comment 8

[Jonathan Griffin (:jgriffin)]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Comment on attachment 8631100 [details] [diff] [review]
> Update runtimes files,
> 
> Review of attachment 8631100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Any idea why there's whitespace after each line? Also how much bigger does
> this make the files?

That seems to be what json.dumps() does by default (at least with indent=2).

> @@ +115,2 @@
> >              with open(outfilename, 'w') as f:
> > +                f.write(json.dumps(results, indent=2))
> 
> This will increase the file sizes a bit, probably not too bad though.

So the real reason to do this is not for readability, but so when we rev the files the next time, only the runtimes that have changed more than 20% get modified, and most of the file should remain unchanged (unless there's been across-the-board deterioration in runtimes).  We can't do that if we emit the entire JSON on one line.  Theoretically, this should reduce the size of future diffs.

Comment 9

[Kyle Lahnakoski [:ekyle]]

you may consider json.dumps(... sort_keys=True) for ordered output

Comment 10

[Jonathan Griffin (:jgriffin)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #9)
> you may consider json.dumps(... sort_keys=True) for ordered output

Nice, I didn't know about that trick.  I can simplify this code a bit more then.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631100 [details] [diff] [review]
Update runtimes files,

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

Nice, I didn't know about sort_keys either. Yeah, a dict is much nicer to look at than a list of lists.

Comment 12

[Jonathan Griffin (:jgriffin)]

Attachment: Update runtimes,

Comment 13

[Jonathan Griffin (:jgriffin)]

Attachment: Write new runtime data only if changed,

And the script patch, in case you want to review again.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0a4bb7ba09
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07252796012

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a0a4bb7ba09

Comment 16

[Jonathan Griffin (:jgriffin)]

Still need to land the actual runtimes changes, which were backed out.

Comment 17

[Jonathan Griffin (:jgriffin)]

Attachment: Make mozinfo recognize linux64-asan as a distinct platform,

Verifies this fixes the problem I had updating runtimes, with a green try run to show:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8043b90096f1

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8633786 [details] [diff] [review]
Make mozinfo recognize linux64-asan as a distinct platform,

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

Thanks, lgtm!

::: python/mozbuild/mozbuild/mozinfo.py
@@ +108,5 @@
>              if d['buildapp'] == 'mulet':
>                  p = '{}-mulet'.format(p)
> +
> +            if d['asan']:
> +                p += "-asan"

nit: '{}-asan'.format(p) to be consistent with above

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6376e51185e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7107f5d99e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6376e51185e8
https://hg.mozilla.org/mozilla-central/rev/9b7107f5d99e