Call with V8 team - Githubissues

indutny commented 7 years ago

Hello everyone!

I think we should coordinate and schedule a joint call between Node.js CTC and V8 team to discuss following topic:

GYP vs GN, and how both projects could move forward on this
General performance feedback

cc @nodejs/ctc @bmeurer

sam-github commented 7 years ago

Object.assign()!

hashseed commented 7 years ago

cc @ofrobots

bmeurer commented 7 years ago

Would be really interested to hear feedback for Object.assign(). We spend some time optimizing that last year, but maybe there's more we can do!

sam-github commented 7 years ago

@cjihrig @Trott Why do we think its slower than util._extend(), and to be avoided?

cjihrig commented 7 years ago

I think every time someone benchmarked the two, util._extend() came out faster. That's not to say that Object.assign() might have improved since last we checked. Here are some related PRs:

People have reported seeing util._extend() win by up to 50%, although, again, that might not currently be the case. FWIW, I'm in favor of using Object.assign() if the performance has improved enough.

EDIT: A benchmark was provided in https://github.com/nodejs/node/pull/7255.

Trott commented 7 years ago

Here are two different benchmarks with conflicting results. So which one is faster may depend on your use case.

Here's the one showing `util._extend()` being faster.

Here is a slightly modified version of a benchmark originally written by @jasnell. #### Code ```js 'use strict'; const common = require('../common'); const util = require('util'); const bench = common.createBenchmark(main, { method: ['Object.assign()', 'util._extend()'], size: [5, 10, 15, 30], n: [1e5] }); function makeObject(size) { var m = {}; for (var n = 0; n < size; n++) m[`key${n}`] = n; return m; } function useObjectAssign(n, obj) { bench.start(); for (var i = 0; i < n; i++) { Object.assign({}, obj); } bench.end(n); } function useUtilExtend(n, obj) { bench.start(); for (var i = 0; i < n; i++) { util._extend({}, obj); } bench.end(n); } function main(conf) { const n = +conf.n; const obj = makeObject(+conf.size); switch (conf.method) { case 'Object.assign()': useObjectAssign(n, obj); break; case 'util._extend()': useUtilExtend(n, obj); break; default: throw new Error('Unexpected method'); } } ``` #### Results ```console es/more-different-object-assign.js n=100000 size=5 method="Object.assign()": 1,250,221.8987592561 es/more-different-object-assign.js n=100000 size=10 method="Object.assign()": 631,233.9839762895 es/more-different-object-assign.js n=100000 size=15 method="Object.assign()": 370,360.6970976447 es/more-different-object-assign.js n=100000 size=30 method="Object.assign()": 91,588.78644709584 es/more-different-object-assign.js n=100000 size=5 method="util._extend()": 4,757,977.84419069 es/more-different-object-assign.js n=100000 size=10 method="util._extend()": 2,845,257.808810056 es/more-different-object-assign.js n=100000 size=15 method="util._extend()": 2,240,614.63106566 es/more-different-object-assign.js n=100000 size=30 method="util._extend()": 253,510.71525401654 ```

Here's the one showing `Object.assign()` being faster.

This is a modified version of a benchmark originally written by @suryagh. #### Code ```js 'use strict'; const common = require('../common.js'); const util = require('util'); const bench = common.createBenchmark(main, { n: [1, 10, 100, 1000], method: ['_extend', 'assign'] }); const methods = { '_extend': util._extend, 'assign': Object.assign } function main(conf) { const n = +conf.n; const method = methods[conf.method]; const target = {}; bench.start() for (var i = 0; i < n; i++) { method(target, {i: i}); } bench.end(n) } ``` #### Results ```console es/object-assign.js method="_extend" n=1: 11,816.559726801139 es/object-assign.js method="assign" n=1: 13,891.590031394993 es/object-assign.js method="_extend" n=10: 73,593.80634525798 es/object-assign.js method="assign" n=10: 74,392.03112562583 es/object-assign.js method="_extend" n=100: 646,663.2177961718 es/object-assign.js method="assign" n=100: 716,707.1606212418 es/object-assign.js method="_extend" n=1000: 2,613,279.116502596 es/object-assign.js method="assign" n=1000: 3,053,313.914256839 ```

andrasq commented 7 years ago

they're different tests -- one copies an object with 10,15,30 fields, the other transfers 10,100,1000 properties to an existing object one property at a time.

Trott commented 7 years ago

