feat: improved v35 performance #439

awwit · 2020-05-06T23:25:58Z

For v35, I did a faster parsing of uuid value. This greatly affected the performance.
When parsing a value, it is also validated.

But the bundle size is slightly exceeded…

t’s up to you to decide if this performance increase (and uuid validation) is worth increasing build size.

(Your current solution does not validate uuid strings)

Benchmark master:

uuidv1() x 1,644,613 ops/sec ±1.21% (91 runs sampled)
uuidv1() fill existing array x 7,209,045 ops/sec ±0.82% (91 runs sampled)
uuidv4() x 432,989 ops/sec ±0.91% (92 runs sampled)
uuidv4() fill existing array x 454,731 ops/sec ±0.74% (93 runs sampled)
uuidv3() x 137,231 ops/sec ±0.78% (86 runs sampled)
uuidv5() x 135,756 ops/sec ±1.27% (85 runs sampled)

Benchmark this branch:

uuidv1() x 1,645,002 ops/sec ±1.09% (89 runs sampled)
uuidv1() fill existing array x 7,294,826 ops/sec ±0.82% (92 runs sampled)
uuidv4() x 457,979 ops/sec ±0.76% (90 runs sampled)
uuidv4() fill existing array x 466,131 ops/sec ±0.76% (91 runs sampled)
uuidv3() x 253,029 ops/sec ±1.01% (83 runs sampled)
uuidv5() x 254,426 ops/sec ±1.22% (84 runs sampled)

awwit · 2020-05-06T23:30:07Z

Bundlewatch result: https://ja2r7.app.goo.gl/R7EhcSrDJemj7yhk6

Please read PR description ^

ctavan

Since the bundlesize of neither v1 nor v4 change I would be fine accepting the increase in bundlesize.

However I'm currently a bit undecided whether the increase in code complexity is really worth the improved performance. v3/v5 are used very rarely and we have never received any reports about perf issues with them.

I'm tempted to prefer a concise implementation over performance optimizations in this case.

On the other hand we have good test coverage and can be pretty sure that the code behaves well… 🤷‍♂️

@broofa @LinusU opinions?

ctavan · 2020-05-07T07:26:15Z

src/v35.js

-    bytes.push(parseInt(hex, 16));
-  });
+  if (uuid.length === 36) {
+    for (let i = 0; i < uuid.length; ++i) {


Why not i+=2 instead of ++i here and another ++i below?

@ctavan because not in all cases it is necessary to increase by 2. If we find a -, then increase only by 1.
https://github.com/uuidjs/uuid/pull/439/files#diff-4c3fa2fbfa9d4b899d8be4b79ae6907fR33

ctavan · 2020-05-07T07:28:15Z

test/unit/v35.test.js

@@ -69,7 +69,7 @@ describe('v5', () => {
    assert.equal(v3('hello.example.com', v3.DNS), '9125a8dc-52ee-365b-a5aa-81b0b3681cf6');
    assert.equal(v3('http://example.com/hello', v3.URL), 'c6235813-3ba4-3801-ae84-e0a6ebb7d138');
    assert.equal(
-      v3('hello', '0f5abcd1-c194-47f3-905b-2df7263a084b'),
+      v3('hello', '0f5abcd1-c194-47f3-905b-2df7263a084b'.toUpperCase()),


This would be a good opportunity to break up the huge v3/v5 tests into several smaller test() cases, Ideally 1 or 2 assertions per test block.

Then I believe that the toUpperCase() test would deserve its own test case with a reasonable description like ("accepts uppercase uuid notation for namespace")

I also believe we should, just for the sake of consistency, keep the v3 and v5 tests in sync and test the same stuff on both variants of the algorithm.

ctavan · 2020-05-07T07:31:56Z

test/unit/v35.test.js

+      invalid = true;
+    }
+
+    assert.ok(invalid, 'v3 namespace should be invalid');


Use assert.throws() instead.

ctavan · 2020-05-07T07:32:20Z

test/unit/v35.test.js

@@ -69,7 +69,7 @@ describe('v5', () => {
    assert.equal(v3('hello.example.com', v3.DNS), '9125a8dc-52ee-365b-a5aa-81b0b3681cf6');
    assert.equal(v3('http://example.com/hello', v3.URL), 'c6235813-3ba4-3801-ae84-e0a6ebb7d138');
    assert.equal(


Move to assert.strictEqual()

ctavan · 2020-05-07T07:35:29Z

src/v35.js

-  uuid.replace(/[a-fA-F0-9]{2}/g, function (hex) {
-    bytes.push(parseInt(hex, 16));
-  });
+  if (uuid.length === 36) {


Prefer early return to save one level of indentation:

if (uuid.length !== 36) { return bytes; }

ctavan · 2020-05-07T12:04:13Z

src/v35.js

@@ -1,12 +1,53 @@
 import bytesToUuid from './bytesToUuid.js';

+function hexSymToDecNum(n) {


Is this so much faster than parseInt(n, 16)?

Yes, faster than parseInt(uuid.substr(i, 2), 16)
You can check it yourself.

This is ridiculous 😂 🤷‍♂️

Noticeably faster, otherwise I would have done through parseInt %)

Don't get me wrong: I didn't mean to question, that your solution is considerably faster, I was just amazed by the fact…

splitted v35 tests

broofa

However I'm currently a bit undecided whether the increase in code complexity is really worth the improved performance. v3/v5 are used very rarely and we have never received any reports about perf issues with them.

I'm tempted to prefer a concise implementation over performance optimizations in this case.

I'm of two minds. As @ctavan notes, this only benefits v3/v5 users who are complaining about perf... which is nobody. So does not justify the extra complexity and code size. For that reason, I'll suggest that we not take this PR.

That said, Iwe regularly get requests for the ability to parse and validate UUIDs. If/when we offer such (E.g. See my abandoned UUID class), I could definitely see this making sense, as parsing perf will be more relevant in that context.

awwit · 2020-05-10T21:38:43Z

I completed the validation using the sample from UUID class.

This of course increased the size of the module. But not more than 1 Kb (for complete bundle). Why do you have such strict limits? )

It’s a little strange that you care so much about the size of your module’s bundle. Since it includes a little-used v35.

It would be logical to publish versions separately (v1, v4, v35): @uuid/v1, @uuid/v4, @uuid/v35.

Of course you understand that the new functionality requires more code (and larger size of bundle).

broofa · 2020-05-11T00:09:03Z

Sorry, I think there may have been a misunderstanding. I wasn't suggesting that we add the UUID class' code to this PR. Rather, I was simply using that as an example of where we might leverage your code if/when we decided to add support for a parse/validate feature. Historically, however, we've shied away from that feature (too small an audience, already addressed in other modules).

awwit · 2020-05-11T00:12:38Z

@broofa now it’s clear ... then I will try to make the fastest parsing of the UUID that will fit into the limits you set.

ctavan · 2020-05-11T08:02:47Z

@awwit I believe there might still be a misinterpretation: with regards to UUID parsing we‘re really not concerned about bundle size! We‘re more concerned about the feature and API scope of this library! This library is used by millions of projects in the node.js ecosystem to generate UUIDs. If you look at how widespread UUID parser libraries on npm are you will notice that this is an almost negligible fraction compared to the usage of this library.

Hence, we are reluctant of adding little-used features in here that would probably be better off in a separate, special purpose npm module. I think we would be happy to host such an effort in the uuidjs GitHub organization, but for the time being not in this module itself.

And regarding bundlesize: since we moved to ES modules this library supports treeshaking. So people will only get those algorithms in their frontend bundle that they actually use. For Node.js bundle size is not a concern, hence no need to publish separate npm modules.

awwit · 2020-05-11T14:04:37Z

@ctavan good =) then which option will suit you?

In this PR, initially I did only a quick parsing of the UUID and checking for string validation by template 00000000-0000-0000-0000-000000000000. To match the exception "namespace must be uuid string or an Array of 16 byte values"

It seemed correct to me to correspond to the description of the exception. =)

broofa · 2020-05-12T16:12:18Z

Closing, with apologies in advance if that comes across as rude. That is not my intent. To recap the reasons for this:

The increased code complexity is not warranted. It doesn't offer enough benefit to the existing uuid audience (mainly because this only affects ~1% of users that use v3/v5 uuids).
High performance parsing will be more interesting if/when it is exposed as part of a module's public API.
Currently we (uuidjs org) don't have plans for such a module. If that changes, we should consider using this implementation.

Thanks!

feat: improved v35 performance

55d0f65

ctavan reviewed May 7, 2020

View reviewed changes

feat: fixed code style in uuidToBytes

bb17b41

splitted v35 tests

broofa reviewed May 7, 2020

View reviewed changes

awwit added 2 commits May 10, 2020 19:44

Merge branch 'master' into improve/v35

ba1076a

feat: validation uuid by RFC4122

930d640

awwit requested review from ctavan and broofa May 10, 2020 17:23

Merge branch 'master' into improve/v35

8f396a9

broofa closed this May 12, 2020

ctavan mentioned this pull request May 14, 2020

test: improve v3/v5 test #450

Merged

awwit mentioned this pull request May 26, 2020

test: parsing non RFC uuid values #455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved v35 performance #439

feat: improved v35 performance #439

awwit commented May 6, 2020

awwit commented May 6, 2020

ctavan left a comment

ctavan May 7, 2020

awwit May 7, 2020

ctavan May 7, 2020

ctavan May 7, 2020

ctavan May 7, 2020

ctavan May 7, 2020

ctavan May 7, 2020

ctavan May 7, 2020

ctavan May 7, 2020

awwit May 7, 2020

ctavan May 7, 2020

awwit May 7, 2020

ctavan May 7, 2020

broofa left a comment •

edited

awwit commented May 10, 2020

broofa commented May 11, 2020 •

edited

awwit commented May 11, 2020

ctavan commented May 11, 2020

awwit commented May 11, 2020

broofa commented May 12, 2020 •

edited

		@@ -1,12 +1,53 @@
		import bytesToUuid from './bytesToUuid.js';

		function hexSymToDecNum(n) {

feat: improved v35 performance #439

feat: improved v35 performance #439

Conversation

awwit commented May 6, 2020

awwit commented May 6, 2020

ctavan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broofa left a comment • edited

Choose a reason for hiding this comment

awwit commented May 10, 2020

broofa commented May 11, 2020 • edited

awwit commented May 11, 2020

ctavan commented May 11, 2020

awwit commented May 11, 2020

broofa commented May 12, 2020 • edited

broofa left a comment •

edited

broofa commented May 11, 2020 •

edited

broofa commented May 12, 2020 •

edited