Open Bug 1299271 Opened 3 years ago Updated 2 years ago

create some service worker page load talos tests

Categories

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

defect

Tracking

()

People

(Reporter: bkelly, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Depends on: 1299887
Attached patch work-in-progressSplinter Review
Joel, what do you think of this test so far?  Takes about 3 minutes or so to run on my machine.
Attachment #8787379 - Flags: feedback?(jmaher)
Attachment #8787379 - Attachment is patch: true
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 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?
Attachment #8787379 - Flags: feedback?(jmaher) → feedback+
(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.
I think Catalin said he was going to steal this from me.  In either case, I am not working this at the moment.
Assignee: bkelly → catalin.badea392
Priority: -- → P2
: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.
No longer blocks: ServiceWorkers-e10s
Won't be able to take this.
Assignee: catalin.badea392 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mdaly)
:bkelly, in your spare time
Flags: needinfo?(mdaly) → needinfo?(bkelly)
Flags: needinfo?(bkelly)
You need to log in before you can comment on or make changes to this bug.