they're different tests -- one copies an object with 10,15,30 fields, the other transfers 10,100,1000 properties to an existing object one property at a time.

Yes, I probably shouldn't have said "conflicting". I didn't mean "contradictory" or "paradoxical" or anything like that. I just meant what results you get depend on what you're testing. Although I guess I should also add that the one showing util._extend() faster is probably the more realistic/typical use.

bmeurer commented 7 years ago

cc @verwaest who did the Object.assign() improvements.

jasnell commented 7 years ago

I was just playing with this a bit earlier this week. util._extend() consistently outperformed Object.assign() by about 200-300%. The different was significant. I included a new benchmark test in https://github.com/nodejs/node/pull/10849

andrasq commented 7 years ago

I can confirm the above, _extend is 2-5x faster. The exceptions are the empty object {} (assign 2x faster) and objects with a single property (30% faster), and object whose property names are numeric strings. The last two are why the test that copies properties singly favors assign.

Also, there is an order of magnitude speed difference between copying objects whose properties are indexed by numeric strings "123" vs words "a123" (indexing by words up to 12x faster), but this has to be due to v8 internals. The speed to copy gradually slows for large numeric values up to 4 billion (2^32); above 2^32 numeric strings run as fast as words. This quirk can bias benchmarks.

E.g., extend/assign relative rates for an object with 10 properties: property names numeric strings '0' .. '9': extend: 1.00x assign: 1.25x property names '1230' .. '1239': extend: 1.18x assign: 0.90x property names '4000000000' ... '4000000009': extend: 0.45x assign: 0.80x property names words 'a0' .. 'a9': extend: 12x assign: 1.9x (timed with node-v7.4.0)

(Unrelated, but surprisingly _extend reverses the order of the properties; _extend({}, {a:1, b:2}) yields the unexpected {b:2, a:1}.)

verwaest-zz commented 7 years ago

There is definitely still room for improvement in Object.assign. We'll should to look into it again sometime soon.

OTOH you have to be careful with microbenchmarks for hand rolled versions because they may look fast in isolation but won't always give good perf when used in an application. They will quickly really on a shared cache that only starts conflicting when there are other users as well. Object.assign doesn't have that issue.

On Thu, Jan 19, 2017, 20:03 Andras notifications@github.com wrote:

I can confirm the above, _extend is 2-5x faster. The exceptions are the empty object {} (assign 2x faster) and objects with a single property (30% faster), and object whose property names are numeric strings. The last two are why the test that copies properties singly favors assign.

Also, there is an order of magnitude speed difference between copying objects whose properties are indexed by numeric strings "123" vs words "a123" (indexing by words up to 12x faster), but this has to be due to v8 internals. The speed to copy gradually slows for large numeric values up to 4 billion (2^32); above 2^32 numeric strings run as fast as words. This quirk can bias benchmarks.

E.g., extend/assign relative rates for an object with 10 properties: property names numeric strings '0' .. '9': extend: 1.00x assign: 1.25x property names '1230' .. '1239': extend: 1.18x assign: 0.90x property names '4000000000' ... '4000000009': extend: 0.45x assign: 0.80x property names words 'a0' .. 'a9': extend: 12x assign: 1.9x (timed with node-v7.4.0)

