1299271 - Create some service worker page load performance tests

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, task, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Large web properties interested in using service workers are very sensitive to performance.  Right now we are not measuring our service worker performance at all.  We should correct this by adding some talos tests.

As a start, I plan to build some tests around this benchmark written by Jake Archibald:

https://jakearchibald.github.io/service-worker-benchmark/
https://github.com/jakearchibald/service-worker-benchmark/

I have permission from Jake to use the images.

Blocking the e10s effort because we expect our multi-process architecture changes to impact perf here.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: work-in-progress

Joel, what do you think of this test so far?  Takes about 3 minutes or so to run on my machine.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I will probably collapse them all back into a single talos test entry since my attempt to disable image cache with a pref didn't seem to work.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 8787379 [details] [diff] [review]
work-in-progress

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

do you need all of those image files?  Could we not use the same file over and over again?

I have a handful of nits/questions/feedback.  I am looking forward to this coming online- have you ran this on try?  I usually edit:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?q=path%3Atalos.json&redirect_type=single

for example, edit g4/g4-e10s to put your test name(s) in there and push to try- that will schedule the jobs in the g4.

::: testing/talos/talos/test.py
@@ +714,5 @@
> +    Service worker tests
> +    """
> +    tpmanifest = '${talos}/tests/service_worker/sw_images.manifest'
> +    tpcycles = 1
> +    tppagecycles = 1

typically we collect many data points- most cases have tppagecycles as 20 or 25.

@@ +725,5 @@
> +class image_bench_image_cache(PageloaderTest):
> +    """
> +    Service worker tests
> +    """
> +    tpmanifest = '${talos}/tests/service_worker/necko_images.manifest'

I don't ssee necko_images.manifest in the patch.

::: testing/talos/talos/tests/service_worker/benchmark/cached-fetch.js
@@ +1,4 @@
> +self.oninstall = event => {
> +  self.skipWaiting();
> +
> +  const toCache = Array(300).fill('').map((v, i) => `images/${i}.png`);

possibly if we need different images, could we temporarily rename all the files?  Effectively store a single image and have the file system copy it?  How are the filesystem .png files different from what is in data-url-fetch.js ?

@@ +11,5 @@
> +
> +self.onfetch = event => {
> +  if (!event.request.url.endsWith('.png')) {
> +    return;
> +  }

the coding style in data-url-fetch.js doesn't match what is here- we should make the styles the same :)

::: testing/talos/talos/tests/service_worker/benchmark/index.html
@@ +377,5 @@
> +        button.textContent = text;
> +        button.onclick = event;
> +        return button;
> +      }
> +      

nit: whitespace on line 381

::: testing/talos/talos/tests/service_worker/images.html
@@ +4,5 @@
> +<script>
> +let params = new URLSearchParams(window.location.search);
> +let opts = {
> +  sw: 'none',
> +  cycles: 25,

I prefer cycles to be defined in the python test.py file, but if there is a good reason to keep it in here, I have no problem with it.

@@ +69,5 @@
> +  if (opts.sw === 'none') {
> +    return finish();
> +  }
> +
> +  delay(1000).then(_ => {

with a delay(1000) can we add a comment why this is here?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Joel Maher ( :jmaher) from comment #3)
> Comment on attachment 8787379 [details] [diff] [review]
> work-in-progress
> 
> Review of attachment 8787379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> do you need all of those image files?  Could we not use the same file over
> and over again?

So, a lot of the code and images are actually an import of a separate benchmark that lives here:

  https://github.com/jakearchibald/service-worker-benchmark/

I'd like to keep it as close to that as possible so if someone regresses these tests I can just point them at:

  https://jakearchibald.github.io/service-worker-benchmark/

I'll split the patches up to make it clear which files are imported vs. newly written here.

> ::: testing/talos/talos/test.py
> @@ +714,5 @@
> > +    Service worker tests
> > +    """
> > +    tpmanifest = '${talos}/tests/service_worker/sw_images.manifest'
> > +    tpcycles = 1
> > +    tppagecycles = 1
> 
> typically we collect many data points- most cases have tppagecycles as 20 or
> 25.

Since I'm doing internal measurements with 25 iterations, I thought I would do only a single outer cycle.

I was going to tweak this stuff anyway, since I'd like to have 30 iterations for run.  So 31 ignoring the first iteration.

If you want me to use something like tppagecycles=5, then I would need to add code to internally ignore the first data point.  It will typically be skewed by service worker thread startup, cold http cache, etc.  I plan to add a separate test for service worker cold start.

> I don't ssee necko_images.manifest in the patch.

Yea, I forgot to hg add.  I'm going to remove it, though.

> possibly if we need different images, could we temporarily rename all the
> files?  Effectively store a single image and have the file system copy it? 
> How are the filesystem .png files different from what is in
> data-url-fetch.js ?

