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: slightly improved v1 perf #453

Merged
merged 4 commits into from
May 25, 2020
Merged

Conversation

awwit
Copy link
Contributor

@awwit awwit commented May 21, 2020

Hello!

I managed to increase the uuidv1 generation performance by replacing [] to new Array(16).

I also added an error code when throwing an exception for exceeding v1 generation limit per second.
I would like to add error codes to all your exceptions. To make it easier to identify the error by its code (instead of a message).
But we need to work on the names of the error codes. We need to standardize names (or somewhere to look).

If at all you like the idea with error codes =)
If not, then I can remove them.

Benchmark master:

uuidv1() x 1,634,946 ops/sec ±0.82% (90 runs sampled)
uuidv1() fill existing array x 7,727,514 ops/sec ±0.63% (92 runs sampled)
uuidv4() x 449,289 ops/sec ±0.63% (95 runs sampled)
uuidv4() fill existing array x 463,831 ops/sec ±0.70% (92 runs sampled)
uuidv3() x 144,401 ops/sec ±0.86% (87 runs sampled)
uuidv5() x 147,395 ops/sec ±1.17% (88 runs sampled)

Benchmark this branch:

uuidv1() x 1,723,775 ops/sec ±1.21% (92 runs sampled)
uuidv1() fill existing array x 7,763,768 ops/sec ±0.98% (91 runs sampled)
uuidv4() x 425,037 ops/sec ±0.49% (93 runs sampled)
uuidv4() fill existing array x 444,126 ops/sec ±0.52% (95 runs sampled)
uuidv3() x 142,065 ops/sec ±0.85% (88 runs sampled)
uuidv5() x 142,814 ops/sec ±1.04% (88 runs sampled)

Copy link
Member

@ctavan ctavan left a comment

Choose a reason for hiding this comment

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

Thanks for the perf improvement. Looks like a good opportunity to finally harmonize the signatures of all 3 versions: We should always use Uint8Array(16) if no buffer is passed and we should finally get rid of the binary v4 function signature overloading, see #437 .

Regarding error codes: We only have a total of 4 different errors in the entire code base. I believe it is overkill and not worth the added complexity to introduce error codes. After all, 3 of the 4 errors are thrown for usage errors (bad v3/v5 parameters and the lack of a secure RNG), and only the one you are touching here can really happen at runtime in a setup where uuid is being used correctly. I'd prefer removing that added complexity again.

src/bytesToUuid.js Outdated Show resolved Hide resolved
src/v1.js Show resolved Hide resolved
Comment on lines 24 to 30
try {
uuidv1(null, array, 0);
} catch (err) {
if (err.code !== 'UUID_V1_LIMIT_PER_SEC') {
throw err;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Revert?

Why is this needed? Are we hitting the rate limit with our benchmark? If so, that's kind of cool... but it also invalidates the benchmark so should probably be abort()ed with an appropriate message.

Note: Silently dropping errors in catch blocks is a very strong code smell. At a minimum, there should be a comment explaining the rationale for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@broofa periodically the limit is exceeded on my machine and an exception is thrown.
This is strange, but the block try/catch does not affect the performance %)
If an exception is thrown, the following tests will fail.

In new versions of nodejs (v13.14.0), performance is even higher. Exceptions are thrown more often.

Copy link
Member

@broofa broofa May 23, 2020

Choose a reason for hiding this comment

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

periodically the limit is exceeded on my machine

This is surprising. It's the first time I've seen this limit hit in the real world. (I added that rate check years ago to comply with the spec, never really expecting to hit it in practice.) What hardware are you running on if I may ask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@broofa I have my own computer on which I run tests (nodejs v12.16.3):

CPU: AMD Ryzen 7 1700
RAM: 32 Gb 2999 MHz

Typically, an exception occurs the first few times the benchmark is launched. And then the testing goes fine. Maybe it's a bug. (With try/catch I never get a performance close or higher to 10M)

The exception is much more likely to occur on new versions of nodejs (v13). Because the overall performance of the library is increasing.

On nodejs v13 I usually get performance: uuidv1() fill existing array x 8,777,105 ops/sec ±0.82% (96 runs sampled).

src/v1.js Outdated
Comment on lines 75 to 77
const error = new Error("uuid.v1(): Can't create more than 10M uuids/sec");
error.code = 'UUID_V1_LIMIT_PER_SEC';
throw error;
Copy link
Member

Choose a reason for hiding this comment

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

Revert.

code is a non-standard error property, and so better to avoid. Yes, other modules do this but typically it's for the purpose of forwarding codes in some underlying API.

If this is needed (which I'm on the fence about, and 'would like to get @ctavan's opinion here) I'd suggest something along these lines...


// Create minimal Error subclass (Define in UuidRateError.js file so it can be exported as part of src/index.js?)
export class UuidRateError extends Error {}

// ... Throw in this file when rate exceeded ...
throw new UuidRateError(`UUID creation rate limit exceeded`)

// ... use `instanceof` elsewhere to detect... 
import {v1 as uuidv1, UuidRateError} from 'uuid'
// ...
try {
  uuidv1()
} catch (err) {
  if (err instanceof UuidRateError) // etc...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@broofa it will be harder for library users. They will need to guess that non-standard exceptions are thrown. When it is already accepted in nodejs to identify an error by code.

https://nodejs.org/api/errors.html#errors_error_code

Copy link
Member

Choose a reason for hiding this comment

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

Huh... I wasn't aware code was used by node core. Interesting.

src/v1.js Outdated
@@ -70,7 +72,9 @@ function v1(options, buf, offset) {

// Per 4.2.1.2 Throw error if too many uuids are requested
if (nsecs >= 10000) {
throw new Error("uuid.v1(): Can't create more than 10M uuids/sec");
const error = new Error("uuid.v1(): Can't create more than 10M uuids/sec");
error.code = 'UUID_V1_LIMIT_PER_SEC';
Copy link
Member

Choose a reason for hiding this comment

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

If we keep this, I suggest to align it with Node.js error code naming and call ist something like ERR_UUID_V1_RATE_EXCEEDED.

Since, as explained below, this is the only runtime error of this library, I would prefer to revert this for now (or extract this into a separate PR so we can merge the perf optimization first and think about error codes a bit more).

@awwit awwit requested review from ctavan and broofa May 25, 2020 12:27
examples/benchmark/benchmark.js Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
@awwit awwit requested a review from ctavan May 25, 2020 15:45
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