-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
perf: optimize hot paths and reduce overhead for low-end devices #368
Conversation
Woah this is really impressive work, thank you so much. I'll try get some benchmarks tomorrow but all looks good so far.
I think this can probably be dropped from the API in v2, I don't think they're really used and the logic is confusing. (that is users providing promises to useHead as input)
Agreed! |
Separately, I would like to share a small victory in
After
|
Just let me know when you're finished making the changes you'd like, I'll work on getting it merged and benchmarked further. |
@harlan-zw I have made all the changes I wanted. You can merge now 😊 |
You've done a great job on this PR, thanks again. I've tried to do some benchmarking on these changes but the performance actually seems very slightly worst off, maybe you have a better idea on how to accurately bench this e2e though. Before
After
I think we can still probably merge this since we've slightly reduced the bundle size but wanted to give you the opportunity to provide e2e benchmarks as it would be great if we could advertise a % improvement. |
I think it greatly depends on the specifics of the project. Typically we have more than one During testing I used slowdown 6x in dev tools. For me, two factors were important in optimization:
A little later I will check what is slowing down in this benchmark. |
@harlan-zw I've finished improving other areas where performance was dropping. I updated the results in the main description of the PR. Could you please review it again? The only thing that bothers me in the It turns out that if we really want more performance, we need to move away from using Another thing consuming 1/4 of the performance is mini-loops that just checked a couple of properties to avoid code duplication. I removed them where direct checks are possible, but this doesn't add much duplication. The overall package size has indeed increased, but executing JS is much more expensive than parsing it. We could reduce the package size by dropping support for |
Hi @negezor, thanks again for your hard work on this I really appreciate it. I had some time to sit and make a proper SSR benchmark based on my site and all of the plugins being used. Running it on this PR it looks like you've improved the SSR by 20% 🎉 🚀! The package size difference is negligible so all good there.
As all tests are passing I'm going to merge this, I'll hold off on the release until I can do a little bit more testing, ideally, we release this as a patch but it might (?) be a little risky. Future Performance Opportunities I'd be very open to making breaking changes that will simplify the API, reduce internal code and improve the speed. These can be introduced in a v2 beta. You identified removing promises would help with this, would you be interested in working on this in a separate PR? The only constraint is that the DOM rendering should still be delayed via a promise (to avoid thrashing DOM updates with incremental Would be great to hear any other ideas you have also. Btw you should setup github sponsors if you can 😛, would be great to give you a one-off amount. |
That's cool. I've also been testing with my SSR, there the numbers range from 10% for the simplest pages and up to 25% on complex ones with multiple nested routers.
In case of any regressions, a patch can be released 😅
Yeah sure, now it'll just come down to deleting branches for Promise.
The module is great as it is, covering all my needs and covered by almost every possible test. I don't even have an idea of what to suggest 😅
I'm not planning to setup Github Sponsors at the moment, but thank you for the suggestion 😊 |
Wow very nice! 😍 🚀 I will try get the release out next couple of days, sorry for the delay! |
While profiling my website, I often see numerous
unhead
calls on hot paths on low-performance devices. Therefore, this PR aims to reduce the overall impact on performance. This PR includes:Set
instead of an array sinceset.has()
runs in constant time, unlikearray.includes()
https://jsperf.app/pikuzu/3string.startsWith()
for the first character, it's better to use index access as it is faster https://jsbench.me/welq15149d/1string.split()[0]
, it's better to usestring.indexOf()
+string.substring()
https://jsperf.app/zovukeasync
modifier from the function where aPromise
is already returned without the need forawait
Benchmark Result
There are a few other areas where performance can be improved, namely:
AvoidingImplemented via thePromise
as they hurt performancethenable
helper.Promise
is now seen as an edge caseReducing the number ofDone. If we want performance, we'll have to say goodbye toObject.entries()
Object.entries
,Object.values
andObject.keys
Possibly combining two sorts in the sort plugin.DoneSince this library is a building block for other applications, I believe it should have the maximum possible performance.
Performance was tested on the following hardware:
CPU:
AMD Ryzen 7950x3D
System:
WSL 2 Arch Linux on Windows 11
Node.js:
v22.5.0
Before
After
After using the forked hookable with these changes unjs/hookable#102