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

chore: optimize uuid.v4 by 4.3x (430%) (internal stringify by 33x) #597

Merged
merged 2 commits into from Dec 2, 2021

Conversation

Rush
Copy link
Contributor

@Rush Rush commented Nov 24, 2021

It turns out that validation in uuidStringify() is slowing it down by 33x. Since uuidv1() and uuidv4() produce valid buffers by the virtue of their algorithm they can safely use a validation-free version of stringify.

Benchmark changes

uuidv1() - faster by 1.3x
uuidv4() - faster by 4.3x
the gains are due to .unsafeStringify() with no validation (as it is not required) is 33x faster than the version with validation.

The public uuidStringify() has been left unchanged but my recommendation would be to also remove validation from this method in the future and just export the unsafe version.

Benchmark (Core i9-10900K)

uuidStringify() x 2,957,961 ops/sec ±0.78% (86 runs sampled)
uuidParse() x 3,331,671 ops/sec ±0.59% (93 runs sampled)
---

uuidv1() x 3,185,198 ops/sec ±0.54% (91 runs sampled)
uuidv1() fill existing array x 9,987,064 ops/sec ±0.21% (95 runs sampled)
uuidv4() x 6,610,790 ops/sec ±0.77% (94 runs sampled)
uuidv4() fill existing array x 7,085,027 ops/sec ±0.68% (96 runs sampled)
uuidv3() x 385,764 ops/sec ±1.00% (89 runs sampled)
uuidv5() x 424,133 ops/sec ±1.43% (89 runs sampled)
Fastest is uuidv1() fill existing array

Benchmark prior to changes

Starting. Tests take ~1 minute to run ...
uuidStringify() x 2,901,140 ops/sec ±0.61% (93 runs sampled)
uuidParse() x 3,279,020 ops/sec ±0.38% (88 runs sampled)
---

uuidv1() x 2,328,848 ops/sec ±0.60% (90 runs sampled)
uuidv1() fill existing array x 9,944,353 ops/sec ±0.26% (93 runs sampled)
uuidv4() x 1,677,495 ops/sec ±0.35% (90 runs sampled)
uuidv4() fill existing array x 6,861,270 ops/sec ±0.36% (91 runs sampled)
Fastest is uuidv1() fill existing array

@Rush Rush changed the title chore: Optimize stringify by 30x (3000%) and optimize uuid.v4 by 4x (400%) chore: Optimize stringify by 33x (3300%) and optimize uuid.v4 by 4.3x (430%) Nov 24, 2021
@Rush
Copy link
Contributor Author

Rush commented Nov 24, 2021

I left the version with validation as it may be useful for development. Please let me know if it should be removed.

@LinusU
Copy link
Member

LinusU commented Nov 24, 2021

The new safeStringify no longer validates that the version is between 0-5, or that version 0 consists only of zeros, as the previous stringify does.

How about keeping the public stringify as it was before, but have an internal unsafeStringify that we use internally from v1/v35/v4? Since there we know that the bytes are valid.

That way this won't be a breaking change either, so it can go out in a regular release.

@Rush
Copy link
Contributor Author

Rush commented Nov 24, 2021

All right, but I still find the check's usefulness questionable. It is not a very strict check, it's just checking appearances. A more proper test would validate the input buffer for proper length and version even prior to stringifying.

@Rush Rush changed the title chore: Optimize stringify by 33x (3300%) and optimize uuid.v4 by 4.3x (430%) chore: optimize uuid.v4 by 4.3x (430%) (internal stringify by 33x) Nov 24, 2021
@Rush Rush force-pushed the optimize-v4-and-stringify branch 4 times, most recently from cb6396b to 4327216 Compare November 24, 2021 18:12
@Rush
Copy link
Contributor Author

Rush commented Nov 24, 2021

Updated PR according to your suggestions. What's maybe is a missed opportunity is not exposing the validation-free stringify function. Any idea what are the use cases for users using this public .stringify() method?

@Rush
Copy link
Contributor Author

Rush commented Nov 27, 2021

Is there anything else I should change?

package.json Outdated Show resolved Hide resolved
src/stringify.js Outdated Show resolved Hide resolved
test/unit/stringify.test.js Outdated Show resolved Hide resolved
@Rush
Copy link
Contributor Author

Rush commented Nov 28, 2021

Addressed. Thank you for the review.

@broofa broofa mentioned this pull request Nov 28, 2021
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Neat! 👍

@broofa broofa added this to Done in Version 9 Dec 1, 2021
@broofa broofa moved this from Done to In progress in Version 9 Dec 1, 2021
@broofa
Copy link
Member

broofa commented Dec 1, 2021

@ctavan Do you know why the bundlewatch check isn't running here?

