New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v8: expose new V8 5.5 serialization API #11048

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@addaleax
Member

addaleax commented Jan 28, 2017

Expose the new serialization API that was added in V8 5.5 to userland. The JS API is virtually a direct copy of what V8 provides on the C++ level.

This is useful Node as a possible replacement for some internals that currently use JSON, like IPC, but is likely to be useful to general userland code as well.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

V8

/cc @nodejs/v8

Show outdated Hide outdated deps/v8/gypfiles/toolchain.gypi
@@ -61,7 +61,7 @@
'v8_android_log_stdout%': 0,
# Force disable libstdc++ debug mode.
'disable_glibcxx_debug%': 0,
'disable_glibcxx_debug%': 1,

This comment has been minimized.

@addaleax

addaleax Jan 28, 2017

Member

This change is weird, and I’m not sure what the right place to do it is. It’s required because the std::vector that is exposed by _GLIBCXX_DEBUG isn’t necessarily ABI-compatible with the one that’s exposed without the define; so either Node would also have to use that flag or V8 needs to disable it by default.

And in general, I’m not sure it’s intentional on @nodejs/v8’s side that debug builds expose a different ABI than release builds…?

@addaleax

addaleax Jan 28, 2017

Member

This change is weird, and I’m not sure what the right place to do it is. It’s required because the std::vector that is exposed by _GLIBCXX_DEBUG isn’t necessarily ABI-compatible with the one that’s exposed without the define; so either Node would also have to use that flag or V8 needs to disable it by default.

And in general, I’m not sure it’s intentional on @nodejs/v8’s side that debug builds expose a different ABI than release builds…?

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 30, 2017

Member

You should be able to override it from common.gypi but I don't expect it's necessary to do so.

Checked STL containers have different name mangling in libstdc++. I think the worst case is that the add-on fails to load at runtime, but not that it silently does the wrong thing.

@bnoordhuis

bnoordhuis Jan 30, 2017

Member

You should be able to override it from common.gypi but I don't expect it's necessary to do so.

Checked STL containers have different name mangling in libstdc++. I think the worst case is that the add-on fails to load at runtime, but not that it silently does the wrong thing.

This comment has been minimized.

@targos

targos Jan 30, 2017

Member

FWIW in V8 5.6 Release will return a std::pair<uint8_t*, size_t> instead of a vector.

Ref: https://github.com/v8/v8/blob/753a2b55781face0f10b8d9a6fb0da45ede2c693/include/v8.h#L1784

@targos

targos Jan 30, 2017

Member

FWIW in V8 5.6 Release will return a std::pair<uint8_t*, size_t> instead of a vector.

Ref: https://github.com/v8/v8/blob/753a2b55781face0f10b8d9a6fb0da45ede2c693/include/v8.h#L1784

This comment has been minimized.

@addaleax

addaleax Jan 30, 2017

Member

Checked STL containers have different name mangling in libstdc++. I think the worst case is that the add-on fails to load at runtime, but not that it silently does the wrong thing.

Some of the functions do get inlined, even in debug builds. (Like, this modification is not just academic; without it, I could observe actual out-of-bound writes.)

FWIW in V8 5.6 Release will return a std::pair<uint8_t*, size_t> instead of a vector.

Ok, then at least we don’t need to worry about it. :)

@addaleax

addaleax Jan 30, 2017

Member

Checked STL containers have different name mangling in libstdc++. I think the worst case is that the add-on fails to load at runtime, but not that it silently does the wrong thing.

Some of the functions do get inlined, even in debug builds. (Like, this modification is not just academic; without it, I could observe actual out-of-bound writes.)

FWIW in V8 5.6 Release will return a std::pair<uint8_t*, size_t> instead of a vector.

Ok, then at least we don’t need to worry about it. :)

Show outdated Hide outdated doc/api/v8.md
Read raw bytes from the deserializer’s internal buffer. The `length` parameter
must correspond to the length of the buffer that was passed to
[`serializer.writeRawBytes()`][].
For use inside of a custom `options.writeHostObject`.

This comment has been minimized.

@addaleax

addaleax Jan 28, 2017

Member

Most of the wording here is taken directly from the V8 headers, btw.

@addaleax

addaleax Jan 28, 2017

Member

Most of the wording here is taken directly from the V8 headers, btw.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 28, 2017

Contributor

How does this compare performance-wise to using JSON.parse()/JSON.stringify() for IPC? I looked into the possibility of using a binary protocol for IPC a little while ago and it wasn't even close to the JSON performance IIRC.

Contributor

mscdex commented Jan 28, 2017

How does this compare performance-wise to using JSON.parse()/JSON.stringify() for IPC? I looked into the possibility of using a binary protocol for IPC a little while ago and it wasn't even close to the JSON performance IIRC.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jan 28, 2017

Member

How does this compare performance-wise to using JSON.parse()/JSON.stringify() for IPC?

I haven’t really benchmarked that so far. A round-trip on the data in test/fixtures/url-tests.json shows that it’s about 15 % slower than JSON, a round-trip on 'x'.repeat(100000) works out about 4× faster than JSON for me; I guess it just depends a lot on what kind of data you’re sending. I’m also assuming there’s room for improvement in V8 here.

I’m definitely not suggesting that we make a decision to change the IPC procotol right now; it’s just an option we may want to keep an eye on.

Also: This protocol is a lot more expressive than JSON, so having this available is going to be useful on its own.

Member

addaleax commented Jan 28, 2017

How does this compare performance-wise to using JSON.parse()/JSON.stringify() for IPC?

I haven’t really benchmarked that so far. A round-trip on the data in test/fixtures/url-tests.json shows that it’s about 15 % slower than JSON, a round-trip on 'x'.repeat(100000) works out about 4× faster than JSON for me; I guess it just depends a lot on what kind of data you’re sending. I’m also assuming there’s room for improvement in V8 here.

I’m definitely not suggesting that we make a decision to change the IPC procotol right now; it’s just an option we may want to keep an eye on.

Also: This protocol is a lot more expressive than JSON, so having this available is going to be useful on its own.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 28, 2017

Member

+1 but I think we should wait until the API and binary format are stable. ReleaseBuffer will be renamed to Release (and return a std::pair) in V8 5.6 for example.

Member

targos commented Jan 28, 2017

+1 but I think we should wait until the API and binary format are stable. ReleaseBuffer will be renamed to Release (and return a std::pair) in V8 5.6 for example.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 28, 2017

Member

What about allowing to extend the class and implement the optional methods instead of using an object of options?

Member

targos commented Jan 28, 2017

What about allowing to extend the class and implement the optional methods instead of using an object of options?

@addaleax addaleax added the blocked label Jan 28, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jan 28, 2017

Member

What about allowing to extend the class and implement the optional methods instead of using an object of options?

That should work just as well. The options approach felt more Node-like, but I don’t feel strongly about it.

I think we should wait until the API and binary format are stable.

Sure, we can do that. I’ve marked the PR as “blocked” but tbh I am not too worried about the binary format given that it’s non-portable anyway.

Member

addaleax commented Jan 28, 2017

What about allowing to extend the class and implement the optional methods instead of using an object of options?

That should work just as well. The options approach felt more Node-like, but I don’t feel strongly about it.

I think we should wait until the API and binary format are stable.

Sure, we can do that. I’ve marked the PR as “blocked” but tbh I am not too worried about the binary format given that it’s non-portable anyway.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 28, 2017

Member

I'm worried because I believe one of the first things users will do with this API is serialize complex objects and save them to disk for later comparison in unit tests.

Member

targos commented Jan 28, 2017

I'm worried because I believe one of the first things users will do with this API is serialize complex objects and save them to disk for later comparison in unit tests.

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Jan 28, 2017

Member

Considering that it uses just v8.h, is there any reason to not do it in npm module?

I know getting things to core sounds great, but it is simpler to improve API while the module is managed separately. It can always be included in core later, when the API will be stabilized and well tested.

What do you think?

Member

indutny commented Jan 28, 2017

Considering that it uses just v8.h, is there any reason to not do it in npm module?

I know getting things to core sounds great, but it is simpler to improve API while the module is managed separately. It can always be included in core later, when the API will be stabilized and well tested.

What do you think?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jan 29, 2017

Member

I'm worried because I believe one of the first things users will do with this API is serialize complex objects and save them to disk for later comparison in unit tests.

Yeah, sure. Like, I didn’t want to imply that I think waiting until the API is no longer experimental is a bad idea. :)

Considering that it uses just v8.h, is there any reason to not do it in npm module?

Basically no other reason than that I think (and I might be wrong) that Node core is going to be a consumer of this API anyway.

It can always be included in core later, when the API will be stabilized and well tested.

This PR is very very (maybe too) close to the V8 API, so if we do what @targos suggests and we just wait until V8 considers it stable we should be fine.

Member

addaleax commented Jan 29, 2017

I'm worried because I believe one of the first things users will do with this API is serialize complex objects and save them to disk for later comparison in unit tests.

Yeah, sure. Like, I didn’t want to imply that I think waiting until the API is no longer experimental is a bad idea. :)

Considering that it uses just v8.h, is there any reason to not do it in npm module?

Basically no other reason than that I think (and I might be wrong) that Node core is going to be a consumer of this API anyway.

It can always be included in core later, when the API will be stabilized and well tested.

This PR is very very (maybe too) close to the V8 API, so if we do what @targos suggests and we just wait until V8 considers it stable we should be fine.

@jasnell

Great to see. Almost there.

@@ -134,3 +134,150 @@ setTimeout(function() { v8.setFlagsFromString('--notrace_gc'); }, 60e3);
[V8]: https://developers.google.com/v8/
[here]: https://github.com/thlorenz/v8-flags/blob/master/flags-0.11.md
[`GetHeapSpaceStatistics`]: https://v8docs.nodesource.com/node-5.0/d5/dda/classv8_1_1_isolate.html#ac673576f24fdc7a33378f8f57e1d13a4
## Serialization API

This comment has been minimized.

@jasnell

jasnell Jan 30, 2017

Member

If we are going to introduce this API, even tho I know this is going into the v8 module that is already clearly marked as being fluid based on what v8 chooses to do, we should mark this explicitly as being Experimental for the time being.

@jasnell

jasnell Jan 30, 2017

Member

If we are going to introduce this API, even tho I know this is going into the v8 module that is already clearly marked as being fluid based on what v8 chooses to do, we should mark this explicitly as being Experimental for the time being.

This comment has been minimized.

@addaleax
@addaleax
const v8binding = process.binding('v8');
const serdesBinding = process.binding('serdes');

This comment has been minimized.

@jasnell

jasnell Jan 30, 2017

Member

perhaps something more explicit such as v8serdes would be better? Not sure about that tho.
@nodejs/node-chakracore have you all considered implementing this mechanism yet?

@jasnell

jasnell Jan 30, 2017

Member

perhaps something more explicit such as v8serdes would be better? Not sure about that tho.
@nodejs/node-chakracore have you all considered implementing this mechanism yet?

This comment has been minimized.

@addaleax

addaleax Jan 31, 2017

Member

Yeah, I guess that depends if and how node-chakracore would want to implement this. I don’t think the binding name really matters a lot anyway.

@addaleax

addaleax Jan 31, 2017

Member

Yeah, I guess that depends if and how node-chakracore would want to implement this. I don’t think the binding name really matters a lot anyway.

