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: 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
Co-authored-by: Sukka <isukkaw@gmail.com>
@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.

# 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.