@ctavan
Copy link
Member

ctavan commented Dec 2, 2021

@ctavan Do you know why the bundlewatch check isn't running here?

The browser tests and bundlewatch both use API keys (for Browserstack and bundlewatch respectively) and therefore don't run on PRs created from branches of forks. They only run on branches of this very repo.

The reason is that it would otherwise be trivial to extract those secrets (just console.log(process.env.API_KEY)).

I haven't checked in a while if there is a solution for this problem by now. But until then we can also just merge and observe the checks on main.

@LinusU
Copy link
Member

LinusU commented Dec 2, 2021

Bundlewatch report for v8.3.2-14-g577c840 (this PR):

PASS ./examples/browser-rollup/dist/v1-size.js: 802B < 1KB (gzip)
PASS ./examples/browser-rollup/dist/v3-size.js: 1.96KB < 2.1KB (gzip)
PASS ./examples/browser-rollup/dist/v4-size.js: 522B < 716B (gzip)
PASS ./examples/browser-rollup/dist/v5-size.js: 1.36KB < 1.5KB (gzip)
PASS ./examples/browser-webpack/dist/v1-size.js: 859B < 1KB (gzip)
PASS ./examples/browser-webpack/dist/v3-size.js: 1.96KB < 2.1KB (gzip)
PASS ./examples/browser-webpack/dist/v4-size.js: 552B < 716B (gzip)
PASS ./examples/browser-webpack/dist/v5-size.js: 1.35KB < 1.5KB (gzip)

Bundlewatch report for v8.3.2-12-gc9e076c (main):

PASS ./examples/browser-rollup/dist/v1-size.js: 912B < 1KB (gzip)
PASS ./examples/browser-rollup/dist/v3-size.js: 1.99KB < 2.1KB (gzip)
PASS ./examples/browser-rollup/dist/v4-size.js: 624B < 716B (gzip)
PASS ./examples/browser-rollup/dist/v5-size.js: 1.39KB < 1.5KB (gzip)
PASS ./examples/browser-webpack/dist/v1-size.js: 905B < 1KB (gzip)
PASS ./examples/browser-webpack/dist/v3-size.js: 1.99KB < 2.1KB (gzip)
PASS ./examples/browser-webpack/dist/v4-size.js: 589B < 716B (gzip)
PASS ./examples/browser-webpack/dist/v5-size.js: 1.39KB < 1.5KB (gzip)

Summary:

  v8.3.2-12-gc9e076c v8.3.2-14-g577c840
rollup/v1-size.js 912B 802B
rollup/v3-size.js 1.99KB 1.96KB
rollup/v4-size.js 624B 522B
rollup/v5-size.js 1.39KB 1.36KB
webpack/v1-size.js 905B 859B
webpack/v3-size.js 1.99KB 1.96KB
webpack/v4-size.js 589B 552B
webpack/v5-size.js 1.39KB 1.35KB

Lower bundle sizes overall 🙌

@ctavan ctavan requested a review from broofa December 2, 2021 09:58
@ctavan
Copy link
Member

ctavan commented Dec 2, 2021

@broofa are you fine with pulling this in now?

@broofa
Copy link
Member

broofa commented Dec 2, 2021

@ctavan image

@ctavan ctavan merged commit 3a033f6 into uuidjs:main Dec 2, 2021
@ctavan
Copy link
Member

ctavan commented Dec 2, 2021

Thank you for this great contribution, @Rush! 🎉

@ctavan ctavan moved this from In progress to Done in Version 9 Dec 2, 2021
@Rush
Copy link
Contributor Author

Rush commented Dec 2, 2021

It's my pleasure. Thank you for creating and maintaining this project. 💯

@andrejewski
Copy link

It would be great to expose this unsafeStringify so others can use it on trusted inputs as well; an export is used here but I can't import the code; when I do I get:

node
> require('uuid/dist/stringify')
Uncaught:
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './dist/stringify' is not defined by "exports" in ~/my-project/node_modules/uuid/package.json
> node -v
v15.14.0

@broofa
Copy link
Member

broofa commented Dec 24, 2021

@andrejewski What's your use case for this? Unless stringify() is being called 1M's/second the perf gain of unsafeStringify() is unlikely matter.

@Rush
Copy link
Contributor Author

Rush commented Dec 24, 2021

Same as calling uuid 1M times per second :) I even think that the unsafe one should be the default.

@andrejewski
Copy link

@broofa I happen to have my UUIDs represented as UInt8Array wrapped in a simple class type:

class UUID {
  readonly bytes: Readonly<Uint8Array>
}

When this class is constructed I'm always validating the array is of the right size, so when I go to stringify I don't need to run the additional validation. UUIDs are quite pervasive in my applications, and I definitely have not benchmarked anything, but it is wonky to pay the validation cost twice.

@Rush I disagree that the unsafe version should be the default. String types are very pervasive in JS codebases and it's all too easy to pass an arbitrary string when only a UUID is expected. The type error can catch these bugs sooner. On the flip size, passing around a UUID class instance like above is quite safe and less likely to be misused. For folks that really care about eking out performance gains (ourselves in this case), explicitly opting in and understanding the trade-offs is the better choice.

@broofa
Copy link
Member

broofa commented Dec 30, 2021

@andrejewski The more I think about this, the less inclined I am to expose unsafeStringify(). It's generally not a good idea to incorporate these types of perf optimizations into the public API.

Since you already have a UUID class, it might make sense for you to inline your preferred stringify implementation in your class. (Feel free to copy the unsafeStringify() code for that if you'd like)

@Rush
Copy link
Contributor Author

Rush commented Dec 30, 2021

@andrejewski It's a PITA but since there is a lot of opposition to the idea of exposing the optimized version of stringify maybe you can just copy & paste the stringify function to your codebase and be done with it.

Not sure why we would make a statement that doing stringification a million times per second is only reserved to uuid internals and not to other use cases. I can imagine a dozen use-cases which could benefit from it.

Especially since the optimized version is 30x, yes 30 times faster.

@broofa
Copy link
Member

broofa commented Dec 30, 2021

@Rush You asked previously what the use cases for stringify() were. I don't have good data on that. Hence why I'm asking you about your case(s).

For what it's worth, part of my reticence here is that I've been down this path before. If you look at the git history of this project, you'll see code and docs for parse() and unparse() methods that were included in version 1. We ended up removing those from the API, however, because allowing invalid UUIDs to be generated just felt sloppy. Especially given the core premise of this module, which is that it is a rigorous implementation of RFC4122.

Thus, when we added the new methods back in as part of version 8 we made the decision to be strict about enforcing RFC compliance on inputs. Hence why stringify() throws.

Not sure why we would make a statement that doing stringification a million times per second is only reserved to uuid internals and not to other use cases.

It's a maintenance burden thing. As long as this method is internal we're free to change things as we see fit. But once it's made public, we're obligated to support it, even if we find a better solution. For example, we could inline the validation in stringify() with something like this ...

if (arr.length - offset < 16) throw('Not enough values in array')
if ((arr[offset + 6] >>> 4) & 0x07 > 5) throw ('Invalid version')
if (arr[offset+8] & 0xc0 !== 0x80) throw ('Invalid variant')

... and probably get comparable performance to unsafeStringify(). What then? If unsafeStringify() is public, we're stuck having to support it until we have a deprecation strategy. It's a pain in the ass I don't particularly want unless there's a good reason for it.

Especially since the optimized version is 30x, yes 30 times faster.

... but that's only in the tightest possible loop. As soon as you do anything interesting inside that loop - a network request, a db query, DOM manipulation, file system I/O, basically anything that makes a UUID useful - the loop time increases by 100X and that "30X" becomes "1.03X".

In absolute terms, unsafeStringify() is trimming ~0.3 µ-seconds off each stringify() call. Something like that. You'd have to call stringify() at least 33,000 times/second to see a noticeable (read, "> 1%") perf improvement? Any slower and it's just not going to matter.

I can imagine a dozen use-cases which could benefit from it.

Please share details of those, because I can't think of any.

For example, really big, high-traffic systems like Amazon, Walmart, Facebook... even if you're generating a UUID for every user action, every comment, every purchase, you're talking about maybe 100,000-1,000,000 operations/sec. But that load is spread across 100K's of multi-core servers. The per-core rate is going to be ≪ 1,000 ops/second.

It's not just about the ops/second rate. You have to sustain that rate long enough for it to matter. Calling stringify() on 10 million UUIDs (say, as part of a DB migration) is only going to take an extra 3 seconds over unsafeStringify(). 🤷

@uuidjs uuidjs deleted a comment Jan 12, 2022
@Rush
Copy link
Contributor Author

Rush commented Jun 14, 2022

Am I correct that this hasn't been released yet?

oshi97 added a commit to yext/studio that referenced this pull request Sep 9, 2022
…ts (#37)

There was a breaking change in ts-morph 16 that broke the ts build,
so kept it at 15 dsherret/ts-morph#1325
uuid v9 speedup seems cool uuidjs/uuid#597

TEST=manual

started up the app, ran jest tests
Vylpes pushed a commit to Vylpes/Droplet that referenced this pull request Sep 14, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [uuid](https://github.com/uuidjs/uuid) | dependencies | major | [`^8.3.2` -> `^9.0.0`](https://renovatebot.com/diffs/npm/uuid/8.3.2/9.0.0) |
| [@types/uuid](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/uuid) ([source](https://github.com/DefinitelyTyped/DefinitelyTyped)) | dependencies | major | [`^8.3.0` -> `^9.0.0`](https://renovatebot.com/diffs/npm/@types%2fuuid/8.3.0/9.0.1) |

---

### Release Notes

<details>
<summary>uuidjs/uuid</summary>

### [`v9.0.0`](https://github.com/uuidjs/uuid/blob/HEAD/CHANGELOG.md#&#8203;900-httpsgithubcomuuidjsuuidcomparev832v900-2022-09-05)

[Compare Source](uuidjs/uuid@v8.3.2...v9.0.0)

##### ⚠ BREAKING CHANGES

-   Drop Node.js 10.x support. This library always aims at supporting one EOLed LTS release which by this time now is 12.x which has reached EOL 30 Apr 2022.

-   Remove the minified UMD build from the package.

    Minified code is hard to audit and since this is a widely used library it seems more appropriate nowadays to optimize for auditability than to ship a legacy module format that, at best, serves educational purposes nowadays.

    For production browser use cases, users should be using a bundler. For educational purposes, today's online sandboxes like replit.com offer convenient ways to load npm modules, so the use case for UMD through repos like UNPKG or jsDelivr has largely vanished.

-   Drop IE 11 and Safari 10 support. Drop support for browsers that don't correctly implement const/let and default arguments, and no longer transpile the browser build to ES2015.

    This also removes the fallback on msCrypto instead of the crypto API.

    Browser tests are run in the first supported version of each supported browser and in the latest (as of this commit) version available on Browserstack.

##### Features

-   optimize uuid.v1 by 1.3x uuid.v4 by 4.3x (430%) ([#&#8203;597](uuidjs/uuid#597)) ([3a033f6](uuidjs/uuid@3a033f6))
-   remove UMD build ([#&#8203;645](uuidjs/uuid#645)) ([e948a0f](uuidjs/uuid@e948a0f)), closes [#&#8203;620](uuidjs/uuid#620)
-   use native crypto.randomUUID when available ([#&#8203;600](uuidjs/uuid#600)) ([c9e076c](uuidjs/uuid@c9e076c))

##### Bug Fixes

-   add Jest/jsdom compatibility ([#&#8203;642](uuidjs/uuid#642)) ([16f9c46](uuidjs/uuid@16f9c46))
-   change default export to named function ([#&#8203;545](uuidjs/uuid#545)) ([c57bc5a](uuidjs/uuid@c57bc5a))
-   handle error when parameter is not set in v3 and v5 ([#&#8203;622](uuidjs/uuid#622)) ([fcd7388](uuidjs/uuid@fcd7388))
-   run npm audit fix ([#&#8203;644](uuidjs/uuid#644)) ([04686f5](uuidjs/uuid@04686f5))
-   upgrading from uuid3 broken link ([#&#8203;568](uuidjs/uuid#568)) ([1c849da](uuidjs/uuid@1c849da))

##### build

-   drop Node.js 8.x from babel transpile target ([#&#8203;603](uuidjs/uuid#603)) ([aa11485](uuidjs/uuid@aa11485))

-   drop support for legacy browsers (IE11, Safari 10) ([#&#8203;604](uuidjs/uuid#604)) ([0f433e5](uuidjs/uuid@0f433e5))

-   drop node 10.x to upgrade dev dependencies ([#&#8203;653](uuidjs/uuid#653)) ([28a5712](uuidjs/uuid@28a5712)), closes [#&#8203;643](uuidjs/uuid#643)

##### [8.3.2](uuidjs/uuid@v8.3.1...v8.3.2) (2020-12-08)

##### Bug Fixes

-   lazy load getRandomValues ([#&#8203;537](uuidjs/uuid#537)) ([16c8f6d](uuidjs/uuid@16c8f6d)), closes [#&#8203;536](uuidjs/uuid#536)

##### [8.3.1](uuidjs/uuid@v8.3.0...v8.3.1) (2020-10-04)

##### Bug Fixes

-   support expo>=39.0.0 ([#&#8203;515](uuidjs/uuid#515)) ([c65a0f3](uuidjs/uuid@c65a0f3)), closes [#&#8203;375](uuidjs/uuid#375)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4yIiwidXBkYXRlZEluVmVyIjoiMzQuNzQuMiJ9-->

Co-authored-by: Renovate Bot <renovate@vylpes.com>
Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/Droplet/pulls/111
Reviewed-by: Vylpes <ethan@vylpes.com>
Co-authored-by: RenovateBot <renovate@vylpes.com>
Co-committed-by: RenovateBot <renovate@vylpes.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants