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.

Sign up for GitHub

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

feat: slightly improved v1 perf #453

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a361e29
feat: slightly improved v1 perf
awwit May 21, 2020
056d3e3
revert: throw original Error
awwit May 25, 2020
1b158ff
test: added comment for v1 in benchmark
awwit May 25, 2020
45b85d4
Merge branch 'master' into improve/v1-perf
ctavan May 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion examples/benchmark/benchmark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ suite
uuidv1();
})
.add('uuidv1() fill existing array', function () {
uuidv1(null, array, 0);
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 •
edited
Loading

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

})
.add('uuidv4()', function () {
uuidv4();
Expand Down
42 changes: 19 additions & 23 deletions src/bytesToUuid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,40 +2,36 @@
* Convert array of 16 byte values to UUID string format of the form:
* XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
*/
const byteToHex = [];
const bth = [];
awwit marked this conversation as resolved.
Show resolved Hide resolved

for (let i = 0; i < 256; ++i) {
byteToHex.push((i + 0x100).toString(16).substr(1));
bth.push((i + 0x100).toString(16).substr(1));
}

function bytesToUuid(buf, offset) {
const i = offset || 0;

const bth = byteToHex;

function bytesToUuid(buf, offset = 0) {
// Note: Be careful editing this code! It's been tuned for performance
// and works in ways you may not expect. See https://github.com/uuidjs/uuid/pull/434
return (
bth[buf[i + 0]] +
bth[buf[i + 1]] +
bth[buf[i + 2]] +
bth[buf[i + 3]] +
bth[buf[offset + 0]] +
bth[buf[offset + 1]] +
bth[buf[offset + 2]] +
bth[buf[offset + 3]] +
'-' +
bth[buf[i + 4]] +
bth[buf[i + 5]] +
bth[buf[offset + 4]] +
bth[buf[offset + 5]] +
'-' +
bth[buf[i + 6]] +
bth[buf[i + 7]] +
bth[buf[offset + 6]] +
bth[buf[offset + 7]] +
'-' +
bth[buf[i + 8]] +
bth[buf[i + 9]] +
bth[buf[offset + 8]] +
bth[buf[offset + 9]] +
'-' +
bth[buf[i + 10]] +
bth[buf[i + 11]] +
bth[buf[i + 12]] +
bth[buf[i + 13]] +
bth[buf[i + 14]] +
bth[buf[i + 15]]
bth[buf[offset + 10]] +
bth[buf[offset + 11]] +
bth[buf[offset + 12]] +
bth[buf[offset + 13]] +
bth[buf[offset + 14]] +
bth[buf[offset + 15]]
).toLowerCase();
}

Expand Down
8 changes: 6 additions & 2 deletions src/v1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ let _lastNSecs = 0;
// See https://github.com/uuidjs/uuid for API details
function v1(options, buf, offset) {
let i = (buf && offset) || 0;
const b = buf || [];
const b = buf || new Array(16);
ctavan marked this conversation as resolved.
Show resolved Hide resolved

options = options || {};
let node = options.node || _nodeId;
Expand All @@ -27,6 +27,7 @@ function v1(options, buf, offset) {
// system entropy. See #189
if (node == null || clockseq == null) {
const seedBytes = options.random || (options.rng || rng)();

if (node == null) {
// Per 4.5, create and 48-bit node id, (47 random bits + multicast bit = 1)
node = _nodeId = [
Expand All @@ -38,6 +39,7 @@ function v1(options, buf, offset) {
seedBytes[5],
];
}

if (clockseq == null) {
// Per 4.2.2, randomize (14 bit) clockseq
clockseq = _clockseq = ((seedBytes[6] << 8) | seedBytes[7]) & 0x3fff;
Expand Down Expand Up @@ -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).

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.

}

_lastMSecs = msecs;
Expand Down