Show outdated Hide outdated lib/v8.js
exports.Serializer =
class Serializer extends serdesBinding.Serializer {
constructor(options = {}) {

This comment has been minimized.

@jasnell

jasnell Jan 30, 2017

Member

Extremely minor nit but use of argument defaults has not benchmarked rather well yet. It's likely not something to worry about in this case, however

@jasnell

jasnell Jan 30, 2017

Member

Extremely minor nit but use of argument defaults has not benchmarked rather well yet. It's likely not something to worry about in this case, however

@@ -0,0 +1,93 @@
'use strict';

This comment has been minimized.

@jasnell

jasnell Jan 30, 2017

Member

A test that includes a pre-serialized buffer that can be round-tripped serialized->deser->re-serialized would be good also.

@jasnell

jasnell Jan 30, 2017

Member

A test that includes a pre-serialized buffer that can be round-tripped serialized->deser->re-serialized would be good also.

This comment has been minimized.

@addaleax
@addaleax
@sam-github

I know this won't land until the v8 api is stable, and maybe better documented, but left some comments on docs I don't understand.

Show outdated Hide outdated doc/api/v8.md
* `getDataCloneError` {Function} A callback that takes a single `message`
argument and returns an error object. Defaults to the `Error` constructor.
* `writeHostObject` {Function} A callback that takes a single `object`
argument and is used to write some kind of host object, if possible. If not,

This comment has been minimized.

@sam-github

sam-github Jan 31, 2017

Member

what's a host object?

@sam-github

sam-github Jan 31, 2017

Member

what's a host object?

This comment has been minimized.

@addaleax

addaleax Feb 3, 2017

Member

what's a host object?

An object that’s exposed from C++, i.e. by Node or an addon. Most of the options & methods here won’t be very useful to the average user… so I’ve gone ahead and implemented v8.[de]serialize shorthands on top of these classes.

@addaleax

addaleax Feb 3, 2017

Member

what's a host object?

An object that’s exposed from C++, i.e. by Node or an addon. Most of the options & methods here won’t be very useful to the average user… so I’ve gone ahead and implemented v8.[de]serialize shorthands on top of these classes.

Returns the stored internal buffer. This serializer should not be used once
the buffer is released. Calling this method results in undefined behavior
if a previous write has failed.

This comment has been minimized.

@sam-github

sam-github Jan 31, 2017

Member

Is that accurate? On failure, you aren't allowed to cleanup?

@sam-github

sam-github Jan 31, 2017

Member

Is that accurate? On failure, you aren't allowed to cleanup?

This comment has been minimized.

@sam-github

sam-github Jan 31, 2017

Member

how do writes fail? do they throw errors? writeHeader/Value don't say they can throw.

@sam-github

sam-github Jan 31, 2017

Member

how do writes fail? do they throw errors? writeHeader/Value don't say they can throw.

This comment has been minimized.

@addaleax

addaleax Feb 3, 2017

Member

On failure, you aren't allowed to cleanup?

I guess that depends on what you mean by “cleanup”? Any resources held by the serializer will be returned when it gets gc’ed.

how do writes fail? do they throw errors? writeHeader/Value don't say they can throw.

Yes, writeValue may throw errors. I’ve noted that in the corresponding section.

@addaleax

addaleax Feb 3, 2017

Member

On failure, you aren't allowed to cleanup?

I guess that depends on what you mean by “cleanup”? Any resources held by the serializer will be returned when it gets gc’ed.

how do writes fail? do they throw errors? writeHeader/Value don't say they can throw.

Yes, writeValue may throw errors. I’ve noted that in the corresponding section.

Show outdated Hide outdated doc/api/v8.md
* `id` {Integer} A 32-bit unsigned integer.
* `arrayBuffer` {ArrayBuffer|SharedArrayBuffer} An `ArrayBuffer` instance.
Marks an `ArrayBuffer` as havings its contents transferred out of band.

This comment has been minimized.

@sam-github

sam-github Jan 31, 2017

Member

transferred? out-of-band?

@sam-github

sam-github Jan 31, 2017

Member

transferred? out-of-band?

This comment has been minimized.

@addaleax

addaleax Feb 3, 2017

Member

transferred? out-of-band?

Uh, yes. Basically says that the actual ArrayBuffer isn’t included in the resulting serialization, and that the data is made available to the deserializer through some other means.

If you have suggestions for better wording on anything, it might be a good idea to upstream them into V8, too.

@addaleax

addaleax Feb 3, 2017

Member

transferred? out-of-band?

Uh, yes. Basically says that the actual ArrayBuffer isn’t included in the resulting serialization, and that the data is made available to the deserializer through some other means.

If you have suggestions for better wording on anything, it might be a good idea to upstream them into V8, too.

Show outdated Hide outdated doc/api/v8.md
#### deserializer.readHeader()
* Returns: {Boolean}

This comment has been minimized.

@sam-github

sam-github Jan 31, 2017

Member

true/false means?

@sam-github

sam-github Jan 31, 2017

Member

true/false means?

This comment has been minimized.

@addaleax

addaleax Feb 3, 2017

Member

true/false means?

I’ve removed the return type from the documentation. Right now it’s always true except when an error is thrown.

@addaleax

addaleax Feb 3, 2017

Member

true/false means?

I’ve removed the return type from the documentation. Right now it’s always true except when an error is thrown.

@bnoordhuis

Left some comments. Overall impression is good, though.

Show outdated Hide outdated lib/v8.js
readRawBytes(length) {
const offset = this._readRawBytes(length);
return Buffer.prototype.slice.call(this.buffer, offset, offset + length);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Can you add a comment explaining that the contorted .call is because this.buffer can be either a Buffer or a plain Uint8Array? It's not immediately obvious from looking at just the code.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Can you add a comment explaining that the contorted .call is because this.buffer can be either a Buffer or a plain Uint8Array? It's not immediately obvious from looking at just the code.

static void ReadUint32(const FunctionCallbackInfo<Value>& args);
static void ReadUint64(const FunctionCallbackInfo<Value>& args);
static void ReadDouble(const FunctionCallbackInfo<Value>& args);
static void ReadRawBytes(const FunctionCallbackInfo<Value>& args);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Consider giving all methods inline linkage if they're not used outside this file.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Consider giving all methods inline linkage if they're not used outside this file.

This comment has been minimized.

@addaleax

addaleax Feb 4, 2017

Member

Consider giving all methods inline linkage if they're not used outside this file.

These functions are passed to V8, so what at least gcc does is to expose them as weak symbols instead of regular symbols.

I can wrap everything in here in an anonymous namespace if you want, if it’s about hiding them – is there a reason we don’t do that in our other source files?

@addaleax

addaleax Feb 4, 2017

Member

Consider giving all methods inline linkage if they're not used outside this file.

These functions are passed to V8, so what at least gcc does is to expose them as weak symbols instead of regular symbols.

I can wrap everything in here in an anonymous namespace if you want, if it’s about hiding them – is there a reason we don’t do that in our other source files?

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

My thinking was that e.g. SerializerContext::New() is a generic enough name that it might cause symbol clashes when another file introduces the same class/method (so yes, hiding.)

is there a reason we don’t do that in our other source files?

No reason except inertia. I give most new code I write inline linkage.

(Also, I like inline better than namespace { ... } because the former doesn't require me to scroll up to figure out the namespace we're in. Maybe I should use a fancier editor.)

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

My thinking was that e.g. SerializerContext::New() is a generic enough name that it might cause symbol clashes when another file introduces the same class/method (so yes, hiding.)

is there a reason we don’t do that in our other source files?

No reason except inertia. I give most new code I write inline linkage.

(Also, I like inline better than namespace { ... } because the former doesn't require me to scroll up to figure out the namespace we're in. Maybe I should use a fancier editor.)

Show outdated Hide outdated src/node_serdes.cc
void SerializerContext::ThrowDataCloneError(Local<String> message) {
Local<Value> args[1] = { message };
Local<Value> getDataCloneError =

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

get_data_clone_error? Same suggestion applies to a couple of places further down.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

get_data_clone_error? Same suggestion applies to a couple of places further down.

Show outdated Hide outdated src/node_serdes.cc
SerializerContext::SerializerContext(Environment* env,
Local<Object> wrap,
Local<Value> throwDataCloneError,
Local<Value> writeHostObject)

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Using camelCase for params is a minor stylistic issue although I kind of see why you're doing that here.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Using camelCase for params is a minor stylistic issue although I kind of see why you're doing that here.

Show outdated Hide outdated src/node_serdes.cc
deserializer_(env->isolate(), data_, length_, this) {
object()->Set(env->context(), env->buffer_string(), buffer);
object()->Set(env->context(), env->read_host_object_string(), readHostObject);
deserializer_.SetSupportsLegacyWireFormat(true);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Maybe explain in a comment that otherwise it always throws an "invalid or unsupported version" exception.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Maybe explain in a comment that otherwise it always throws an "invalid or unsupported version" exception.

Show outdated Hide outdated src/node_serdes.cc
Local<Value> retValue = ret.ToLocalChecked();
if (!retValue->IsObject()) {
env()->ThrowTypeError("readHostObject must return an object");

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

It's not a blocker but I admit to not being fond of code that throws JS exceptions as a side effect.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

It's not a blocker but I admit to not being fond of code that throws JS exceptions as a side effect.

This comment has been minimized.

@addaleax

addaleax Feb 4, 2017

Member

It's not a blocker but I admit to not being fond of code that throws JS exceptions as a side effect.

What alternative would you prefer? I think I’d like this better than a CHECK or just not doing any type checking.

@addaleax

addaleax Feb 4, 2017

Member

It's not a blocker but I admit to not being fond of code that throws JS exceptions as a side effect.

What alternative would you prefer? I think I’d like this better than a CHECK or just not doing any type checking.

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

I was thinking of a Local<Value>* exception_out parameter but, on second thought, since the function->Call() can raise an exception anyway, that's pointless. Never mind.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

I was thinking of a Local<Value>* exception_out parameter but, on second thought, since the function->Call() can raise an exception anyway, that's pointless. Never mind.

Maybe<bool> ret = ctx->deserializer_.ReadHeader(ctx->env()->context());
if (ret.IsJust()) args.GetReturnValue().Set(ret.FromJust());

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Consistency: you just .FromMaybe(false) without an if elsewhere. I think I like this approach better, though; it's Obviously Correct, even if it's a little more verbose.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Consistency: you just .FromMaybe(false) without an if elsewhere. I think I like this approach better, though; it's Obviously Correct, even if it's a little more verbose.

Show outdated Hide outdated src/node_serdes.cc
ser->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Serializer"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "Serializer"),
ser->GetFunction());

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Can you use the overload that takes a v8::Context?

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Can you use the overload that takes a v8::Context?

Show outdated Hide outdated src/node_serdes.cc
des->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Deserializer"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "Deserializer"),
des->GetFunction());

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Ditto. There are a few other places where you use old-style (implicit-style?) methods, such as the no-arg overloads of NumberValue() and Uint32Value().

@bnoordhuis

bnoordhuis Feb 3, 2017

Member

Ditto. There are a few other places where you use old-style (implicit-style?) methods, such as the no-arg overloads of NumberValue() and Uint32Value().

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Feb 4, 2017

Member

@bnoordhuis I should have addressed most of your comments, PTAL

Member

addaleax commented Feb 4, 2017

@bnoordhuis I should have addressed most of your comments, PTAL

@bnoordhuis

LGTM with some final comments.

static void ReadUint32(const FunctionCallbackInfo<Value>& args);
static void ReadUint64(const FunctionCallbackInfo<Value>& args);
static void ReadDouble(const FunctionCallbackInfo<Value>& args);
static void ReadRawBytes(const FunctionCallbackInfo<Value>& args);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

My thinking was that e.g. SerializerContext::New() is a generic enough name that it might cause symbol clashes when another file introduces the same class/method (so yes, hiding.)

is there a reason we don’t do that in our other source files?

No reason except inertia. I give most new code I write inline linkage.

(Also, I like inline better than namespace { ... } because the former doesn't require me to scroll up to figure out the namespace we're in. Maybe I should use a fancier editor.)

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

My thinking was that e.g. SerializerContext::New() is a generic enough name that it might cause symbol clashes when another file introduces the same class/method (so yes, hiding.)

is there a reason we don’t do that in our other source files?

No reason except inertia. I give most new code I write inline linkage.

(Also, I like inline better than namespace { ... } because the former doesn't require me to scroll up to figure out the namespace we're in. Maybe I should use a fancier editor.)

Show outdated Hide outdated src/node_serdes.cc
MaybeLocal<Value> error =
get_data_clone_error.As<Function>()->Call(env()->context(),
object(),
1,

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Prefer arraysize(args) if you're passing an array.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Prefer arraysize(args) if you're passing an array.

Show outdated Hide outdated src/node_serdes.cc
ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Holder());
Maybe<bool> ret =
ctx->serializer_.WriteValue(ctx->env()->context(),
args[0]);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Fits on one or two lines, doesn't it?

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Fits on one or two lines, doesn't it?

Show outdated Hide outdated src/node_serdes.cc
DeserializerContext::DeserializerContext(Environment* env,
Local<Object> wrap,
Local<Value> buffer,
Local<Value> readHostObject)

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

camelCase leftOver?

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

camelCase leftOver?

Show outdated Hide outdated src/node_serdes.cc
}
MaybeLocal<Object> DeserializerContext::ReadHostObject(Isolate* isolate) {
Local<Value> readHostObject =

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Likewise.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Likewise.

Show outdated Hide outdated src/node_serdes.cc
Local<Value> retValue = ret.ToLocalChecked();
if (!retValue->IsObject()) {
env()->ThrowTypeError("readHostObject must return an object");

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

I was thinking of a Local<Value>* exception_out parameter but, on second thought, since the function->Call() can raise an exception anyway, that's pointless. Never mind.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

I was thinking of a Local<Value>* exception_out parameter but, on second thought, since the function->Call() can raise an exception anyway, that's pointless. Never mind.

Show outdated Hide outdated src/node_serdes.cc
if (ret.IsEmpty())
return MaybeLocal<Object>();
Local<Value> retValue = ret.ToLocalChecked();

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

return_value or rval if you think that's too wordy.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

return_value or rval if you think that's too wordy.

Show outdated Hide outdated src/node_serdes.cc
DeserializerContext* ctx;
ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Holder());
Maybe<double> length_arg = args[0]->NumberValue(ctx->env()->context());

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

IntegerValue()? It returns an int64_t.

@bnoordhuis

bnoordhuis Feb 14, 2017

Member

IntegerValue()? It returns an int64_t.

can't figure out what changes I requested

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Feb 15, 2017

Member

@bnoordhuis Updated, but I’d like to wait a bit and rebase this on the V8 5.7 API + wait for https://bugs.chromium.org/p/v8/issues/detail?id=5926 to be resolved (unless you think that that’s silly).

Member

addaleax commented Feb 15, 2017

@bnoordhuis Updated, but I’d like to wait a bit and rebase this on the V8 5.7 API + wait for https://bugs.chromium.org/p/v8/issues/detail?id=5926 to be resolved (unless you think that that’s silly).

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 15, 2017

Member

Seems worthwhile to wait on but whatever you prefer.

Member

bnoordhuis commented Feb 15, 2017

Seems worthwhile to wait on but whatever you prefer.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 10, 2017

Member

Ok, I’ve rebased this on top of #11752 (the V8 5.7 update). I’ve also included a few cherry-picks from V8 that are not in 5.7 here. The only one that would be required for this PR is c4f3651 (cherry-pick of v8/v8@3b15d95), which is a purely additive API change; the rest of them are bugfixes and/or bring the wire format up to date.

@targos I would be grateful if you would consider 4820afa7026a...ada07efe5aaa for inclusion in your 5.7 update. I realize that’s quite a few changes, but they don’t touch anything besides the V8 side of the value serializer (and d8). I’ve reviewed them and used my best judgement for choosing which to include. It’s also a part of the V8 codebase where I’d feel comfortable saying that I can fix problems if any show up.

The big upside of doing that would be that future V8 upgrades are much less likely to force a breaking change of this API, since the wire format should now be stable (see the commit message of v8/v8@a927f81).

If you decide against that, which would be perfectly understandable, I’ll drop everything except for v8/v8@3b15d95 here.

CI: https://ci.nodejs.org/job/node-test-commit/8357/ (edit: earlier link here failed because I missed the merge conflict here)
V8 CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/602/

Member

addaleax commented Mar 10, 2017

Ok, I’ve rebased this on top of #11752 (the V8 5.7 update). I’ve also included a few cherry-picks from V8 that are not in 5.7 here. The only one that would be required for this PR is c4f3651 (cherry-pick of v8/v8@3b15d95), which is a purely additive API change; the rest of them are bugfixes and/or bring the wire format up to date.

@targos I would be grateful if you would consider 4820afa7026a...ada07efe5aaa for inclusion in your 5.7 update. I realize that’s quite a few changes, but they don’t touch anything besides the V8 side of the value serializer (and d8). I’ve reviewed them and used my best judgement for choosing which to include. It’s also a part of the V8 codebase where I’d feel comfortable saying that I can fix problems if any show up.

The big upside of doing that would be that future V8 upgrades are much less likely to force a breaking change of this API, since the wire format should now be stable (see the commit message of v8/v8@a927f81).

If you decide against that, which would be perfectly understandable, I’ll drop everything except for v8/v8@3b15d95 here.

CI: https://ci.nodejs.org/job/node-test-commit/8357/ (edit: earlier link here failed because I missed the merge conflict here)
V8 CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/602/

Old review

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 11, 2017

Member

Thank you @addaleax! I don't see anything wrong about including the commits in the 5.7 update. I cherry-picked all of them and squashed the ones that are in 5.8-lkgr.
It would be nice if we didn't have to float the remaining two in 5.8 (511d8e5 and ada07ef). Would you like to try to ask for a merge?

Member

targos commented Mar 11, 2017

Thank you @addaleax! I don't see anything wrong about including the commits in the 5.7 update. I cherry-picked all of them and squashed the ones that are in 5.8-lkgr.
It would be nice if we didn't have to float the remaining two in 5.8 (511d8e5 and ada07ef). Would you like to try to ask for a merge?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 11, 2017

Member

@targos All of the commits here (the most recent one is v8/v8@a927f81) are in some 5.8 version:

$ git tag --contains a927f81c7c16e6c3d53e590404c954e523b115db | sort -V | head
5.8.300
5.8.301
5.9.1

I’m not sure, maybe I’m misunderstanding how the V8 versioning system works, but doesn’t that mean that the 5.8-lkgr branch needs to be updated in some way? In any case, I am not sure how I’d fill out a merge request under these circumstances. 😄

Member

addaleax commented Mar 11, 2017

@targos All of the commits here (the most recent one is v8/v8@a927f81) are in some 5.8 version:

$ git tag --contains a927f81c7c16e6c3d53e590404c954e523b115db | sort -V | head
5.8.300
5.8.301
5.9.1

I’m not sure, maybe I’m misunderstanding how the V8 versioning system works, but doesn’t that mean that the 5.8-lkgr branch needs to be updated in some way? In any case, I am not sure how I’d fill out a merge request under these circumstances. 😄

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 11, 2017

Member

I don't know exactly how it works but the release branch is always cut from an earlier version before the bump (I guess it's related to Canary coverage). In that case, 5.8-lkgr is based on 5.8.283.
You can use this template to make a merge request.

Edit: the merge process is documented here: https://github.com/v8/v8/wiki/Merging%20&%20Patching

Member

targos commented Mar 11, 2017

I don't know exactly how it works but the release branch is always cut from an earlier version before the bump (I guess it's related to Canary coverage). In that case, 5.8-lkgr is based on 5.8.283.
You can use this template to make a merge request.

Edit: the merge process is documented here: https://github.com/v8/v8/wiki/Merging%20&%20Patching

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 12, 2017

Member

@targos tried to catch you on IRC but since I’m going away for today, here’s the c&p:

[00:49] addaleax: targos: What would the versions I would put down in the template be? “On what Canary was the fix deployed first?” is 5.8.300 and “Into which version should it be merged?” is 5.8?
[00:49] addaleax: that just seems so odd

EDIT: https://bugs.chromium.org/p/v8/issues/detail?id=6080

Member

addaleax commented Mar 12, 2017

@targos tried to catch you on IRC but since I’m going away for today, here’s the c&p:

[00:49] addaleax: targos: What would the versions I would put down in the template be? “On what Canary was the fix deployed first?” is 5.8.300 and “Into which version should it be merged?” is 5.8?
[00:49] addaleax: that just seems so odd

EDIT: https://bugs.chromium.org/p/v8/issues/detail?id=6080

targos added a commit to targos/node that referenced this pull request Mar 23, 2017

deps: cherry-pick V8 ValueSerializer changes
Refs: nodejs#11048

Below is the list of commits:

deps: cherry-pick 78c0be52d from V8 upstream

Original commit message:

  ValueSerializer: Promote scheduled exceptions from wasm::ErrorThrower.

  wasm::ErrorThrower doesn't actually throw exceptions, it just schedules them.
  As a result, this exception isn't handled properly by code which expects
  ValueDeserializer to actually throw. For instance, the unit tests use a
  TryCatch to catch and handle expected exceptions in unit tests.

  Before this patch, I see local unit test failures because a wasm decode test
  schedules one, but it isn't caught (and instead causes Context::New to fail
  at the beginning of the next test).

