Node: Making V8 6.1 ABI compatible with V8 6.0

Created on 13 Jul 2017 · 11Comments · Source: nodejs/node

There is currently work under way to pull in V8 6.0 into Node 8 (#14004). In order to reduce the number of cherry-picks to Node 8 there might be the option to later on adopt V8 6.1 (to be cut next week). This means V8 6.1 needs to be ABI compatible with V8 6.0.

The V8 team is currently analyzing the information (tracking issue) and there are the following issues found:

Node likely needs to float these

AllowCodeGenerationFromStringsCallback

Signature changed for AllowCodeGenerationFromStringsCallback in https://chromium-review.googlesource.com/c/532875/
The old signature still exists as DeprecatedAllowCodeGenerationFromStringsCallback. Should V8 rename the new signature to AllowCodeGenerationFromStringCallback2 and restore the old signature? Is this being used in Node.js anywhere?

Does node need to float these?

Are these removals used/important for Node?

bool Object::ForceSet

Removed bool Object::ForceSet(Local key, Local value, PropertyAttribute attribs = None) in https://chromium-review.googlesource.com/c/518162/
That was already deprecated and removed which means there should be no usage?

void Isolate::SetWasmCompileCallback

Removed void Isolate::SetWasmCompileCallback(ExtensionCallback callback) and void Isolate::SetWasmInstantiateCallback(ExtensionCallback callback) in https://chromium-review.googlesource.com/c/525141/
Can we simply assume that people are not going to use these Wasm APIs yet?

Changes that shouldn't be a problem

These removals change the symbol table but the APIs were experimental so nobody should depend on them.

Removed class DynamicImportResult in https://chromium-review.googlesource.com/c/492646/
Removed Isolate::CreateParams::host_import_module_dynamically_callback_ in https://codereview.chromium.org/2897103002
Signature changed for HostImportModuleDynamicallyCallback in https://chromium-review.googlesource.com/c/492646/

Fixed on V8 upstream

Class layout changed for ArrayBuffer::Contents

Class layout changed for ArrayBuffer::Contents in https://chromium-review.googlesource.com/c/523271/
This is being fixed by reordering the new fields to the end. This works because native modules that are built for V8 6.0 will be able to access correct fields in ArrayBuffer::Contents objects created in V8 6.1. Fix is here: https://chromium-review.googlesource.com/c/569758/

Class layout changed for SharedArrayBuffer::Contents

Class layout changed for SharedArrayBuffer::Contents in https://chromium-review.googlesource.com/c/523271/
This is being fixed by reordering the new fields to the end. Fix is here: https://chromium-review.googlesource.com/c/569758/

Constants changed in Internals

Constants changed in Internals in https://chromium-review.googlesource.com/c/526001/ and https://chromium-review.googlesource.com/c/500447/
This is being fixed by removing padding instance types in https://chromium-review.googlesource.com/c/569618/

cc @MylesBorins @hashseed

V8 Engine

Source

natorion

🎉11

Most helpful comment

I was asked by @vsemozhetbyt to provide a few details about a performance difference between using ES6 classes vs ES5 protoyting. I created a simple Neural Net application in Typescript and I found that when requesting TS to generate ES5 code the performance was 6x better than when requesting TS to generate ES6 code. See Javascript ES6 class is slower than using ES5 prototype on the v8-dev mailing list for details, but the problem came down to V8 5.8 isn't as performant as V8 6.1 code when it comes to ES6 classes.

Subsequently I tested with Node v8 pre release which uses V8 6.1 and found that the performance is now basically the same, see the nodev9 branch of my test code here.

winksaville on 18 Jul 2017

👍5

All 11 comments

/cc @nodejs/ctc @nodejs/v8 @nodejs/n-api

MylesBorins on 13 Jul 2017

I'm definitely +1 with this change, 6.1 fixes a lot of the issues I have seen with turbofan.

mcollina on 13 Jul 2017

👍3

Definitely +1, since 6.0 and 5.9 still have a lot of rough edges that we addressed with 6.1 now.

bmeurer on 13 Jul 2017

+1 from me

evanlucas on 13 Jul 2017

Refs: http://benediktmeurer.de/2017/07/14/faster-collection-iterators/

vsemozhetbyt on 14 Jul 2017

One other thing that came up for the V8 5.8 → V8 6.0 compat was the increased requirements for ArrayBuffer Allocators for wasm. Right now the feature that uses them is behind a flag but apparently was supposed to be enabled in V8 6.1.

Maybe we should just keep that feature disabled in Node 8, that would also help with the (Shared)ArrayBuffer::Contents thing.

Should V8 rename the new signature to AllowCodeGenerationFromStringCallback2 and restore the old signature? Is this being used in Node.js anywhere?

I wouldn’t think so (at least not in Node’s own code) but this seems fixable, for example by your suggested approach, so I think we should address it. That doesn’t need to happen on V8’s side.

Removed bool Object::ForceSet(Local key, Local value, PropertyAttribute attribs = None) in https://chromium-review.googlesource.com/c/518162/
That was already deprecated and removed which means there should be no usage?

I’m not sure – but I guess reverting that removal would be easy enough? If not, the method can be made a simple wrapper around the soon-to-be-deprecated one, right?

Can we simply assume that people are not going to use these Wasm APIs yet?

I’d say so. I think we already made that assumption for the previous API compatibility patches.

Class layout changed for SharedArrayBuffer::Contents in https://chromium-review.googlesource.com/c/523271/
This is being fixed by reordering the new fields to the end. Fix is here: https://chromium-review.googlesource.com/c/569758/

I don’t think that’s enough though? Right now, the Contents structs are returned by value from V8 methods. At least on some platforms/calling conventions (checked Linux x64) that’s implemented by the caller passing a pointer to the callee, but since these structs are larger now, the caller doesn’t know the correct amount of memory to allocate and so the callee performs out-of-bounds writes – ouch.

We should probably just remove these fields and their getters?

V8 is already trying to fix these

Thank you! :)

addaleax on 14 Jul 2017

👍1

Subsequently I tested with Node v8 pre release which uses V8 6.1 and found that the performance is now basically the same, see the nodev9 branch of my test code here.

winksaville on 18 Jul 2017

👍5

An update: Items under "Fixed on V8 upstream" are done. I suppose the other relevant items need to be floated in node.

natorion on 19 Jul 2017

Removed bool Object::ForceSet(Local key, Local value, PropertyAttribute attribs = None) in https://chromium-review.googlesource.com/c/518162/
That was already deprecated and removed which means there should be no usage?

I’m not sure – but I guess reverting that removal would be easy enough? If not, the method can be made a simple wrapper around the soon-to-be-deprecated one, right?

The deleted bool ForceSet() method is not used anywhere. The now deprecated Maybe<bool> ForceSet() is used in async-wrap.cc. Opening a PR to replace it with DefineOwnProperty(), see https://github.com/nodejs/node/pull/14450.