(Unrelated, but surprisingly _extend reverses the order of the properties; _extend({}, {a:1, b:2}) yields the unexpected {b:2, a:1}.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/CTC/issues/62#issuecomment-273866920, or mute the thread https://github.com/notifications/unsubscribe-auth/AASgHlve2POgC9NwuTk2LmGBd5lc5p-gks5rT7OFgaJpZM4Ll2Fv .

ofrobots commented 7 years ago

I think the discussion on Object.assign is potentially interesting, but it seems that it has hijacked the original intent of this issue. I do think it would good to have a call w/ the V8 team. Any interest from the CTC on that?

As for logistics, since most of the V8 team is Central European time-zone, perhaps the standard CTC afternoon Europe / morning Pacific time slot could work, but on a different day?

Trott commented 7 years ago

I think the discussion on Object.assign is potentially interesting, but it seems that it has hijacked the original intent of this issue. I do think it would good to have a call w/ the V8 team. Any interest from the CTC on that?

o/

As for logistics, since most of the V8 team is Central European time-zone, perhaps the standard CTC afternoon Europe / morning Pacific time slot could work, but on a different day?

That would be the 16:00 UTC time which (depending on daylight savings status) is 8am PST, 9am PDT, 5pm CET, and 6pm CEST.

(For anyone who cares: Confusingly, the "S" in PST means "standard" so that's the time in winter as compared to "daylight savings" time in summer. The "S" in CEST is for "summer" so it's basically the opposite of the "S" in PST.)

hashseed commented 7 years ago

5pm CET and later generally work well for me.

mhdawson commented 7 years ago

I'd be interested.

Trott commented 7 years ago

I think all that's needed at this point is someone willing to do the logistical work: possibly a Doodle poll, definitely schedule a time, probably put together an agenda...that's probably about it. Decisions like Hangout vs. Uberconference etc. can be made later based on how many people are attending and who they are.

Fishrock123 commented 7 years ago

Is this still being planned? Should we open a Doodle to find out times?

Trott commented 7 years ago

Is this still being planned? Should we open a Doodle to find out times?

I think all that's needed at this point is someone willing to do the logistical work: possibly a Doodle poll, definitely schedule a time, probably put together an agenda...that's probably about it.

hashseed commented 7 years ago

I've set up a doodle here: http://doodle.com/poll/r2wz2vqqr9rk925i I added six days from Feb 13 to Feb 21, for 8am, 9am, or 10am. I'm proposing to use Google Hangouts, but am really open for any alternative. Regarding agenda, I'd like to talk about:

Future of GYP
Performance bottlenecks
V8 features that could be leveraged by Node.js, e.g. startup snapshots
How to prepare Node.js for API deprecations, e.g. debug context API
Whether to make this meeting a regular thing

joshgav commented 7 years ago

Is this open to all collaborators? I'd like to join. Thanks!

hashseed commented 7 years ago

Fewer people than I hoped actually signed up on the Doodle poll. If we want to go through with this nevertheless, I suggest doing the call on Feb 15, 10am PST.

I prepared a document on the GYP issue as to get everyone on the same page for discussion: https://docs.google.com/document/d/1gvHuesiuvLzD6X6ONddxXRxhODlOJlxgfoTNZTlKLGA/edit#

@natorion could you help me setting up a public Hangouts session?

joshgav commented 7 years ago

@hashseed

Fewer people than I hoped actually signed up on the Doodle poll.

From my experience around here, 13 is a pretty nice turnout 😄 .

cc @digitalinfinity @kunalspathak

hashseed commented 7 years ago

Well half are from the V8 team. Talking among the V8 team is not really the goal :)

evanlucas commented 7 years ago

Sorry, I somehow missed this. I added myself to the doodle, but if I'm too late, then that's ok.

natorion commented 7 years ago

I suppose it makes most sense to make the meeting public id public and post it here? Than everybody can join easily.

hashseed commented 7 years ago

Wednesday 10am PST still winning. Making the hangout public sgtm.

bnoordhuis commented 7 years ago

Future of GYP

I guess discussion of gyp is a good reason to join... I expect you will be dropping the gyp files at some point?

hashseed commented 7 years ago

See document I linked above.

trevnorris commented 7 years ago

Re: Object.assign() vs util._extend(). They can't really be compared. Object.assign() copies over Symbols along with named properties. So if _extend() included Object.getOwnPropertySymbols() the performance would drastically decrease. Let's look at an alternate implementation that behaves more correctly.

function _extend(target) {
  for (var arg_idx = 1; arg_idx < arguments.length; arg_idx++) {
    const source = arguments[arg_idx];
    const names = Object.getOwnPropertyNames(source);
    const syms = Object.getOwnPropertySymbols(source);
    for (var i = 0; i < names.length; i++) {
      target[names[i]] = source[names[i]];
    }
    for (var i = 0; i < syms.length; i++) {
      target[syms[i]] = source[syms[i]];
    }
  }
  return target;
}

This implementation is ~1.8x slower than Object.assign().

Threw my votes up on the Doodle.

natorion commented 7 years ago

Public Hangouts link: https://hangouts.google.com/hangouts/_/google.com/node-ctc-v8

fhinkel commented 7 years ago

Is Wednesday (2/15/2017) 10am PST the final time? @williamkapke can you add this to the Node calendar please?

hashseed commented 7 years ago

Yes. Wednesday (2/15/2017) 10am PST.

joshgav commented 7 years ago

Added to calendar: direct event link.

mhdawson commented 7 years ago

I know this is listed as CTC meeting with v8 team but I'm wondering if it would be ok if @jbajwa were to join in (possibly as an observer) as he is involved in the v8 ports to PPC and s390 in the core V8 repos and the Node.js repos. I'm asking becuse its not clear if the meeting is open to all or just CTC members.

hashseed commented 7 years ago

@mhdawson I don't really mind. Given that the GYP migration probably affects less popular platforms more than popular ones, I think it actually makes sense.

williamkapke commented 7 years ago

Will this be broadcast live? or just a google hangout?

I'll want to watch live or maybe after it is over if I miss it. Can we get this on the Node.js YouTube channel?... or is it going to some v8 channel?

natorion commented 7 years ago

I didn't set up any live streaming and I was not planning to record it. There is definitely the possibility for the latter though.

hashseed commented 7 years ago

Thanks for joining the meeting! It was very valuable to us! A few quick notes on what we discussed. Please correct me if I misunderstood anything.

GYP deprecation:

Native modules will keep using GYP in the foreseeable future.
The build system of Node core is decoupled from native modules though.
GN seems not to be a suitable option.
General sentiment leans towards the "legacy" and "transpiler" options. This puts the burden on whoever updates V8, and makes CI difficult.
Big interest in CI or Nightly builds. @targos was mentioned for currently working on this.
IBM has a solution for building for PPC with GN.
Discussion on this needs to be taken public.

Snapshot and code cache

Startup snapshot and code caching seem interesting, and worth keeping an eye on.
But probably not that important for Node.js, since there are still other low-hanging fruits to pick.

Debug context deprecation

Debug context is being deprecated at the end of the year.
Inspector protocol is the alternative, and can replicate most functionality of the debug context.
Mirror objects are going away with it as well.
Maintainers of native modules that use the debug context need to be warned.
LiveEdit is going to be reimplemented, but will remain usable through the inspector protocol.

Misc

Separate meeting regarding ES6 modules.
Repeat this format in four weeks. New Doodle to be set up.

@ofrobots could you make the recording of the meeting available? Thanks.

ofrobots commented 7 years ago

@ofrobots could you make the recording of the meeting available?

I am not sure if I can do that. Apparently we needed consent from everyone involved :sweat:.

rvagg commented 7 years ago

Inspector protocol is the alternative, and can replicate most functionality of the debug context.

Just out of interest. What are we losing here (noting "most")? Anything that might upset people using the debugger or using it from add-ons?

hashseed commented 7 years ago

I don't have a full list of missing features. We were able to migrate all of V8's debugger tests that use the debug context to the inspector protocol, with a wrapper to emulate the debug context API.

Notably missing is true mirror support. You can still do most of the things that mirrors offer, but you can't get access to the actual object you created the mirror for. That's because of the limitations of the inspector protocol. There are a few smaller features that have no inspector counter part, which are implemented as runtime functions (prefixed with %) in the wrapper.

hashseed commented 7 years ago

Since the last meeting was reasonably well-received, and we did agree on repeating it, here's the Doodle link for the next one: http://doodle.com/poll/h8td28wnzt3kdmmr

@bmeurer wants to talk about performance pain points for Node. Other agenda items are welcome too.

MylesBorins commented 7 years ago

@hashseed what timezone is the doodle in? All I see is either 6 am or 7 am...

targos commented 7 years ago

@MylesBorins 6/7 am would be CET. You can change the timezone, though. See on the right:

MylesBorins commented 7 years ago

Thanks!

On Wed, Mar 1, 2017 at 11:26 AM, Michaël Zasso notifications@github.com wrote:

@MylesBorins https://github.com/MylesBorins 6/7 am would be CET. You can change the timezone, though. See on the right: [image: image] https://cloud.githubusercontent.com/assets/2352663/23469493/0d5593ec-fea4-11e6-945e-b576951a92e8.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/CTC/issues/62#issuecomment-283390071, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecV3D2Wrd-OOJHhXIhu3S1TV3qYCtcks5rhZwegaJpZM4Ll2Fv .

joshgav commented 7 years ago

@hashseed thanks! It might be more discoverable and easier to follow the thread if you open a new issue for the new meeting.

mhdawson commented 7 years ago

I'd like to attend but the proposed times are the middle of the night for me (12/1 AM).

hashseed commented 7 years ago

Yes. These times are not ideal for everyone. I'd like to switch to other times on a follow up meeting if there is interest. But for this one let's keep the current choices.

hashseed commented 7 years ago

Just in case the other issue has low visibility: looks like March 30th, 6am CET, is the winner for the second meeting. Hangouts link.

nodejs / CTC

Call with V8 team #62