  BUG=685713

  Review-Url: https://codereview.chromium.org/2659483004
  Cr-Commit-Position: refs/heads/master@{#42718}

deps: cherry-pick 966355585 from V8 upstream

Original commit message:

  [d8] Use ValueSerializer for postMessage (instead of ad-hoc serializer)

  Review-Url: https://codereview.chromium.org/2643723010
  Cr-Commit-Position: refs/heads/master@{#42749}

deps: cherry-pick bf511b426 from V8 upstream

Original commit message:

  ValueSerializer: Support efficiently reading and writing one-byte strings.

  memcpy is faster than UTF-8 encoding/decoding. This yields 10-20% wins on
  serializing and deserializing long ASCII strings, according to
  blink_perf.bindings -- and these are already in a fast path where the entire
  string is known to be ASCII (but this has to be checked). The win may be
  larger for strings in Latin-1 but not ASCII (though I suspect this is an
  uncommon case).

  A change is also made to make ValueSerializerTest.EncodeTwoByteStringUsesPadding
  survive wire format version number changes.

  This is the first of a series of wire format changes from the previous Blink
  format. The deserializer continues to be able to read the old format, but
  Chromium M56 will no longer be able to read the messages written by this, in M58.

  BUG=chromium:686159

  Review-Url: https://codereview.chromium.org/2658793004
  Cr-Commit-Position: refs/heads/master@{#42753}

deps: cherry-pick 6f1639ed1 from V8 upstream

Original commit message:

  ValueSerializer: Distinguish between 'undefined' and an absent property.

  Dealing with this case requires a wire format change. It is possible that an
  element can be absent even in an array where the dense format was chosen
  (because the array initially had no holes), if the elements are modified while
  they are being serialized. In this case, a new tag for the "hole" is emitted.

  The logic to treat undefined in dense arrays as an absent property is restricted
  to versions of the wire format that this tag did not exist.

  BUG=chromium:686159,chromium:665820

  Review-Url: https://codereview.chromium.org/2660093002
  Cr-Original-Commit-Position: refs/heads/master@{#42784}
  Committed: https://chromium.googlesource.com/v8/v8/+/dc85f4c8338c1c824af4f7ee3274dc9f95d14e49
  Review-Url: https://codereview.chromium.org/2660093002
  Cr-Commit-Position: refs/heads/master@{#42800}

deps: cherry-pick c3856de37 from V8 upstream

Original commit message:

  ValueSerializer: Check for zero length before casting to FixedDoubleArray.

  Even though the elements kind is FAST_DOUBLE_ELEMENTS, if length is zero
  the isolate's empty_fixed_array is used. It's illegal to cast this to
  FixedDoubleArray, so we avoid the cast.

  BUG=chromium:686479

  Review-Url: https://codereview.chromium.org/2665313003
  Cr-Commit-Position: refs/heads/master@{#42867}

deps: cherry-pick 591cc0b4c from V8 upstream

Original commit message:

  ValueSerializer: Share string encoding code with String and RegExp objects.

  This avoids the need to pull in the UTF-8 encoding code from the public API,
  and allows it to take advantage of any supported way that i::String can be
  encoded (one- or two-byte).

  Backward compatibility is maintained, but this is the behavior beginning
  with this version.

  BUG=chromium:686159

  Review-Url: https://codereview.chromium.org/2665653004
  Cr-Commit-Position: refs/heads/master@{#42872}

deps: cherry-pick 79837f5f6 from V8 upstream

Original commit message:

  Improve ValueSerializer perf regression after 96635558

  BUG=687196
  R=jbroman@chromium.org

  Review-Url: https://codereview.chromium.org/2674613002
  Cr-Commit-Position: refs/heads/master@{#42938}

deps: cherry-pick 8990399dc from V8 upstream

Original commit message:

  ValueDeserializer: Only allow valid keys when deserializing object properties.

  The serializer won't ever write a more complex object. Not validating this
  allows other things to be used as keys, and converted to string when the
  property set actually occurs. It turns out this gives an opportunity to trigger
  OOM by giving an object a key which is a very large sparse array (whose string
  representation is very large).

  This case is now rejected by the deserializer.

  BUG=chromium:686511

  Review-Url: https://codereview.chromium.org/2697023002
  Cr-Commit-Position: refs/heads/master@{#43249}

deps: cherry-pick 68960eeb7 from V8 upstream

Original commit message:

  ValueDeserializer: Make sure that an exception is the legacy path.

  The entry points to the deserializer are responsible for ensuring that an
  exception is pending by the time they return. Some failures throw exceptions
  themselves, while others (like errors in the format) are exceptions caused by
  the deserializer, not coming from the runtime.

  Like the non-legacy path, a default deserialization exception should be thrown
  in such cases.

  BUG=chromium:693411

  Review-Url: https://codereview.chromium.org/2712713002
  Cr-Commit-Position: refs/heads/master@{#43390}

deps: cherry-pick 3b15d950e from V8 upstream

Original commit message:
  ValueSerializer: Add SetTreatArrayBufferViewsAsHostObjects() flag

  Add `ValueSerializer::SetTreatArrayBufferViewsAsHostObjects()` which
  instructs the `ValueSerializer` to treat ArrayBufferView objects as
  host objects.

  BUG=v8:5926

  Review-Url: https://codereview.chromium.org/2696133007
  Cr-Commit-Position: refs/heads/master@{#43281}

deps: cherry-pick 654351997 from V8 upstream

Original commit message:

  ValueSerializer: Add an explicit tag for host objects.

  This makes it no longer necessary to ensure that V8 and Blink have non-colliding
  tags, which makes it easier for them to evolve independently, and also makes
  the wire format more suitable for other V8 embedders, who would not
  necessarily be surveyed before V8 introduced a new tag that might collide
  with theirs.

  BUG=chromium:686159

  Review-Url: https://codereview.chromium.org/2709023003
  Cr-Commit-Position: refs/heads/master@{#43466}

@targos targos referenced this pull request Mar 23, 2017

Merged

deps: update V8 to 5.7 #11752

2 of 2 tasks complete

targos added a commit to targos/node that referenced this pull request Mar 25, 2017

deps: cherry-pick V8 ValueSerializer changes
Refs: nodejs#11048

Below is the list of commits:

deps: cherry-pick 78c0be52d from V8 upstream

Original commit message:

  ValueSerializer: Promote scheduled exceptions from wasm::ErrorThrower.

  wasm::ErrorThrower doesn't actually throw exceptions, it just schedules them.
  As a result, this exception isn't handled properly by code which expects
  ValueDeserializer to actually throw. For instance, the unit tests use a
  TryCatch to catch and handle expected exceptions in unit tests.

  Before this patch, I see local unit test failures because a wasm decode test
  schedules one, but it isn't caught (and instead causes Context::New to fail
  at the beginning of the next test).

  BUG=685713

  Review-Url: https://codereview.chromium.org/2659483004
  Cr-Commit-Position: refs/heads/master@{#42718}

deps: cherry-pick 966355585 from V8 upstream

Original commit message:

  [d8] Use ValueSerializer for postMessage (instead of ad-hoc serializer)

  Review-Url: https://codereview.chromium.org/2643723010
  Cr-Commit-Position: refs/heads/master@{#42749}

deps: cherry-pick bf511b426 from V8 upstream

Original commit message:

  ValueSerializer: Support efficiently reading and writing one-byte strings.

  memcpy is faster than UTF-8 encoding/decoding. This yields 10-20% wins on
  serializing and deserializing long ASCII strings, according to
  blink_perf.bindings -- and these are already in a fast path where the entire
  string is known to be ASCII (but this has to be checked). The win may be
  larger for strings in Latin-1 but not ASCII (though I suspect this is an
  uncommon case).

  A change is also made to make ValueSerializerTest.EncodeTwoByteStringUsesPadding
  survive wire format version number changes.

  This is the first of a series of wire format changes from the previous Blink
  format. The deserializer continues to be able to read the old format, but
  Chromium M56 will no longer be able to read the messages written by this, in M58.

  BUG=chromium:686159

  Review-Url: https://codereview.chromium.org/2658793004
  Cr-Commit-Position: refs/heads/master@{#42753}

deps: cherry-pick 6f1639ed1 from V8 upstream

Original commit message:

  ValueSerializer: Distinguish between 'undefined' and an absent property.

  Dealing with this case requires a wire format change. It is possible that an
  element can be absent even in an array where the dense format was chosen
  (because the array initially had no holes), if the elements are modified while
  they are being serialized. In this case, a new tag for the "hole" is emitted.

  The logic to treat undefined in dense arrays as an absent property is restricted
  to versions of the wire format that this tag did not exist.

  BUG=chromium:686159,chromium:665820

  Review-Url: https://codereview.chromium.org/2660093002
  Cr-Original-Commit-Position: refs/heads/master@{#42784}
  Committed: https://chromium.googlesource.com/v8/v8/+/dc85f4c8338c1c824af4f7ee3274dc9f95d14e49
  Review-Url: https://codereview.chromium.org/2660093002
  Cr-Commit-Position: refs/heads/master@{#42800}

deps: cherry-pick c3856de37 from V8 upstream

Original commit message:

  ValueSerializer: Check for zero length before casting to FixedDoubleArray.

  Even though the elements kind is FAST_DOUBLE_ELEMENTS, if length is zero
  the isolate's empty_fixed_array is used. It's illegal to cast this to
  FixedDoubleArray, so we avoid the cast.

  BUG=chromium:686479

  Review-Url: https://codereview.chromium.org/2665313003
  Cr-Commit-Position: refs/heads/master@{#42867}

deps: cherry-pick 591cc0b4c from V8 upstream

Original commit message:

  ValueSerializer: Share string encoding code with String and RegExp objects.

  This avoids the need to pull in the UTF-8 encoding code from the public API,
  and allows it to take advantage of any supported way that i::String can be
  encoded (one- or two-byte).

  Backward compatibility is maintained, but this is the behavior beginning
  with this version.

  BUG=chromium:686159

  Review-Url: https://codereview.chromium.org/2665653004
  Cr-Commit-Position: refs/heads/master@{#42872}

deps: cherry-pick 79837f5f6 from V8 upstream

Original commit message:

  Improve ValueSerializer perf regression after 96635558

  BUG=687196
  R=jbroman@chromium.org

  Review-Url: https://codereview.chromium.org/2674613002
  Cr-Commit-Position: refs/heads/master@{#42938}

deps: cherry-pick 8990399dc from V8 upstream

Original commit message:

  ValueDeserializer: Only allow valid keys when deserializing object properties.

  The serializer won't ever write a more complex object. Not validating this
  allows other things to be used as keys, and converted to string when the
  property set actually occurs. It turns out this gives an opportunity to trigger
  OOM by giving an object a key which is a very large sparse array (whose string
  representation is very large).

  This case is now rejected by the deserializer.

  BUG=chromium:686511

  Review-Url: https://codereview.chromium.org/2697023002
  Cr-Commit-Position: refs/heads/master@{#43249}

deps: cherry-pick 68960eeb7 from V8 upstream

Original commit message:

  ValueDeserializer: Make sure that an exception is the legacy path.

  The entry points to the deserializer are responsible for ensuring that an
  exception is pending by the time they return. Some failures throw exceptions
  themselves, while others (like errors in the format) are exceptions caused by
  the deserializer, not coming from the runtime.

  Like the non-legacy path, a default deserialization exception should be thrown
  in such cases.

  BUG=chromium:693411

  Review-Url: https://codereview.chromium.org/2712713002
  Cr-Commit-Position: refs/heads/master@{#43390}

deps: cherry-pick 3b15d950e from V8 upstream

Original commit message:
  ValueSerializer: Add SetTreatArrayBufferViewsAsHostObjects() flag

  Add `ValueSerializer::SetTreatArrayBufferViewsAsHostObjects()` which
  instructs the `ValueSerializer` to treat ArrayBufferView objects as
  host objects.

  BUG=v8:5926

  Review-Url: https://codereview.chromium.org/2696133007
  Cr-Commit-Position: refs/heads/master@{#43281}

deps: cherry-pick 654351997 from V8 upstream

Original commit message:

  ValueSerializer: Add an explicit tag for host objects.

  This makes it no longer necessary to ensure that V8 and Blink have non-colliding
  tags, which makes it easier for them to evolve independently, and also makes
  the wire format more suitable for other V8 embedders, who would not
  necessarily be surveyed before V8 introduced a new tag that might collide
  with theirs.

  BUG=chromium:686159

  Review-Url: https://codereview.chromium.org/2709023003
  Cr-Commit-Position: refs/heads/master@{#43466}

PR-URL: nodejs#11752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 25, 2017

Member

Rebased now that V8 5.7 has landed.

CI: https://ci.nodejs.org/job/node-test-commit/8687/

@bnoordhuis Mind taking another look?

Member

addaleax commented Mar 25, 2017

Rebased now that V8 5.7 has landed.

CI: https://ci.nodejs.org/job/node-test-commit/8687/

@bnoordhuis Mind taking another look?

@targos targos self-assigned this Mar 25, 2017

@addaleax addaleax removed the blocked label Mar 25, 2017

@bnoordhuis

Still LGTM at a quick glance.

@@ -80,3 +87,110 @@ exports.getHeapSpaceStatistics = function() {
return heapSpaceStatistics;
};
/* V8 serialization API */

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 26, 2017

Member

Tiniest of style nits but is there a reason for mixing C and C++-style comments?

@bnoordhuis

bnoordhuis Mar 26, 2017

Member

Tiniest of style nits but is there a reason for mixing C and C++-style comments?

This comment has been minimized.

@addaleax

addaleax Mar 27, 2017

Member

@bnoordhuis I would say C-style comments feel a bit more heading-y than C++-style comments? I never consciously noticed but I think I use // for text that only refers to the next one or two statements, whereas /* … */ refers to a longer section of code. That also seems to match how we use eslint-disable comments in our codebase.

If you feel strongly about it, I have no problem changing the format in either way. :)

@addaleax

addaleax Mar 27, 2017

Member

@bnoordhuis I would say C-style comments feel a bit more heading-y than C++-style comments? I never consciously noticed but I think I use // for text that only refers to the next one or two statements, whereas /* … */ refers to a longer section of code. That also seems to match how we use eslint-disable comments in our codebase.

If you feel strongly about it, I have no problem changing the format in either way. :)

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 27, 2017

Member

Never looked at it that way. I suppose it's fine, lib/ is a mixture of both; // is more prevalent but that's probably also because of the copyright header.

@bnoordhuis

bnoordhuis Mar 27, 2017

Member

Never looked at it that way. I suppose it's fine, lib/ is a mixture of both; // is more prevalent but that's probably also because of the copyright header.

addaleax added some commits Jan 27, 2017

v8: expose new V8 serialization API
Expose the new serialization API that was added in V8 5.5 to userland.
The JS API is virtually a direct copy of what V8 provides on the
C++ level.

This is useful Node as a possible replacement for some internals
that currently use JSON, like IPC, but is likely to be useful to
general userland code as well.
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 27, 2017

Member

@targos Does you assigning this to yourself mean that I should wait for a review from you before merging, or that you would like to be the person who merges this, or something else?

Member

addaleax commented Mar 27, 2017

@targos Does you assigning this to yourself mean that I should wait for a review from you before merging, or that you would like to be the person who merges this, or something else?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 27, 2017

Member

@addaleax I'd like to review this, yes. Probably later today.

Member

targos commented Mar 27, 2017

@addaleax I'd like to review this, yes. Probably later today.

This throws an error if `value` cannot be serialized.
#### serializer.releaseBuffer()

This comment has been minimized.

@targos

targos Mar 27, 2017

Member

If we want to keep close to the V8 API, this should be called release(). Did you keep it like that because a Buffer is returned?

@targos

targos Mar 27, 2017

Member

If we want to keep close to the V8 API, this should be called release(). Did you keep it like that because a Buffer is returned?

This comment has been minimized.

@addaleax

addaleax Mar 27, 2017

Member

@targos I would assume V8 calling it Release is because ReleaseBuffer was already taken by the legacy method. But yeah, it’s nice that releaseBuffer() tells you the (otherwise not obvious) return type.

@addaleax

addaleax Mar 27, 2017

Member

@targos I would assume V8 calling it Release is because ReleaseBuffer was already taken by the legacy method. But yeah, it’s nice that releaseBuffer() tells you the (otherwise not obvious) return type.

@targos

targos approved these changes Mar 27, 2017

doc, JS and tests LGTM

Show outdated Hide outdated lib/v8.js
i = bufferConstructorIndex;
} else {
i = arrayBufferViewTypeToIndex.get(objectToString(abView));
}

This comment has been minimized.

@targos

targos Mar 27, 2017

Member

Shouldn't this throw an error when i is undefined (unknown host object)?

@targos

targos Mar 27, 2017

Member

Shouldn't this throw an error when i is undefined (unknown host object)?

This comment has been minimized.

@addaleax

addaleax Mar 27, 2017

Member

@targos Yes thanks for catching! Updated + test added

@addaleax

addaleax Mar 27, 2017

Member

@targos Yes thanks for catching! Updated + test added

@targos targos removed their assignment Mar 27, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Mar 29, 2017

Landed in 9b2dd47...1fde98b

@addaleax addaleax closed this Mar 29, 2017

@addaleax addaleax deleted the addaleax:v8-serdes branch Mar 29, 2017

addaleax added a commit that referenced this pull request Mar 29, 2017

buffer: expose FastBuffer on internal/buffer
PR-URL: #11048
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax added a commit that referenced this pull request Mar 29, 2017

v8: expose new V8 serialization API
Expose the new serialization API that was added in V8 5.5 to userland.
The JS API is virtually a direct copy of what V8 provides on the
C++ level.

This is useful Node as a possible replacement for some internals
that currently use JSON, like IPC, but is likely to be useful to
general userland code as well.

PR-URL: #11048
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax added a commit that referenced this pull request Mar 31, 2017

src: add .FromJust(), fix -Wunused-result warnings
Missed while reviewing 1fde98b ("v8: expose new V8 serialization API.")

PR-URL: #12118
Refs: #11048
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment