1476053 - Make wpt metadata update faster and use less memory

Status: RESOLVED FIXED

(Testing :: web-platform-tests, enhancement)

Description

[James Graham [:jgraham]]

The wpt metadata update is currently super-slow and can use a lot of memory. The basic design at the moment is that we read all the data in to the metadata ini files, update each ini file and write out the results. But that's largely unnecessary.

Instead, we only need to read/update ini files where something changed (this will also avoid unwanted serialization changes to unaffected files). It's still necessary to load all the result data upfront since we need to convert from result-per-run to result-per-test, which is effectively a transpose of the input data. However this can be made much more efficient than the current setup.

In addition we can use the wptreport.json files to do the metadata updates. This will mean parsing far less json data, which should be a notable performance improvement, and certainly reduce the time to download the log files.

Comment 1

[James Graham [:jgraham]]

Attachment: Bug 1476053 - Add an option not to store state when running wpt-update,

This adds some overhead and can be annoying since it requries explicitly aborting failed jobs.
The state storage isn't very useful for just udpating metadata (the typical gecko usecase), but
is useful for performing syncs (a typical servo usecase). Therefore add a --no-store-state option
and set it by default in the gecko mach frontend.

Review commit: https://reviewboard.mozilla.org/r/258434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258434/

Comment 2

[James Graham [:jgraham]]

Attachment: Bug 1476053 - Add lsan leak reports to wptreport output,

This adds a top level key lsan_leaks iff there are any leaks detected in the run.
The data is equivalent to that in the underlying structured logger.

Review commit: https://reviewboard.mozilla.org/r/258436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258436/

Comment 3

[James Graham [:jgraham]]

Attachment: Bug 1476053 - Optimise wpt metadata updates,

The previous strategy for wpt metadata updates was to read add the
existing expected files, annotate them with the new results, compute
the resulting expectation file, and reserialize everything. This was
quite straighforward, but it has a number of disadvantages:

* Slow, since all expectation files need to be read and written, even
  when there were no changes affecting those files.
* Memory intensive, since we have to load all the results and the
  metadata at the same time, and little optimisation had been
  done. This could lead to OOM for large updates on some machines.
* Confusing, since parsing and reserializing files leads to changes in
  "unrelated" metadata

Instead we take the approach that we should only update the metadata
in cases where there are actually changes. Inteads of loading all the
expectation files, we do the following:

* Load all the results data into a large nested dictionary, keeping
  track of which results files had an update that requires
  regenerating metadata.

* Load/serialize metadata only in the cases it changed, and one at a
  time.

Loading all the data upfront is still necessary because we effectively
need to traspose the input data in cases where we have multiple
configurations running the same test (i.e. we get the data per run
and want the data for all runs per test). This approach also has the
disadvantage that it's more likely to miss a necessary update. However
the tradeoffs for better performance are still worthwhile.

Review commit: https://reviewboard.mozilla.org/r/258438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258438/

Comment 4

[James Graham [:jgraham]]

Attachment: Bug 1476053 - Enabling updating wpt metadata from wptreport.json files,

Raw log files have the big disadvantage that they are large and we
therefore spend a lot of time just downloading the log files and
parsing json. wptreport.json files are much smaller and so reduce
these issues.

In order to support both formats, we first try parsing the log as
json. If that succeeds, and we have the expected keys, we parse it as
a wptreport file by reconstructing the events we would have got in a
raw log.

If that doesn't work we fall back to the standard raw log processing.

Review commit: https://reviewboard.mozilla.org/r/258440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258440/

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 8993729 [details]
Bug 1476053 - Add an option not to store state when running wpt-update,

https://reviewboard.mozilla.org/r/258434/#review265530

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8993730 [details]
Bug 1476053 - Add lsan leak reports to wptreport output,

https://reviewboard.mozilla.org/r/258436/#review265560

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8993731 [details]
Bug 1476053 - Optimise wpt metadata updates,

https://reviewboard.mozilla.org/r/258438/#review265562

It would be great to see tests for this code, but I realise that
might be easier said than done…

::: testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py:383
(Diff revision 1)
> +    def set_requires_update(self):
> +        self._requires_update = True

Unsure what the benefit of this function is as opposed to writing
to the mutable class property:

> self.requires_update = True

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8993732 [details]
Bug 1476053 - Enabling updating wpt metadata from wptreport.json files,

https://reviewboard.mozilla.org/r/258440/#review265564

Comment 9

[James Graham [:jgraham]]

Attachment: Bug 1476053 - Handle invalid json in metadata updates,

Sometimes we get errors that result in invalid json in the logs.
There's nothing reasonable to do about this during metadata updates
so just skip it

Review commit: https://reviewboard.mozilla.org/r/258844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258844/

Comment 10

[James Graham [:jgraham]]

Comment on attachment 8993729 [details]
Bug 1476053 - Add an option not to store state when running wpt-update,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258434/diff/1-2/

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8993730 [details]
Bug 1476053 - Add lsan leak reports to wptreport output,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258436/diff/1-2/

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8993731 [details]
Bug 1476053 - Optimise wpt metadata updates,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258438/diff/1-2/

Comment 13

[James Graham [:jgraham]]

Comment on attachment 8993732 [details]
Bug 1476053 - Enabling updating wpt metadata from wptreport.json files,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258440/diff/1-2/

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8994269 [details]
Bug 1476053 - Handle invalid json in metadata updates,

https://reviewboard.mozilla.org/r/258844/#review266062

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc1c54e5dcea
Add an option not to store state when running wpt-update, r=jdm
https://hg.mozilla.org/integration/autoland/rev/1380819f4aa5
Add lsan leak reports to wptreport output, r=ato
https://hg.mozilla.org/integration/autoland/rev/fcf3f7f2079a
Optimise wpt metadata updates, r=ato
https://hg.mozilla.org/integration/autoland/rev/bfb7d92282e8
Enabling updating wpt metadata from wptreport.json files, r=ato
https://hg.mozilla.org/integration/autoland/rev/ab1b54921f26
Handle invalid json in metadata updates, r=ato

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bc1c54e5dcea
https://hg.mozilla.org/mozilla-central/rev/1380819f4aa5
https://hg.mozilla.org/mozilla-central/rev/fcf3f7f2079a
https://hg.mozilla.org/mozilla-central/rev/bfb7d92282e8
https://hg.mozilla.org/mozilla-central/rev/ab1b54921f26

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12246 for changes under testing/web-platform/tests

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: Bug 1476053 add() to 'clear' set for --ignore-existing because append() does not exist r?jgraham

Comment 20

[Phabricator Automation]

Comment on attachment 9044793 [details]
Bug 1476053 add() to 'clear' set for --ignore-existing because append() does not exist r?jgraham

Revision D20246 was moved to bug 1528870. Setting attachment 9044793 [details] to obsolete.