Repository navigation
Making V8 6.1 ABI compatible with V8 6.0 #14220
Description
Activity
/cc @nodejs/ctc @nodejs/v8 @nodejs/n-api
I'm definitely +1 with this change, 6.1 fixes a lot of the issues I have seen with turbofan.
Reacted by F. Hinkelmann, Marvin Hagemeister and SlanternsDefinitely +1, since 6.0 and 5.9 still have a lot of rough edges that we addressed with 6.1 now.
+1 from me
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::Contentsthing.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
Contentsstructs 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! :)
Reacted by F. HinkelmannI 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.
Reacted by Vse Mozhe Buty, Matteo Collina, Marvin Hagemeister, Ádám Lippai and dnalborczykAn update: Items under "Fixed on V8 upstream" are done. I suppose the other relevant items need to be floated in node.
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 deprecatedMaybe<bool> ForceSet()is used in async-wrap.cc. Opening a PR to replace it withDefineOwnProperty(), see #14450.You're right!
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.
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