Skip to content
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

feat: enhance performance #26

Merged
merged 4 commits into from
Dec 29, 2023
Merged

feat: enhance performance #26

merged 4 commits into from
Dec 29, 2023

Conversation

jalalazimi
Copy link
Contributor

@jalalazimi jalalazimi commented Sep 8, 2020

Hi friends, I did some minor changes to increase performance. the benchmark result is:

# Strings
  classcat*    x 6,206,358 ops/sec ±1.24% (90 runs sampled)
  classnames   x 3,408,198 ops/sec ±0.88% (92 runs sampled)
  clsx (prev)  x 7,434,060 ops/sec ±0.92% (93 runs sampled)
  clsx         x 8,112,152 ops/sec ±0.90% (92 runs sampled)

# Objects
  classcat*    x 5,865,653 ops/sec ±0.89% (92 runs sampled)
  classnames   x 3,108,156 ops/sec ±0.90% (92 runs sampled)
  clsx (prev)  x 4,765,299 ops/sec ±0.83% (93 runs sampled)
  clsx         x 6,063,173 ops/sec ±0.93% (93 runs sampled)

# Arrays
  classcat*    x 5,471,773 ops/sec ±0.81% (92 runs sampled)
  classnames   x 1,811,028 ops/sec ±0.71% (94 runs sampled)
  clsx (prev)  x 5,606,905 ops/sec ±1.12% (91 runs sampled)
  clsx         x 5,710,108 ops/sec ±0.94% (92 runs sampled)

# Nested Arrays
  classcat*    x 4,333,798 ops/sec ±0.97% (90 runs sampled)
  classnames   x 1,066,391 ops/sec ±0.70% (93 runs sampled)
  clsx (prev)  x 4,351,329 ops/sec ±1.05% (93 runs sampled)
  clsx         x 4,505,775 ops/sec ±0.94% (92 runs sampled)

# Nested Arrays w/ Objects
  classcat*    x 4,524,749 ops/sec ±0.99% (92 runs sampled)
  classnames   x 1,716,114 ops/sec ±0.78% (94 runs sampled)
  clsx (prev)  x 4,239,522 ops/sec ±0.96% (91 runs sampled)
  clsx         x 4,702,464 ops/sec ±0.96% (93 runs sampled)

# Mixed
  classcat*    x 4,826,735 ops/sec ±0.89% (94 runs sampled)
  classnames   x 2,144,730 ops/sec ±0.92% (91 runs sampled)
  clsx (prev)  x 4,651,749 ops/sec ±1.04% (89 runs sampled)
  clsx         x 5,129,713 ops/sec ±0.89% (91 runs sampled)

# Mixed (Bad Data)
  classcat*    x 1,400,863 ops/sec ±0.87% (92 runs sampled)
  classnames   x 1,038,999 ops/sec ±0.81% (93 runs sampled)
  clsx (prev)  x 1,708,531 ops/sec ±0.98% (93 runs sampled)
  clsx         x 1,830,703 ops/sec ±1.03% (93 runs sampled)

Perf on MacBook Pro, 2.9 GHz Quad-Core Intel Core i7, 16 GB Ram

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2020

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6da37d6) 100.00% compared to head (8adf785) 100.00%.
Report is 12 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #26   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           22        43   +21     
=========================================
+ Hits            22        43   +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sant123
Copy link

sant123 commented Feb 4, 2021

Looks good. Could this be merged?

@doug-shontz
Copy link

Possible for this to be merged?

@uicowboy
Copy link

uicowboy commented May 5, 2021

@lukeed I bet you're really busy, but any chance to get this reviewed/merged? The PR's been open for nearly 8 months.

@karlprieb
Copy link

@lukeed any chance to get this merged? thanks for this great library :)

src/index.js Outdated Show resolved Hide resolved
@Brandontam29
Copy link

Has this been merged?

@lukeed
Copy link
Owner

lukeed commented Jul 12, 2022

Does it look merged? :D

Waiting because all these PRs have varying results depending on the minor (or patch) version when benchmarking. There’s needs to be a clear/obvious pattern before accepting and releasing a new patch for all to consume, especially with a cost of a few extra bytes

@yordis
Copy link

yordis commented Sep 14, 2023

Cashing the value avoids transversing the prototype chaining by looking for such a .length property, as some of you know.

@lukeed I know you said, "It doesn't matter": #28 (comment)

I tend to agree with you.

Except, it is such simple things to change without trading off complexity that I would take the code snippet from #28

I am biased towards taking the "improvements."

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@lukeed
Copy link
Owner

lukeed commented Dec 29, 2023

The reason for the delay is that the code changes performed worse (if anything) for most of the recent Node/V8 versions due to the change in while->for loop. Caching the length had no noticeable effect for the majority of this time. (These things often come & go)

Merging now only because there's reproducible effect in the latest 20.x track.
It's an increase of 5 bytes (2%) for an increase of -2% to 6% performance change. Numbers below. It's worth the trade-off only because the 6% perf gain is seen in the Strings case which is how nearly everyone uses clsx.

Thanks for the PR & patience @jalalazimi. However if you plan to keep your project you're currently in violation of MIT license. The source is near identical & there's no acknowledgement nor attribution anywhere. It's most clearly a fork & should be labeled as such.

Benchmark Results

Node 20.8.1

# Strings
  classnames   x  6,576,471 ops/sec ±0.38% (95 runs sampled)
  clsx (pr26)  x 12,933,752 ops/sec ±0.11% (95 runs sampled)
  clsx (main)  x 12,142,343 ops/sec ±0.15% (99 runs sampled)

# Objects
  classnames   x 6,318,942 ops/sec ±0.12% (99 runs sampled)
  clsx (pr26)  x 9,473,723 ops/sec ±0.10% (100 runs sampled)
  clsx (main)  x 9,466,986 ops/sec ±0.10% (99 runs sampled)

# Arrays
  classnames   x 3,545,087 ops/sec ±0.22% (99 runs sampled)
  clsx (pr26)  x 9,444,148 ops/sec ±0.09% (102 runs sampled)
  clsx (main)  x 9,118,411 ops/sec ±0.17% (96 runs sampled)

# Nested Arrays
  classnames   x 2,072,197 ops/sec ±0.20% (98 runs sampled)
  clsx (pr26)  x 7,087,216 ops/sec ±0.32% (98 runs sampled)
  clsx (main)  x 7,202,070 ops/sec ±0.52% (99 runs sampled)

# Nested Arrays w/ Objects
  classnames   x 3,155,991 ops/sec ±0.40% (94 runs sampled)
  clsx (pr26)  x 7,312,535 ops/sec ±0.50% (95 runs sampled)
  clsx (main)  x 7,121,246 ops/sec ±0.27% (100 runs sampled)

# Mixed
  classnames   x 4,163,925 ops/sec ±0.28% (97 runs sampled)
  clsx (pr26)  x 8,086,030 ops/sec ±0.40% (98 runs sampled)
  clsx (main)  x 7,921,147 ops/sec ±0.20% (97 runs sampled)

# Mixed (Bad Data)
  classnames   x 1,909,345 ops/sec ±0.08% (101 runs sampled)
  clsx (pr26)  x 2,973,733 ops/sec ±0.19% (99 runs sampled)
  clsx (main)  x 2,964,893 ops/sec ±0.10% (101 runs sampled)

@lukeed lukeed merged commit deff09b into lukeed:master Dec 29, 2023
8 checks passed
@jalalazimi
Copy link
Contributor Author

Thank you, @lukeed, for your prompt response; I really appreciate your support. I'll make sure to add a note explaining the purpose of my project and update the benchmark results accordingly.

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

Successfully merging this pull request may close these issues.