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

Much faster implementation #466

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Much faster implementation #466

merged 4 commits into from
Feb 1, 2024

Conversation

samuelgozi
Copy link
Contributor

@samuelgozi samuelgozi commented Jan 31, 2024

I know this is a lazy PR since I didn't check other files to make sure I'm making the correct changes, so I'm sorry about that.
This implementation is much faster.
The benchmark checks the time to generate one million nanoids.

$ node ./main.js
Old 1365ms
New 948ms

$ bun ./main.js
Old 462ms
New 138ms

I hope you find it useful 🙏

Much faster now
@ai
Copy link
Owner

ai commented Jan 31, 2024

  1. It is the file generated from index.browser.js. You need to fix it first.
  2. But browser's file should be tested in browsers. For Node/Bun we have another file index.js which has different optimizations.

@samuelgozi
Copy link
Contributor Author

samuelgozi commented Jan 31, 2024

Tested on latest version:

Chrome

Old 1634ms
New 1245ms

Safari

Old 1208ms
New 760ms

Firefox

Old 1207ms
New 742ms

Browser support doesn't change at all.
I will update the source file.

@samuelgozi
Copy link
Contributor Author

I now updated the right place.

nanoid.js Outdated Show resolved Hide resolved
index.browser.js Outdated Show resolved Hide resolved
index.browser.js Outdated Show resolved Hide resolved
@samuelgozi
Copy link
Contributor Author

Ok i will fix everything you mentioned.
In regards to the use of the string inside the function, I don't think we should hoist it manually since I didn't see any effect on performance, but I do think that this way it looks nicer.

Maybe I should remove the comment instead since it not important

@ai
Copy link
Owner

ai commented Jan 31, 2024

OK, let's keep string as is. I will do some optimizations later for smaller bundle size.

@samuelgozi
Copy link
Contributor Author

Done. Package size limit has exceeded by 34 B

@ai
Copy link
Owner

ai commented Feb 1, 2024

Package size limit has exceeded by 34 B

Try to import alphabet from https://github.com/ai/nanoid/blob/main/url-alphabet/index.js

We have a fun hack there, we put the same letters not in alhabetical order, but in order for better gzip.

And please update packge→size-limit for new size (so we will track size changes by commit).

@samuelgozi
Copy link
Contributor Author

Package size limit has exceeded by 15 B
TIL, very cool!

Made the changes

@ai ai merged commit 70409a3 into ai:main Feb 1, 2024
4 checks passed
@ai
Copy link
Owner

ai commented Feb 1, 2024

I also reduced 8 bytes by reusing size variable 4db3464

@samuelgozi
Copy link
Contributor Author

Using the while loop is actually a little bit faster.
Very nice idea!

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.

2 participants