-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance issues with swiCleanup #40
Comments
Note, this also shows up in firefox. Here is the firefox nightly profile: |
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 I see |
After poking at this a bit more, I think that some of the overhead is due to
If I change that @Snugug, what's the significance of using There's no test suite for |
It's not. But try adding the dotAll |
|
Switching to At least it's obvious what the expensive bit is now! |
I've been looking into the Does the If so, I think a way forward that will be lighter-weight would be to just save the "pristine" HTML in the The service worker should be automatically kept alive via (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 I'm happy to do a little refactoring of the code to accomplish this if you think it sounds good, @Snugug. |
@jeffposnick Yah, you nailed it.
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. |
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 |
Can the server provide a different representation that's easier to process? |
(FWIW, this expensive |
@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 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.
|
Also, here's a new My original
|
@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. |
The code complexity increases quite a bit if you have to supported nested includes, unfortunately 😒 Perhaps you could get away with not supported nesting? For those playing along at home, I've been trying out the nested scenarios with
as a sample input test case, and the expected result is
|
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
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.The text was updated successfully, but these errors were encountered: