V8 plan for Node.js LTS Carbon (A potential path to TurboFan + Ignition) #99
Comments
|
Some refs: |
|
Is this version of Node built from sources of |
|
Can |
|
@vsemozhetbyt that wasn't the core test suite. That was a test suite for one of my services at help.com. :] |
|
@chicoxyzzy the build above is indeed |
|
Personally I'd lean toward being prepared for 6.0, but I know a lot of companies that can be very strict about predictable performance. I imagine there'd be a few companies that'd avoid the 8.x line until after TF+I lands so they can do a proper performance audit and have a more clear idea what they can expect. |
|
I'm +1 on option 3. |
|
An example of a real life case. Run ESLint (v4.0.0-alpha.0) check on Node.js code base. Test script: 'use strict';
const execSync = require('child_process').execSync;
const command =
'eslint' +
' --rule "indent: 0, no-multi-spaces: 0, space-before-function-paren: 0"' +
' --rulesdir tools/eslint-rules/' +
' benchmark lib test';
console.time('eslint1');
try { execSync(command, { encoding: 'utf8' }); }
catch (err) { console.error(err.stdout); }
console.timeEnd('eslint1');
console.time('eslint2');
try { execSync(command, { encoding: 'utf8' }); }
catch (err) { console.error(err.stdout); }
console.timeEnd('eslint2');
console.time('eslint3');
try { execSync(command, { encoding: 'utf8' }); }
catch (err) { console.error(err.stdout); }
console.timeEnd('eslint3');Results: |
|
Would running CITGM with the new v8 produce any meaningful results? |
|
Another real world benchmark suite: https://github.com/eggjs/benchmark (benchmark of an enterprise Web framework egg.js), this includes real-world stuff like security & authentication and needs minimal external dependencies to run (no DB, wrk is required though, simply |
|
CITGM run (V8 5.9.213): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/711/ |
|
Results from egg's benchmark, the test build defintely outperforms the nightly 8.0.0-nightly20170411b8f416023d (v8 5.7.492.69)------- koa hello ------- 8.0.0-test201704119b43f9c487 (v8 5.9.0 candidate)------- koa hello ------- (Trying to write a script to output structured data with it..UPDATE: PR opened in eggjs/benchmark#6) |
|
Sitting here with @mcollina running a number of module benchmarks are we're not yet seeing a significant performance delta between 8-with-5.9 and master or 6.10.x. yet. We will be running a number of additional benchmarks over the coming couple of days to prove that out. If there is not going to be a significant difference between the two, then I'm seeing less of a need to delay the release for 5.9 but I'm also not seeing a significant technical reason not to. It's a bit up in the air, I think. We'll definitely need to run additional tests but if the performance profile is not significantly different, then it should actually be ok to ship 8.0.0 with 5.7 or 5.8 on time this month (option 1) and move up to 5.9 later in 8.x assuming the ABI compatibility is preserved without any issue. |
|
It may also be nice to see some typical front-end workflows comparison
|
|
ABI compatibility can be preserved if this patch is applied on top of the upgrade to V8 5.8. I'm still waiting for CITGM to verify that though :) |
|
@joyeecheung Awesome, thanks for doing the measurement. |
|
@fhinkel do you think we should investigate whether it makes sense to turn eslint and eggjs into benchmarks similar to AcmeAir? |
|
/cc @ofrobots for benchmark question. |
Just to make sure: you've put the tested Node binary to PATH, right? Because if not, you might actually have been testing your system Node both times and it might have only been the launcher script that you've run with Node 8. FWIW, a similar (but not equivalent) measurement: |
|
I think am +1 on 3. If delaying the release of Node 8 is not feasible, then at least option 2 for me. IMO we should be treating Node 8 the same way we treat any other "Current" branch (until it becomes LTS of course). I feel like going with option 1 would make using Node 8 a lot less appealing come October. We have had semver-minor upgrades of V8 in the last two major versions (v7, v6) and I think we should continue to try to track the latest version of V8 possible (wasn't that part of the reason for iojs in the first place?). We would have 5-6 months to get things up to par, but from my current testing (limited of course, but still real applications), real world performance has actually improved quite a lot with V8 5.8 and 5.9. |
|
@aqrln I've considered this code in the @IF EXIST "%~dp0\node.exe" (
"%~dp0\node.exe" "%~dp0\node_modules\eslint\bin\eslint.js" %*
) ELSE (
@SETLOCAL
@SET PATHEXT=%PATHEXT:;.JS;=;%
node "%~dp0\node_modules\eslint\bin\eslint.js" %*
)So I just put various Node.js versions in the global module folder. I've checked this approach with process manager to be sure I tested the right version. |
|
Here are some numbers for building a react/redux application with webpack (NODE_ENV=production, babel-loader, css-loader, url-loader, babili-webpack-plugin, code splitting, source maps...) |
|
@jasnell I the need for a delay has nothing to do with perf in 5.9... it was about being able to have api / abi compat up to 6.0... due to churn between 5.9 and 6.0 |
|
Yeah, I know there's more to it than just the performance and I still want to see what the consensus direction is. If we push off a month, then so be it :-) My goal, btw, is to get another test build later on today but it's going to depend on availability and reliability of the wifi connection from my hotel. |
|
I'm thinking that people using the LTS releases, generally favor stability over new features/performance improvements. I also see turbofan + ignition being on by default as a relatively larger risk to stability versus other V8 releases that we have pulled in during past releases. For these reasons I'd be leaning towards option 1. |
|
I'd like to chime in for a second to share my own small, non-scientific point of view -- apologies if this is out-of-order. As someone who writes libraries for Node.js, I would like to see the new pipeline regardless of current performance issues. A longstanding pain-point with Node.js has been V8's legacy compiler tree effectively disallowing certain language features in performance-critical areas, which to me has meant extra mental overhead and stress to make sure I'm playing nice with the JIT, even in area that aren't necessarily high stress. I would much prefer to work in an environment that doesn't cause me to worry (at least reflexively) if I'm using a technique that's going to arbitrarily slow down or otherwise negatively impact my code, where I can use features in the language that fit my mental model for what lexically makes sense in the code block I'm working with. Thanks for all your effort with the project! |
|
I'll be doing the benchmark of my real life app tomorrow or the day after. As we work only with docker I forked the docker-node and made dockerfiles for the v8.0.0-test201704119b43f9c487 I hope this help other to make their benchmarks Dockerfiles repo Images on dockerhub |
|
I would love to see a performance guide for collaborators regarding this migration - I think it will cause a lot of confusion with regards to what's slow and what's fast and we all know how hard measuring the sort of performance changes is very hard. |
|
Option 3 seems to be the 'sanest' from a maintenance point of view |
If we want to sync node releases with v8, then why don't we do that? Release 8.x right after 6.0 is released? Why did we choose twice a year at arbitrary times if V8 alignment is so important? I'm not going to rail forever against switching v8 in the midst of a major. I'm kindof curious about what kind of commotion an entirely new opt pipeline will cause. Maybe it will be no big deal, and we can be more free with this in the future. Maybe it will be a low point in node.js stability. I've no crystal ball. |
|
@sam-github afaik the release schedule we had was designed around what was preferable for enterprises and shops that usually work around LTS releases. @mhdawson can speak to this. If we want to wait until 6.0 is released we will be delaying until around August 1st... which is not tenable imho. |
|
The current release schedule was selected after many conversations with stakeholders throughout the ecosystem based on what makes the most sense for adopters. It was understood at the time that the V8 release schedule mismatch would cause some headaches and nothing has changed in that regard. We knew what we were signing up for and we knew that discussions like this would come up. At this point I am +1 on Option 3 but I am calling for an official @nodejs/ctc vote to settle the matter. @nodejs/ctc members, please weigh in. |
|
Abstain. I have an opinion, but I defer to those who do releases and/or otherwise will be most impacted by the decision here. I'll also note that if we don't have an option that gets a majority of non-abstaining CTC members to endorse it, we'll need a run-off vote to achieve that. Our governance rules require a majority of non-abstaining CTC members to get a decision (in voting situations). |
|
Tallying CTC votes.
|
|
I vote for Option 3 |
|
I vote for option 3
Please feel free to edit the original comment to tally votes
…On Apr 14, 2017 7:03 PM, "Evan Lucas" ***@***.***> wrote:
I vote for Option 3
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#99 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAecV2WLSz3e6563RgHdDr6TMTuneARcks5rv_tNgaJpZM4M6u7I>
.
|
Added a link at the bottom of the original comment to the comment with the vote tally. |
|
Ok, my votes for each specific proposal would be:
If one option has to be picked, I vote for option 3. |
|
My votes:
If one option has to be picked, I vote for option 3. |
|
Option 3 |
|
@Fishrock123 @indutny @misterdjules @mscdex @ofrobots @rvagg @shigeki @targos @thefourtheye @trevnorris ... can I please trouble each of you to get your votes in. We're less than 10 days from the originally scheduled release and I need to know how to proceed. |
|
Option 3 |
|
My vote is for option 3. |
|
My vote is for option 3 as well
…On Sun, Apr 16, 2017 at 1:20 PM Julien Gilli ***@***.***> wrote:
My vote is for option 3.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#99 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAE0qWgJIXTGDGa91ALvyLD9GWA2UL9Iks5rwnglgaJpZM4M6u7I>
.
|
|
With the additional votes, option 3 passes. The 8.0.0 release will be delayed until May 30th. |
|
I still think moving the scheduled, regular release data massively in favor of upstream deps yet again is a terrible idea. Option 2. |
|
Abstaining. |
|
I think that option3 is better. |
|
Removed |
|
Does it worth shipping with |
|
That would also be slightly less work: we would not have to retrofit 5.8 to be ABI compatible with 6.0. |
|
Yes, 5.9 will be a semver minor bump. We'll be able to easily move up in an 8.x minor release. |
|
v8 5.8.283.32 was released today in Chrome 58.0.3029.81 |
|
As the vote has been decided I am locking + closing this issue. Thank you all for participating |
and others
EDIT: CTC voting tally for this issue
Problem
V8 5.9 will be the first version with TurboFan + Ignition (TF+I) turned on by default. As parts of the Node.js codebase have been tuned to CrankShaft, there will be a non trivial amount of churn to adapt to the new pipeline. This also creates a security risk as CrankShaft and FullCodeGen are no longer maintained by the V8 team or tested by the Chrome security team.
If TF + I lands in Node.js 9.x backporting any changes to Node.js 8.x is going to prove extremely difficult and time consuming.
Below are three proposals of how we can approach this problem. To anyone in @nodejs/collaborators, and the community at large, we would love to hear your opinions on this. Further we really want to get some real world benchmarks, so if you have a way of testing builds to get non micro benchmarks please chime in and I'll get you binaries to work with.
Three Proposals:
What needs to be done?
Testing
Test build of 8.x including V8 5.9 (TF + I turned on)
Install with nvm
EDIT: CTC voting tally for this issue
The text was updated successfully, but these errors were encountered: