Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feat: Performance enhancements #281

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

fveracoechea
Copy link

@fveracoechea fveracoechea commented Apr 19, 2024

Description

Implement code optimizations on CVA to increase execution speed and reduce the number of operations, such as iterations and object copying.

Additional context

Let's say we have a web page made of React components, where most components utilize CVA.
Since React components render and re-render often, it's important to keep CVA's runtime overhead as low as possible. This optimization will enhance scalability, especially on large projects.

Benchmark results

pnpx ts-node ./packages/cva/src/benchmark.ts
(index) branch name ops/sec average (ms) samples
0 feat/performance-enhancement 440297 0.002 2201487
1 cva/main 125161 0.008 625810 3.52 x slower

image

All tests passing with no problems:

image

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Follow the Style Guide.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).

Copy link

vercel bot commented Apr 19, 2024

@fveracoechea is attempting to deploy a commit to the cva Team on Vercel.

A member of the Team first needs to authorize it.

@fveracoechea fveracoechea changed the title Feat/performance enhancements Feat: Performance enhancements Apr 19, 2024
@fveracoechea fveracoechea marked this pull request as draft April 19, 2024 19:07
@fveracoechea fveracoechea marked this pull request as ready for review April 19, 2024 19:07
@joe-bell
Copy link
Owner

This is super thoughtful of you, thanks @fveracoechea 🙏❤️

Fairly busy at the moment and this will take some time to review, so don't be too disheartened if it takes some time to get around to this! Really appreciate it

@fveracoechea
Copy link
Author

Thanks @joe-bell 🙏 let me know if you have any suggestions or concerns, I’d be happy to help!

@gperdomor

This comment was marked as spam.

@joe-bell

This comment was marked as off-topic.

@gperdomor

This comment was marked as off-topic.

@joe-bell
Copy link
Owner

Hey @fveracoechea, just wanted to give you a quick update rather than leave you in the dark here!

I'm starting to pick up cva@1.0 tasks again at the moment, which means there's quite a few moving parts in flight

For now, I'm going to hold off merging this simply because it's a little easier to work with the code I'm familiar with

That said, I'm keeping this PR open as I don't want to discourage you from introducing your improvements – which are incredibly valuable! I'll keep you posted on cva@1.0s progress and let you know when's a good time to revisit this 🙏

@fveracoechea
Copy link
Author

fveracoechea commented Feb 9, 2025

Thanks @joe-bell, I appreciate the update, it totally makes sense, I'll be happy to revisit it when the timing works better.

Copy link

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

🚮 Removed packages: npm/react@18.2.0, npm/sharp@0.33.5

View full report↗︎

Copy link
Author

@fveracoechea fveracoechea Feb 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to mention, this file is just for benchmarking purposes, basically making sure these changes are actually faster, could be removed before merging.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants