1855674 - Integrate new PDF.js tests into talos pdfpaint

Status: RESOLVED FIXED

(Testing :: Talos, task, P2)

Description

[Greg Mierzwinski [:sparky]]

This bug is for integrating new PDF.js tests into the pdfpaint test (or creating a variant of it).

It'll involve taking all of the pdfs found here, and integrating them in some way: https://github.com/mozilla/pdf.js/tree/master/test/pdfs

Ideally, this would be done with a fetch task. Note that some of the pdfs in that folder are links to PDFs that we need to download. There's a manifest file found here that provides the hashes of the various files: https://github.com/mozilla/pdf.js/blob/master/test/test_manifest.json

In the manifest, the id field should be used as the name of the subtest for a PDF that we are testing.

For metrics, we should gather the same thing as we already are. We should also run the task in the same places as the current pdfpaint task (talos-other) if we create a new one - it's likely as running through all the PDFs can take 20+ minutes.

There are also a couple ways of going about the fetch task: (i) using a cron, we rebuild the archive of PDFs once/week, or (ii) we build the archive outside of the tree, and upload it as a github release to our perf-automation repo.

Comment 1

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

any idea how often this will change? if it is >3x/year then we should go with a cron, otherwise a manual process as a release artifact in github and then use that as an artifact or resource for the perf tests.

Comment 2

[Calixte Denizet (:calixte)]

We regularly add some pdfs in our test-suite:
https://github.com/mozilla/pdf.js/commits/master/test/pdfs
That said to know if something has changed, we could just query gh in order to know if the file test_manifest.json has changed since the last time.

Comment 3

[Greg Mierzwinski [:sparky]]

One issue that comes up with the cron approach is that it can be difficult to tell when the archive changed in mozilla-central since it won't be tied to a commit, and this will cause issues with regression detection. We might end up going the route of a manual/semi-manual approach so that we can pin any test changes to a specific commit. Is there a bot we could maybe use for this?

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Greg Mierzwinski [:sparky] from comment #3)

One issue that comes up with the cron approach is that it can be difficult to tell when the archive changed in mozilla-central since it won't be tied to a commit, and this will cause issues with regression detection. We might end up going the route of a manual/semi-manual approach so that we can pin any test changes to a specific commit. Is there a bot we could maybe use for this?

Isn't it the same if we upload a ZIP file on GitHub with a bot?

Comment 5

[Greg Mierzwinski [:sparky]]

That would work, we could then update the fetch task on the mozilla-central side with a commit.

EDIT: For additional context, I'm talking about these kinds of fetch tasks: https://searchfox.org/mozilla-central/source/taskcluster/ci/fetch/benchmarks.yml

Comment 6

[Marco Castelluccio [:marco]]

What is the difference in terms of regression detection between a cron job on mozilla-central and a cron job that is running externally?

Comment 7

[Greg Mierzwinski [:sparky]]

When the mozilla-central cron job runs, it won't create a commit that we can associate any related performance changes with.

For example, if a mozilla-central cron update pulls an updated PDF, and then we find a performance regression on that test, there won't be an associated inidividual commit on autoland that would have caused it (we detect regressions on autoland). If we make a commit to change the tests, then we'll be able to more easily tell that a regression comes from a test change, or an actual code change.

Something else that comes to mind now is handling local runs, and running on older commits. Using a cron would make this part harder, while having something in-tree would let us more easily get the correct test artifact for any mozilla-central commit.

Comment 8

[Marco Castelluccio [:marco]]

Oh I see, so we would have to manually update the fetch task whenever we add new PDFs?

Comment 9

[Greg Mierzwinski [:sparky]]

Yes, that's correct. We could automate that part a bit though some day if we had something that could produce patches for us to review in phabricator.

Comment 10

[Marco Castelluccio [:marco]]

We could rely on updatebot, we already have a script that updates pdf.js in mozilla-central (https://searchfox.org/mozilla-central/rev/2f5b657343ce18e4b8a56417f707022550f4b742/toolkit/components/pdfjs/moz.yaml#46), it could also take care of updating the revision in the task that downloads the PDFs.
I would avoid the GitHub bot for simplicity of maintenance, we could have a single task in m-c where we store the git revision, and this task can download the PDFs at that specific revision. The talos task can depend on this download task.

Speaking more with Calixte though, we were thinking that we actually never change PDFs, we only add new ones. This means we would never have the problem of identifying whether regressions are due to test changes or code changes, as we can consider there not to be test changes. So we could also just ignore the problem and go with a cron task (unless the two solutions are similar in complexity, then we can go with the other one just to be "safe").

Comment 11

[Greg Mierzwinski [:sparky]]

Ok, that's good to know regarding no updates apart new PDFs, however, we'll still have a major issues with running locally, and on older commits if we use the cron with no pinned dependency.

Comment 12

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1855674 - Setup talos pdf toolchain artifact locally. r?#perftest,aglavic

This patch adds some code to pull in the toolchain artifact that contains the talos pdfs to test into the local mozbuild/state folder.

Comment 13

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1855674 - Remove handler from talos http webserver. r?#perftest,aglavic

This patch removes the handler from the talos http webserver. It's unnecessary with the new pdfpaint test setup, and it's also unclear if it was ever needed. See the phabricator patch D50943.

Depends on D205822

Comment 14

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1855674 - Modify pdfpaint test to run more PDFs. r?#perftest,aglavic

This patch modifies the pdfpaint test to run more pdfs that are found in the Mozilla pdf.js repository. The pdfpaint test is also moved to it's own suite due to the number of PDFs now being tested. These PDFs are pulled in locally from a toolchain task called talos-pdfs. The *ignore files are modified since the pdfpaint folder now contains a symbolic link to the local PDFs that should not be commit in-tree.

To handle running the large number of PDFs, chunking is added to the test with the chunk size being 100 PDFs. Each chunk runs each of the 100 PDFs 5 times. A CLI option is also added for local runs so that users can select a specific pdfpaint PDF to test. An additional issue with the subtest/pdf file name parsing is also fixed for this to work.

Depends on D205823

Comment 15

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1855674 - Run pdfpaint talos test in CI. r?#perftest,kshampur

This patch adds a set of tasks for the talos pdfpaint test in CI. It'll run with, and without the swr variant on linux, windows, and mac. The number of chunks are defined manually in a taskcluster transform where the test is chunked.

Depends on D205824

Comment 16

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1855674 - Add unit tests for pdfpaint test config setup. r?#perftest,aglavic

This patch adds unit tests for the pdfpaint test configuration setup. It tests the chunking (with, and without a specified chunk), bad chunk numbers, and the pdfpaint name option that specifies a single pdf to test.

Depends on D205825

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bf335f101f0
Setup talos pdf toolchain artifact locally. r=aglavic,perftest-reviewers
https://hg.mozilla.org/integration/autoland/rev/0dd2969e831c
Remove handler from talos http webserver. r=aglavic,perftest-reviewers
https://hg.mozilla.org/integration/autoland/rev/c60975ab65c0
Modify pdfpaint test to run more PDFs. r=aglavic,perftest-reviewers
https://hg.mozilla.org/integration/autoland/rev/58d1e628de29
Run pdfpaint talos test in CI. r=kshampur,perftest-reviewers,taskgraph-reviewers,ahal
https://hg.mozilla.org/integration/autoland/rev/8aa3f0cfdd9c
Add unit tests for pdfpaint test config setup. r=aglavic,perftest-reviewers

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3bf335f101f0
https://hg.mozilla.org/mozilla-central/rev/0dd2969e831c
https://hg.mozilla.org/mozilla-central/rev/c60975ab65c0
https://hg.mozilla.org/mozilla-central/rev/58d1e628de29
https://hg.mozilla.org/mozilla-central/rev/8aa3f0cfdd9c