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

fix: concise uuidToBytes #463

Closed
wants to merge 2 commits into from
Closed

fix: concise uuidToBytes #463

wants to merge 2 commits into from

Conversation

broofa
Copy link
Member

@broofa broofa commented Jun 2, 2020

Putting up two possible implementations of uuidToBytes for us to consider:

  • This PR, uuidToBytes_concise, is similar to what's on master. Slightly better perf and compactness, pretty readable.
  • PR Version 9 improvements #464, uuidToBytes_fast is @awwit's approach in v9, but with the numberToBytes stuff done inline. Yields slightly better performance and saves a few bytes when minified. Not great readability (as tends to be the case with highly optimized code), hence the extra comments.

Here are the numbers for the various flavors:

branch performance (ops/sec) size (minified+gzip bytes)1
master2 138-141K 132
v9 209-221K 247
uuidToBytes_concise 146-170K 125
uuidToBytes_fast 228-230K 183

Thoughts? Pick one, I guess. :-)

  1. "size" is for just the uuidToBytes() implementation run through jsmin | gzip -c | wc -c
  2. master implementation does not currently validate() like the other approaches mentioned here do.

@broofa broofa mentioned this pull request Jun 2, 2020
6 tasks
@broofa broofa requested a review from ctavan June 2, 2020 16:53
@broofa broofa marked this pull request as ready for review June 2, 2020 16:54
@ctavan
Copy link
Member

ctavan commented Jun 2, 2020

Tough call, I have no strong opinion 🤷‍♂️

I'm a huge fan of conciseness but I also think bundlesize is not an issue for v3/5 and performance is a promise that this library gives.

I would make good-enough test coverage the requirement for the complex fast solution (I haven't checked how good the test coverage is for this specific code path).

In the next step we could also consider exposing this method alongside bytesToUUID (#180).

@wesleytodd
Copy link

wesleytodd commented Jun 11, 2020

TBQH, the code in here is not entirely illegible, especially with the comments. If that is the only concern I would go with it for the small perf and size improvement.

And also, 👍 to exposing it or moving it out to a separate package. Either one would have solved the issue I had in #180

@broofa
Copy link
Member Author

broofa commented Jun 23, 2020

Moving forward with #464

@broofa broofa closed this Jun 23, 2020
@broofa broofa deleted the uuidToBytes_concise branch June 24, 2020 14:52
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.

3 participants