GeezerVison

CI: https://ci.nodejs.org/job/node-test-commit/6110/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/430/

I wouldn’t mind fast-tracking this if that means it gets into an earlier release.

/cc @fhinkel @mcollina

/cc @nodejs/v8 This is being manually backported as it sounded like this wouldn't make it into the official 5.4 release

How do we want to handle these cases?

IMHO we should backport it to v7. I'm all in for fast tracking, but I do not know what is the process.

Let's wait for @natorion or @ofrobots input.

@addaleax Thanks for opening the PR for me.

The 5.4 release branch is still maintained as far as I'm aware so that means we get a version number conflict if we land this and then upstream does a new 5.4 release. I see three options:

1. Lobby upstream harder to back-port.
2. Wait until V8 5.5 (which will probably land in v7.)
3. Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

When is due 5.5? How probable is another v5.4 release before 5.4? If it's v5.5 is close, it's probably easier to wait. If not, we'll have to backport and add our own version number.

We'll also need to commit the full patch somewhere, in order to reapply it when/if there is another v5.4 release.

This will not land in official 5.4. There are also no plans to land this in 5.5 because:

• The TurboFan part of the backport would be quite involved
• Node uses 5.4
  -- The adoption of 5.5 is news to me, how likely is it?

Merging the CrankShaft part only to 5.5 would be an option though if done in the next few days.

@natorion #9618 is the very-much-in-progress work.

Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

Is there any precedent for this so I could add something based on that to the PR here?

No precedence that I'm aware of. You would have to patch (in deps/v8), include/v8-version.h, src/version.cc and src/log-utils.cc (which would be a version bump-worthy change in its own right.)

@natorion: I have requested a merge to 5.5 in the upstream bug.

Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

Yeah, let's try this workflow out. Even if in this case there are alternatives, we may run into a similar situation in the future with a supported branch where we need a fix more urgently. It would be good to have this workflow in place and tested.

Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

I created #9754 so we can discuss it separately.

5.4 is not maintained anymore. So no risk of running into version conflicts. Can we go ahead an merge this?
@addaleax Can you update the patch number since you started the PR from your repo?

@fhinkel I can do that later (a bit busy @ NINA), but if you want, feel free to use the “Allow edits from maintainers” bit and push to this PR yourself :)

Can I get an LGTM here?

LGTM

Landed in b5453ee.

@fhinkel thanks for taking care of this!

Hey,

This might as well be an offtopic but this fix in V8 (v7.2.1) significantly improves a three years and half old code.

There is a project called benchmark-node-clone that tests various deep clone utilities in Node:

7.2.0

  14 tests completed.

  clone            x 13,138 ops/sec ±0.52% (89 runs sampled)
  clone-deep       x 12,453 ops/sec ±1.54% (88 runs sampled)
  clone-extend     x 10,224 ops/sec ±0.67% (93 runs sampled)
  cloneextend      x 20,309 ops/sec ±0.90% (92 runs sampled)
  component-clone  x 32,984 ops/sec ±0.65% (91 runs sampled)
-  deep-copy        x 20,239 ops/sec ±0.57% (93 runs sampled)
  deepclone        x 11,798 ops/sec ±0.81% (90 runs sampled)
  deepcopy         x 13,512 ops/sec ±0.70% (94 runs sampled)
  extend           x 30,427 ops/sec ±0.78% (94 runs sampled)
  fast-clone       x 15,082 ops/sec ±0.90% (91 runs sampled)
  lodash.cloneDeep x 16,431 ops/sec ±0.97% (90 runs sampled)
  stringify-clone  x 30,513 ops/sec ±0.71% (91 runs sampled)
  structured-clone x 18,772 ops/sec ±0.57% (93 runs sampled)
  utils-copy       x  8,468 ops/sec ±0.69% (94 runs sampled)

Fastest is component-clone
Slowest is utils-copy

7.2.1

14 tests completed.

  clone            x  12,612 ops/sec ±0.63% (87 runs sampled)
  clone-deep       x  24,516 ops/sec ±2.16% (84 runs sampled)
  clone-extend     x   9,766 ops/sec ±0.32% (89 runs sampled)
  cloneextend      x  20,939 ops/sec ±0.52% (91 runs sampled)
  component-clone  x  31,800 ops/sec ±0.85% (91 runs sampled)
+  deep-copy        x 113,871 ops/sec ±0.89% (91 runs sampled)
  deepclone        x  13,853 ops/sec ±1.54% (90 runs sampled)
  deepcopy         x  13,169 ops/sec ±0.88% (90 runs sampled)
  extend           x  31,117 ops/sec ±0.83% (93 runs sampled)
  fast-clone       x  19,035 ops/sec ±0.72% (90 runs sampled)
  lodash.cloneDeep x  16,213 ops/sec ±1.01% (90 runs sampled)
  stringify-clone  x  30,466 ops/sec ±0.92% (90 runs sampled)
  structured-clone x  19,776 ops/sec ±0.64% (93 runs sampled)
  utils-copy       x   7,890 ops/sec ±0.81% (91 runs sampled)

Fastest is deep-copy
Slowest is utils-copy

My module is called deep-copy and I haven't touched it since April 2013, only style changes since then.

6.9.2 - deep-copy        x 115,111 ops/sec ±0.75% (90 runs sampled)
6.9.1 - deep-copy        x 117,174 ops/sec ±0.76% (89 runs sampled)
6.3.1 - deep-copy        x 71,263 ops/sec ±0.67% (92 runs sampled)
6.3.0 - deep-copy        x 52,645 ops/sec ±1.59% (61 runs sampled)
4.7.0 - deep-copy        x 14,604 ops/sec ±1.81% (63 runs sampled)

As you can see the performance boost is quite dramatic and it is not entirely related to the performance regression in v7, so my thinking is that this fixes something that existed in js for a very long time.

Cool, thanks!