-
-
Notifications
You must be signed in to change notification settings - Fork 85
vibe optimization #547
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
vibe optimization #547
Conversation
|
Hey @quantizor, thanks for optimizing the runtime! Just took a brief look at the diff and I think it would be good to split this into multiple PRs so I can verify some changes in isolation. If you're up to it, I'll review your changes in more detail tomorrow, probably leave a bunch of comments and let you know how to best split it up. |
Sure thing! |
dcastil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff! I added some comments around some details. At the scale of tailwind-merge it makes sense to pay attention to every small thing to not introduce potential regressions.
|
@dcastil updated per your suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @quantizor, thanks for the really long wait!
Thanks for addressing all the comments. I added a few commits fixing some minor things as well, hope you don't mind.
I checked everything and it looks good, I could also verify the speed improvements. 👍 There is just one thing in sort-modifiers.ts we should address before merging (#547 (comment)). Could you take a look at it?
After addressing it I consider this to be ready for merge. 🙂
|
Bench results after tweaks: OLD NEW |
|
Best of 3 with memory info from #620 main branch perf branch |
CodSpeed Performance ReportMerging #547 will improve performances by 19.21%Comparing Summary
Benchmarks breakdown
|
|
Just noting here: Size of total ESM bundle increased from 6.68 kB to 6.96 kB minified + Brotli-compressed which is a 4.2% increase. I think this is fine for the 10-20% perf boost. |
dcastil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great work, thanks!
|
This was addressed in release v3.4.0. |
Claude and I jammed on some potential improvements...
Rationale for increasing cache size is the partials it's storing are quite small and a medium-complexity website may have thousands of them. They probably won't take up much space even in volume.
Roughly a 9% improvement in the cached example and 11% overall in the heavy case.