This is part of the imported benchmark, so I don't want to change it.

Note, the data URL case is different from cached-fetch in that it avoids exercising the DOM Cache API.  So you can look at the difference between these two tests and infer overhead costs from Cache API.

> > +self.onfetch = event => {
> > +  if (!event.request.url.endsWith('.png')) {
> > +    return;
> > +  }
> 
> the coding style in data-url-fetch.js doesn't match what is here- we should
> make the styles the same :)

This was a temp hack I did to try to work around the crash and I need to remove it.

> ::: testing/talos/talos/tests/service_worker/benchmark/index.html
> @@ +377,5 @@
> > +        button.textContent = text;
> > +        button.onclick = event;
> > +        return button;
> > +      }
> > +      
> 
> nit: whitespace on line 381

Part of the imported benchmark.

> > +let opts = {
> > +  sw: 'none',
> > +  cycles: 25,
> 
> I prefer cycles to be defined in the python test.py file, but if there is a
> good reason to keep it in here, I have no problem with it.

Well, it would make the overall test run a lot slower if I have to register a SW, execute one iteration, and then unregister the SW for each cycle.  Also, I'd have to build the logic I mention above to run a cold iteration before a hot iteration.  With the current setup I can just use the filter mechanism to ignore that first data point.

> > +  delay(1000).then(_ => {
> 
> with a delay(1000) can we add a comment why this is here?

It was a temp hack when I was trying to debug the crash.  I'll remove it.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I think Catalin said he was going to steal this from me.  In either case, I am not working this at the moment.

Comment 6

[Benoit Girard (:BenWa)]

:bkelly told me to post this here. I created a loading microbenchmark that includes SW. This microbenchmark behaves similar to how a Facebook would from the browser's point of view. It sends a quick 'early' flush response within 100ms (network RTT) that will begin preparing scripts, followed by a final document load 500ms later to finish the page (time to generate the page):

https://github.com/bgirard/fb_loading_microbench

It features a readable stream SW similar to Facebook's. This SW will output the 100ms 'early flush' ASAP and will strip it out of the response generated from the server and it will leave the page's timing intact.

Note that there's a lot of configuration options in the code that can be changed to simulate different scenarios. There's a lot of useful real world scenarios that are worth considering here that can be tested by this microbenchmark:
1) Baseline cache performance.
2) The impact of adding/removing JS resources.
3) If SW is not installed, installed but not started, installed and started
4) If the data is primed in the HTTP cache or not (data is marked as immutable and doesn't revalidate)
5) The server flush timing to simulate high latency environment where a readable stream service worker performs better.

Happy to provide more information if you're interested in using this.

Comment 7

[Cătălin Badea (:catalinb)]

Won't be able to take this.

Comment 8

[Marion Daly [:mdaly]]

:bkelly, in your spare time

Comment 9

[Joshua Marshall]

Attachment: Bug 1299271 - Service worker performance test r=sparky!

Comment 10

[Andrew Creskey [:acreskey]]

Joshua, if you'd like to discuss adding these tests to the performance test frameworks, ping me or jump into
https://matrix.to/#/#perftest:mozilla.org

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a14206eb5a9
Service worker performance test r=asuth,sparky

Comment 12

[Greg Mierzwinski [:sparky]]

This test will be running on each autoland commit, but I'll be landing a patch here to optimize the scheduling for this test (and all other perftest tests): https://bugzilla.mozilla.org/show_bug.cgi?id=1866055

Comment 13

[Sandor Molnar[:smolnar]]

Backed out for causing python taskgraph-tests failures at taskcluster/gecko_taskgraph/test/test_util_chunking.py::test_get_manifests

Backout link: https://hg.mozilla.org/integration/autoland/rev/2d3f172e9411fdb06672578ecd38d5eb38ae03e6

Push with failures

Failure log -> taskcluster/gecko_taskgraph/test/test_util_chunking.py::test_get_manifests[platform0-mochitest-plain] TEST-UNEXPECTED-FAIL

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/fa3223e08cd7
Backed out changeset 6a14206eb5a9 for causing python taskgraph-tests failures at taskcluster/gecko_taskgraph/test/test_util_chunking.py CLOSED TREE

Comment 15

[Joshua Marshall]

Attachment: Bug 1299271 - Add service worker perftest taskcluster tasks for win/mac r=sparky!

Depends on D177427

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed65c46d7d73
Service worker performance test r=asuth,sparky
https://hg.mozilla.org/integration/autoland/rev/c4a3db1210b5
Add service worker perftest taskcluster tasks for win/mac r=sparky,taskgraph-reviewers,ahal

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ed65c46d7d73
https://hg.mozilla.org/mozilla-central/rev/c4a3db1210b5