Performance issues with swiCleanup

[jeffposnick]

Following up on some traces that @wanderview performed on navigation requests to https://chromeos.dev/, it looks like there's a synchronous JS microtask delay of ~1.5s (on desktop Chrome), with much of it attributable to the swiCleanup callback function's usage of [Symbol.split]:

static-site-scaffold-modules/modules/service-worker-includes/index.js

Lines 84 to 115 in e1cd1cd

	async function swiCleanup({ response }) {
	const content = await response.text();
	const matches = content.match(includesRegExp);
	// const neededIncludes = [...new Set(matches)].map(i => i.split(includesFileRegExp)[1]);
	let open = 0;
	const rebuild = content
	.split(includesRegExp)
	.map(i => {
	// If the current item is the include and it's not included from within
	if (matches && i === matches[0]) {
	matches.shift();
	open++;
	if (open > 1) return '';
	return i;
	}
	const endIncludeSplit = i.split(endIncludeWithLeadingRegExp);
	if (endIncludeSplit.length === 1 && open !== 0) {
	return '';
	}
	const count = i.match(endIncludeRegExp);
	open = open - (count ? count.length : 0);
	return endIncludeSplit.join('');
	})
	.join('');
	return new Response(rebuild, { headers: response.headers });
	}

This might be due to the complexity of the RegExps being passed to .split(), the size of the source string that the RegExps are being run against, or something else regarding the algorithm in that function.

If it turns out to be correlated to the complexity of the RegExps, it might be worth checking whether the related serviceWorkerInclude function could be sped up in a similar way, since that's using a similar set of RegExps.

[wanderview]

Note, this also shows up in firefox. Here is the firefox nightly profile:

https://share.firefox.dev/31FGqyY

[jeffposnick]

In an attempt to isolate just the code that might potentially be slow from the larger service worker code, I put together https://gist.github.com/jeffposnick/e0f6a1bc48d66bf118deb908e442188a

It's runnable by copying and pasting into your browser's console, and the input HTML that the RegExps are being run against is taken from the https://chromeos.dev pages-cache.

I see swiCleanup: 2254.2080078125ms when I run it using the real-world HTML, but that drops down to maybe 500ms or so when run against a very small HTML string. So performance definitely degrades as the size of the HTML increases, but even for a trivial string, it still does take a potentially noticeable amount of time.

[jeffposnick]

After poking at this a bit more, I think that some of the overhead is due to

static-site-scaffold-modules/modules/service-worker-includes/index.js

Line 38 in e1cd1cd

const endIncludeWithLeadingRegExp = /[\s\S]*<!--\s*#endinclude\s*-->/gm;

If I change that [\s\S]* from the start of that RegExp into a .*, then the execution time goes from around 2200ms down to 20ms(!). You can try that out by comparing https://gist.github.com/jeffposnick/e0f6a1bc48d66bf118deb908e442188a#file-faster-js to https://gist.github.com/jeffposnick/e0f6a1bc48d66bf118deb908e442188a#file-original-js

@Snugug, what's the significance of using [\s\S]* in your original RegExp? I'm not 100% sure that [\s\S\]* is directly equivalent to .*, but based on what I know about RegExps, I think they're fairly close.

There's no test suite for service-worker-includes, so I don't feel comfortable enough making that change myself and ensuring that it doesn't break anything. But that at least seems like a promising place to investigate.

[tomayac]

I'm not 100% sure that [\s\S]* is directly equivalent to .*

It's not. But try adding the dotAll s flag instead of [\s\S]* so . matches newline characters as well.

[tomayac]

const endIncludeWithLeadingRegExp = /.*<!--\s*#endinclude\s*-->/gms;

[jeffposnick]

Switching to .* with the s flag takes it back to swiCleanup: 2224.31591796875ms.

At least it's obvious what the expensive bit is now!

[jeffposnick]

I've been looking into the service-worker-includes codebase a bit more, and I just wanted to clarify something with @Snugug:

Does the swiCleanup code just "undo" the serviceWorkerInclude code, getting the HTML body back to the "pristine" state matching what's returned from the network?

If so, I think a way forward that will be lighter-weight would be to just save the "pristine" HTML in the serviceWorkerInclude somewhere in your module's state, keyed to the incoming request URL, and then read that "pristine" HTML from memory inside of swiCleanup and use that to write to the cache, clearing out the in-memory cache when that's done. That way, you could avoid the expensive RegExps that undo your templating.

The service worker should be automatically kept alive via waitUntil() during this interval, so I don't think there's much risk of losing the state in between the time you read the network response and the time you write to cache. The worst-case scenario would be that you skip writing to cache if there was missing in-memory state.

(This will actually be cleaner to do in v6, as plugins now get their own request-scoped state, so you wouldn't have to use a static Map on the module.)

I'm happy to do a little refactoring of the code to accomplish this if you think it sounds good, @Snugug.

[Snugug]

@jeffposnick Yah, you nailed it. swiCleanup takes the incoming request, removes the bits that are handled by the include code, and responds with the pristine code. Your solution sounds like it'll work. The things that need to hold true through your refactor are:

• The page being served to the user has the proper includes placed in
• The cleaned version is saved to the cache
• We don't fall into a state where there are cache misses for something that's already been hit via a staleWhileRevalidate, for instance, or into a state where what's in the cache hasn't been cleaned.

If these need to be weighted, properly swapping in the precached includes is most important (even if it means not cleaning them before they hit the cache), followed by properly cleaning them before they hit cache because the former is the core bit of functionality of the module while the later is for reducing cache size.

[jeffposnick]

Oh, I think I am misinterpreting something then

I was under the impression that the network response would already be "pristine" and could just be used as-is to populate the cache, but based on what you say (and on looking at https://chromeos.dev traffic), that's not the case. The network response needs to have bits stripped out before it could be saved to the cache, and stripping out those bits is what the expensive RegExp is doing.

So... I think we're back to figuring out how to speed up the logic in swiCleanup().

[wanderview]

Can the server provide a different representation that's easier to process?

[jeffposnick]

(FWIW, this expensive RegExp processing would be a perfect candidate for offloading to a Worker, but browsers don't support spinning up a new Worker from inside of the ServiceWorkerGlobalScope.)

[jeffposnick]

@Snugug, here's a potential refactoring for which I'd appreciate a correctness check.

The one breaking change (beyond some minor differences about being more strict about extraneous space characters, which is just a slight optimization) is that this won't work if there are nested includes, or any other scenario where there might be more than one <!--include virtual='...'--> before a <!--#endinclude-->.

I'm assuming you don't need nested includes (https://github.com/chromeos/static-site-scaffold-modules/tree/master/modules/service-worker-includes#service-worker-includes doesn't indicate one way or another), but I do want to make sure that they're clearly documented as not being supported in that case.

async function swiCleanup({ response }) {
  const templatedContent = await response.text();

  const newBody = [];
  // We don't want <!--#endinclude--> to be included in the new body, so just
  // split on it and don't add it back.
  for (const part of templatedContent.split('<!--#endinclude-->')) {
    const match = part.match(/<!--#include virtual=['"][^'"]+['"]-->/);
    if (match) {
      // If there's a match, add all of the HTML, up to and including the first
      // <!--#include virtual="..."-->, but excluding anything that follows the
      // first include tag.
      // Note: This won't work as expected if there are multiple include tags
      // before an <!--#endinclude-->, e.g. in nested-include scenarios.
      newBody.push(part.substring(0, match.index + match[0].length));
    } else {
      // If this happens, it means that there was an <!--#endinclude--> without
      // a corresponding <!--#include virtual="..."-->
      // Safest thing to do is just to use all the HTML for that part, as-is.
      newBody.push(part);
    }
  }

  return new Response(newBody.join(''), { headers: response.headers });
}

[jeffposnick]

Also, here's a new gist with a better test case and comparison of the output of both the original code and this revised code: https://gist.github.com/jeffposnick/fd33ce98a32973b2ca738293744101b6

My original gist was processing HTML from the cache (which had already been cleaned), and this new gist uses HTML from the network (which still needs to be cleaned, and therefore more closely matches what the service worker will be doing).

oldCleanup: 1421.18017578125ms
newCleanup: 0.81689453125ms

[Snugug]

@jeffposnick Thanks so much for working on this!

I do not believe we need to support removing multiple includes if they're nested, as the highest-level one will be the one that gets replaced ultimately, but someone should be able to nest multiple includes; consider a logo being reused in a header and a footer, with all three being includes.