refactor: upgraded code to es6 standard #427

awwit · 2020-04-29T12:46:45Z

Changes:
Improved eslint configuration.
Upgraded code to es6 standard.
Optimized use of arrays.

Since you are already using ES6 features (Uint8Array as example), I allowed myself to adapt your code to this standard.

All var were replaced by let or const. So we will give more optimization opportunities for JS engine.

The same goes for using arrays. It is not recommended to create arrays through new Array(N). Because arrays are created full of holes. Which affects performance.
https://v8.dev/blog/elements-kinds

Since you are already using Uint8Array, I replaced regular arrays with typed ones (in some places). Which are created with a fixed size and free from the disadvantages of regular arrays.

P.S. If something does not suit you in my changes, then let me know and I will fix it =)

Optimized use of arrays.

LinusU

Neat 👍

src/v1.js

package.json

.eslintrc.json

ctavan

Thanks for working on this @awwit!

I was considering a similar modernization of the code base but didn't do that yet because I want to be sure that we do not introduce any performance regression.

So before going into a detailed review of your proposed changes: Would you be interested in exploring ways of benchmarking the library to prove that your changes actually at least do not introduce any performance regressions? (If they improve performance, as you claim, even better!)

awwit · 2020-04-29T14:10:19Z

@ctavan if you want me to make a benchmark, then I will do it. But a little later.

What test tools do you prefer? or is it not that important?

ctavan · 2020-04-29T14:13:09Z

@ctavan if you want me to make a benchmark, then I will do it. But a little later.

What test tools do you prefer? or is it not that important?

Great! I haven't done performance testing of JavaScript libraries recently so I would have to investigate what the state-of-the art is.

The only thing that I would consider important is that performance can be tested both, in Node.js as well as in browsers.

The performance tests should probably also be committed to this repository.

broofa

This looks good, thanks!

My one concern is that embracing ES6 is going to encourage the use of patterns and features that may break IE11, where there's limited ES6 support. E.g. see my comment below about using a default value for the offset argument. Such bugs should get caught by our BrowserStack tests though (right, @ctavan?)

Just something to be aware of.

package.json

src/bytesToUuid.js

src/v1.js

src/sha1-browser.js

broofa · 2020-04-30T02:18:36Z

exploring ways of benchmarking the library

For the record, I used to track performance in this module (using ad-hoc perf tests). I removed that code because perf simply stopped being interesting. It's been a while but, last I checked, I think the #'s were on the order of 100K's or even 1M's of ids/second (for v1 and v4).

I eventually concluded that the overhead of UUID creation almost always pales in comparison to the context in which the id is used (e.g. creating a DB entry or creating/validating model data) and, thus, not something to bother about.

That said, having a performance test as part of our CI tests to catch order-of-magnitude regressions would be good, but I don't think we need it as part of this PR. Let's create a separate issue for that.

Note: https://benchmarkjs.com/ is my goto perf test framework. (full disclosure)

Edit: Just ran those tests again, here 's the results (v1.4.7)

Performance testing v1 UUIDs
uuid.v1(): 2000000 uuids/second
uuid.v1('binary'): 833333 uuids/second
uuid.v1('binary', buffer): 555555 uuids/second

Performance testing v4 UUIDs
uuid.v4() with mathRNG: 833333 uuids/second
uuid.v4() with whatwgRNG: not defined
uuid.v4() with nodeRNG: 370370 uuids/second
uuid.v4('binary'): 250000 uuids/second
uuid.v4('binary', buffer): 400000 uuids/second

ctavan · 2020-04-30T12:28:50Z

@broofa I totally agree that performance is currently most likely not a problem of the uuid library. All I want is to catch, as you write, "order-of-magnitude regressions".

I've added a little script that might already do the trick: #430

Would be curious to see it run before and after this PR. @awwit wanna give it a try?

awwit · 2020-04-30T12:42:45Z

@ctavan I also already managed to prepare the code for performance testing (also using benchmark).

Almost the same code turned out as yours. I have to try it.

(In my version, I output the results to a file ${git-branch}-${node--version}.json. So you can save test results and switch to other branches.)

Let's try your benchmark. You need to merge it into a master. Then I can merge it into my branch and see the difference in performance.

ctavan · 2020-05-01T12:35:40Z

@awwit I've merged the benchmark script to master. Could you rebase this PR so we can see the results?

Thanks!

ctavan · 2020-05-01T12:39:42Z

My one concern is that embracing ES6 is going to encourage the use of patterns and features that may break IE11, where there's limited ES6 support. E.g. see my comment below about using a default value for the offset argument. Such bugs should get caught by our BrowserStack tests though (right, @ctavan?)

Regarding JS compatibility: Since we use babel to transpile the source into a form to support all platforms that we want to support we should not need to worry too much.

Also our tests should catch regressions for all currently supported platforms 🤞 ...

The only thing we should be careful about is that the more we let babel transpile code, the higher the risk that we get

increased bundlesize due to poor transformations of babel (we have bundlewatch to catch that!)
performance regressions (we have the benchmark script now to catch that!)

awwit · 2020-05-01T12:58:01Z

@ctavan done, I merged your tests to my branch.
Now we can look at the performance.

ctavan

Thanks for your effort!

In general I think this is a valuable contribution and will help us modernize the code base. We should still clarify a few questions, especially around eslint rules and typed array usage, but I think in general this already looks really good!

ctavan · 2020-05-01T12:43:29Z

.eslintrc.json

+    "standard",
+    "prettier",
+    "prettier/standard",
+    "plugin:prettier/recommended"


"extends": ["plugin:prettier/recommended"] already takes care of loading the prettier eslint plugin, so it should not be necessary to specify it again in the plugins array, see: https://prettier.io/docs/en/integrating-with-linters.html#recommended-configuration

src/md5-browser.js

ctavan · 2020-05-01T12:53:35Z

src/md5-browser.js

+
+  const length8 = input.length * 8;
+
+  const output = new Uint32Array(getOutputLength(length8));


Why a Uint32Array instead of a plain Array?

https://v8.dev/blog/elements-kinds

Should allow for better JS engine optimizations. 'Pretty sure typed arrays are supported on all our supported platforms, so this should be okay.

Hmm, what made me suspicious is the following. With this adjustment to the benchmarks:

--- a/examples/benchmark/benchmark.js +++ b/examples/benchmark/benchmark.js @@ -7,7 +7,7 @@ const uuidv5 = (typeof window !== 'undefined' && window.uuidv5) || require('uuid console.log('Starting. Tests take ~1 minute to run ...'); -const array = new Array(16); +const array = new Uint8Array(16); const suite = new Benchmark.Suite(); suite .add('uuidv1()', function () {

the results change significantly.

With new Array(16):

uuidv1() fill existing array x 4,749,694 ops/sec ±3.14% (87 runs sampled) uuidv4() fill existing array x 356,833 ops/sec ±3.75% (83 runs sampled)

With new Uint8Array(16):

uuidv1() fill existing array x 775,015 ops/sec ±1.50% (93 runs sampled) uuidv4() fill existing array x 364,461 ops/sec ±4.00% (85 runs sampled)

What am I missing?

@ctavan hmm…

On my machine the difference in the margin of error. Try again…
Although, I made some more optimizations locally.

ctavan · 2020-05-01T12:54:52Z

src/rng-browser.js

 export default function rng() {
  if (!getRandomValues) {
    throw new Error(
      'crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported',
    );
  }
+
+  const rnds8 = new Uint8Array(16);


This might have a performance impact. A new Uint8Array is created with each call to rng() instead of reusing a preallocated array. Please verify!

I think this is unsafe, because someone can use your rng() function in their asynchronous code. When generating random numbers, it will always receive the same array. (New data will overwrite old).

Multiple function calls will return the same array.

rng-browser is not part of the public API, so should not be used externally. Internally, there's no async code that uses it. It's also very purpose-specific (generating a 16-byte array of random #'s), so I don't see there being a lot of use cases where someone would actually need to figure out how to reach into this module to access this function. Thus, the risk to users feels more hypothetical than real.

That said, reusing the array was an optimization I put in place a long time ago. There was no real demand for it (nobody was complaining about perf at the time). It was just a vanity thing to see how much perf I could squeeze out of the code.

Now, having said that, it's looks like it's still a hell of an optimization, which I'll suggest is worth keeping.

[/me pats himself on back 😆]

On master:

rng() reusing array: uuidv4() x 507,182 ops/sec ±5.43% (63 runs sampled) benchmark.js:32 uuidv4() fill existing array x 688,903 ops/sec ±8.40% (63 runs sampled) rng() creating new array each time: uuidv4() x 183,198 ops/sec ±10.90% (45 runs sampled) benchmark.js:32 uuidv4() fill existing array x 211,791 ops/sec ±13.46% (41 runs sampled)

On this PR:

rng() reusing array: uuidv4() x 524,604 ops/sec ±0.82% (64 runs sampled) benchmark.js:20 uuidv4() fill existing array x 741,362 ops/sec ±0.52% (64 runs sampled) rng() creating new array each time: uuidv4() x 197,603 ops/sec ±10.09% (51 runs sampled) benchmark.js:20 uuidv4() fill existing array x 211,450 ops/sec ±15.85% (41 runs sampled)

(Note: I suspect the larger margin of error in the new-array case is due is because of the increased levels of array creation causing more erratic garbage collection.)

[/me pats himself on back 😆]

Well played, 9-year-ago-you, well played! 😂

@awwit please re-introduce this optimization by moving the array definition out of the function scope again.

src/sha1-browser.js

src/v4.js

ctavan · 2020-05-01T12:58:54Z

.eslintrc.json

+  "plugins": ["node", "standard", "prettier"],
+  "extends": [
+    "eslint:recommended",
+    "standard",


If we decide to go for a standard eslint preset, I think we should try to get rid of as many custom rules as possible. I would be all in for that!

How did you decide which custom rules to keep and which to drop? Are there maybe more rules we can drop?

Ok, I'll see what rules can be safely removed.

ctavan · 2020-05-01T13:09:18Z

Node 12

Master:

Starting. Tests take ~1 minute to run ...
uuidv1() x 1,319,851 ops/sec ±1.76% (86 runs sampled)
uuidv1() fill existing array x 4,732,686 ops/sec ±1.83% (90 runs sampled)
uuidv4() x 294,904 ops/sec ±2.72% (82 runs sampled)
uuidv4() fill existing array x 357,955 ops/sec ±3.05% (82 runs sampled)
uuidv3() x 106,752 ops/sec ±3.61% (79 runs sampled)
uuidv5() x 113,064 ops/sec ±1.56% (83 runs sampled)
Fastest is uuidv1() fill existing array

This branch:

Starting. Tests take ~1 minute to run ...
uuidv1() x 1,519,697 ops/sec ±2.50% (87 runs sampled)
uuidv1() fill existing array x 8,119,318 ops/sec ±2.89% (92 runs sampled)
uuidv4() x 303,734 ops/sec ±2.53% (86 runs sampled)
uuidv4() fill existing array x 362,363 ops/sec ±4.08% (85 runs sampled)
uuidv3() x 114,053 ops/sec ±1.37% (82 runs sampled)
uuidv5() x 108,882 ops/sec ±2.21% (80 runs sampled)
Fastest is uuidv1() fill existing array

uuidv1() performance has improved, especially in the "fill existing array" case 😲 ! Would be curious to learn what caused this improvement.

The rest seems to perform similar than before (within error margins) 👏

broofa · 2020-05-01T15:20:20Z

Thanks for your effort! ... this is a valuable contribution and will help us modernize the code base. ... I think in general this already looks really good!

Agreed!

Would be curious to learn what caused this improvement.

Me too. I know switching from var to const/let is generally considered a good thing (perf-wise). As is using typed arrays, but I'd be curious to hear what someone familiar with how JS engines work would have to say about this.

awwit · 2020-05-01T16:44:40Z

@ctavan

uuidv1() performance has improved, especially in the "fill existing array" case ! Would be curious to learn what caused this improvement.

I checked, this is due to the fact that we replaced new Date().getTime() to Date.now().

awwit · 2020-05-01T21:19:14Z

@ctavan Bundlewatch test failed =/

We will have to increase the size to get more optimizations (especially parsing uuid to bytes).

If the size is so critical, then I can remove typed arrays in failed bundles.

src/bytesToUuid.js

ctavan · 2020-05-02T08:20:01Z

I think we are starting to change too much stuff in this PR, this no longer feels controllable.

Could you please revert your latest code optimizations (let's discuss them in a separate pull request)?

I would then try to get this merged quickly so that we can go through further code optimizations in a more step-by-step manner and therefore controlling better for possible performance optimizations.

So for the time being I'd prefer if this PR would contain only:

code style changes (eslint config)
let/const changes
typed array changes

Let's move all other code changes into a separate pull request.

ctavan · 2020-05-02T08:26:06Z

Oh, and to get this in it would be great if you could Allow changes to a pull request branch created from a fork. This would help me get this merged and keep all references to your contributions.

This reverts commit 052489b.

ctavan · 2020-05-02T12:55:08Z

Benchmark master:

Starting. Tests take ~1 minute to run ...
uuidv1() x 1,377,681 ops/sec ±1.47% (91 runs sampled)
uuidv1() fill existing array x 4,881,829 ops/sec ±2.99% (89 runs sampled)
uuidv4() x 306,412 ops/sec ±2.56% (85 runs sampled)
uuidv4() fill existing array x 362,883 ops/sec ±3.90% (85 runs sampled)
uuidv3() x 111,873 ops/sec ±2.63% (81 runs sampled)
uuidv5() x 110,720 ops/sec ±2.73% (82 runs sampled)
Fastest is uuidv1() fill existing array

Benchmark this branch:

Starting. Tests take ~1 minute to run ...
uuidv1() x 1,555,686 ops/sec ±2.63% (90 runs sampled)
uuidv1() fill existing array x 7,804,665 ops/sec ±1.92% (87 runs sampled)
uuidv4() x 311,104 ops/sec ±2.22% (82 runs sampled)
uuidv4() fill existing array x 364,342 ops/sec ±4.73% (86 runs sampled)
uuidv3() x 117,100 ops/sec ±3.78% (82 runs sampled)
uuidv5() x 115,292 ops/sec ±1.76% (83 runs sampled)
Fastest is uuidv1() fill existing array

ctavan · 2020-05-02T13:05:48Z

@awwit I just realized that I already hat push access to your branch. So I did the requested cleanups and merged your changes 🎉

I copied the advanced optimizations to https://github.com/uuidjs/uuid/tree/awwit-refactorings feel free to pick them up from there and create another pull request if you want.

I'm happy to merge those changes as well once I understand their benefit and performance implications.

awwit · 2020-05-02T14:11:17Z

@ctavan You're right! Then I will continue to do in another brunch. I will remove the code that greatly inflates the bundle. And I’ll do something else.

And I will post the changes piece by piece.

awwit added 4 commits April 29, 2020 15:16

refactor: upgraded code to es6 standard

8532cab

Optimized use of arrays.

Merge branch 'master' into upgrade/es6

112cf50

Merge branch 'master' into upgrade/es6

c26ba9e

fix: readme docs

557d316

LinusU approved these changes Apr 29, 2020

View reviewed changes

fix: v1 generator

3d496c0

LinusU reviewed Apr 29, 2020

View reviewed changes

src/v1.js Outdated Show resolved Hide resolved

awwit requested a review from LinusU April 29, 2020 13:49

LinusU reviewed Apr 29, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

TrySound reviewed Apr 29, 2020

View reviewed changes

.eslintrc.json Outdated Show resolved Hide resolved

ctavan requested changes Apr 29, 2020

View reviewed changes

broofa requested changes Apr 29, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/bytesToUuid.js Show resolved Hide resolved

src/v1.js Outdated Show resolved Hide resolved

src/sha1-browser.js Outdated Show resolved Hide resolved

awwit added 2 commits April 29, 2020 22:08

fix: v1 generator and revert some changes

06b3d72

Merge branch 'master' into upgrade/es6

39d70eb

broofa approved these changes Apr 30, 2020

View reviewed changes

broofa mentioned this pull request May 1, 2020

test: add benchmark #430

Merged

Merge branch 'master' into upgrade/es6

dda6749

ctavan requested changes May 1, 2020

View reviewed changes

feat: added more optimizations

522eddb

fix: readme_js tests

052489b

ctavan reviewed May 2, 2020

View reviewed changes

src/bytesToUuid.js Show resolved Hide resolved

ctavan added 4 commits May 2, 2020 14:17

Partially revert 522eddb

8b906e6

Simplify eslint config

ffba997

Revert "fix: readme_js tests"

a89aa5c

This reverts commit 052489b.

Revert some changes to benchmark

0391a8b

ctavan approved these changes May 2, 2020

View reviewed changes

ctavan merged commit f6ce4bf into uuidjs:master May 2, 2020

awwit deleted the upgrade/es6 branch May 3, 2020 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upgraded code to es6 standard #427

refactor: upgraded code to es6 standard #427

awwit commented Apr 29, 2020

LinusU left a comment

ctavan left a comment

awwit commented Apr 29, 2020

ctavan commented Apr 29, 2020

broofa left a comment

broofa commented Apr 30, 2020 •

edited

Loading

ctavan commented Apr 30, 2020

awwit commented Apr 30, 2020

ctavan commented May 1, 2020

ctavan commented May 1, 2020

awwit commented May 1, 2020

ctavan left a comment

ctavan May 1, 2020

ctavan May 1, 2020

awwit May 1, 2020

broofa May 1, 2020

ctavan May 1, 2020

awwit May 1, 2020

ctavan May 1, 2020

awwit May 1, 2020

broofa May 1, 2020 •

edited

Loading

ctavan May 1, 2020

ctavan May 1, 2020

ctavan May 1, 2020

awwit May 1, 2020

ctavan commented May 1, 2020

broofa commented May 1, 2020 •

edited

Loading

awwit commented May 1, 2020

awwit commented May 1, 2020 •

edited

Loading

ctavan commented May 2, 2020

ctavan commented May 2, 2020

ctavan commented May 2, 2020

ctavan commented May 2, 2020

awwit commented May 2, 2020 •

edited

Loading


		const length8 = input.length * 8;

		const output = new Uint32Array(getOutputLength(length8));

refactor: upgraded code to es6 standard #427

refactor: upgraded code to es6 standard #427

Conversation

awwit commented Apr 29, 2020

LinusU left a comment

Choose a reason for hiding this comment

ctavan left a comment

Choose a reason for hiding this comment

awwit commented Apr 29, 2020

ctavan commented Apr 29, 2020

broofa left a comment

Choose a reason for hiding this comment

broofa commented Apr 30, 2020 • edited Loading

ctavan commented Apr 30, 2020

awwit commented Apr 30, 2020

ctavan commented May 1, 2020

ctavan commented May 1, 2020

awwit commented May 1, 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

broofa May 1, 2020 • edited Loading

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

ctavan commented May 1, 2020

broofa commented May 1, 2020 • edited Loading

awwit commented May 1, 2020

awwit commented May 1, 2020 • edited Loading

ctavan commented May 2, 2020

ctavan commented May 2, 2020

ctavan commented May 2, 2020

ctavan commented May 2, 2020

awwit commented May 2, 2020 • edited Loading

broofa commented Apr 30, 2020 •

edited

Loading

broofa May 1, 2020 •

edited

Loading

broofa commented May 1, 2020 •

edited

Loading

awwit commented May 1, 2020 •

edited

Loading

awwit commented May 2, 2020 •

edited

Loading