configure: `v8_use_snapshot` should be `true` by indutny · Pull Request #3962 · nodejs/node

indutny · 2015-11-22T01:22:27Z

v8_use_snapshot should be either true or false, not 1 or 0.

R=@bnoordhuis

`v8_use_snapshot` should be either `true` or `false`, not 1 or 0.

ofrobots · 2015-11-22T01:26:15Z

LGTM.

indutny · 2015-11-22T01:42:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/806/

indutny · 2015-11-22T02:14:11Z

CI is green, landing.

indutny · 2015-11-22T02:15:22Z

Landed in 91ccbf0, thank you!

`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

ChALkeR · 2015-11-22T03:06:02Z

The old setting wasn't working? Shouldn't stuff like this be caught by tests?

indutny · 2015-11-22T03:13:11Z

There is no observable behavior change when using snapshot. It doesn't look like there is a public V8 API method to check this either.

bnoordhuis · 2015-11-24T12:32:50Z

This PR broke cross-compiling for ARM (and it was caught by the CI), see nodejs/build#263 (comment).

orangemocha · 2015-11-24T15:11:53Z

Should we temporarily revert the change to keep the CI working?

indutny · 2015-11-24T17:18:44Z

Looking

MylesBorins · 2015-12-01T00:36:53Z

Should we wait for nodejs/build#266 to resolve before landing this on LTS?

rvagg · 2015-12-01T02:15:06Z

@thealphanerd I say hold on it, let's wait for @joaocgreis to catch up on this snapshot + cross-compile thing and weigh in

`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Notable changes: * build: - Add support for Intel's VTune JIT profiling when compiled with --enable-vtune-profiling. For more information about VTune, see https://software.intel.com/en-us/node/544211. (Chunyang Dai) #3785. - Properly enable V8 snapshots by default. Due to a configuration error, snapshots have been kept off by default when the intention is for the feature to be enabled. (Fedor Indutny) #3962. * crypto: - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via crypto.createECDH(curve_name)) with private keys that are not dynamically generated via generateKeys(). The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the setPublicKey() method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) #3511. - Update root certificates from the current list stored maintained by Mozilla NSS. (Ben Noordhuis) #3951. - Multiple CA certificates can now be passed with the ca option to TLS methods as an array of strings or in a single new-line separated string. (Ben Noordhuis) #4099 * tools: Include a tick processor in core, exposed via the --prof-process command-line argument which can be used to process V8 profiling output files generated when using the --prof command-line argument. (Matt Loring) #4021. PR-URL: #4181

joaocgreis · 2015-12-09T16:13:34Z

@thealphanerd nodejs/build#266 has been resolved and the fix has landed in v4.x-staging, this should work for LTS in CI now.

rvagg · 2015-12-10T02:16:45Z

Sucks that we can't test if it's been enabled or not.

My rough testing (node -pe 'process.versions' > /dev/null over 500 iterations) shows the following startup speed increases:

OS X:
- v5.2.0 starts 41.3% faster than v5.1.0
- v5.2.0 starts 44.5% faster than v4.2.3
Linux 64-bit:
- v5.2.0 starts 47.18% faster than v5.1.0
- v5.2.0 starts 42.36% faster than v4.2.3

In real terms that's roughly being able to run those 500 starts over ~35s compared to ~50s. This will be good to get into the next v4.x.

For context: snapshots are disabled in all v0.10 and v0.12 builds (they were disabled somewhere in v0.6.x) for security reasons, we thought we re-enabled them @ io.js v2.0.2 / 36cdc7c but as @indutny discovered, a 1 wasn't actually enabling them.

rvagg · 2015-12-10T02:23:26Z

@nodejs/benchmarking it'd be great if startup speed could be included in your efforts somewhere, it's such a key performance metric to many users (fast-restart-on-error is considered best practice by many, as opposed to the JVM cross-your-fingers-on-error approach). Having measurements to push for improvements and protect against regressions would be really helpful.

fundon · 2015-12-10T16:13:02Z

Really?
Nice!

MylesBorins · 2015-12-15T00:39:50Z

@rvagg is this good to land in LTS now?

rvagg · 2015-12-15T00:43:43Z

I think yes, absolutely @thealphanerd

rvagg · 2015-12-15T00:44:11Z

best run CI against v4.x-staging afterward though @thealphanerd, just in case there's something hanging over from the arm build problems

MylesBorins · 2015-12-15T00:49:13Z

sgtm

`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

MylesBorins · 2015-12-15T01:04:16Z

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

MylesBorins · 2015-12-15T01:16:51Z

@rvagg looks like arm is compiling fine to me

rvagg · 2015-12-15T05:49:43Z

lgtm for v4, fips failures all look unrelated

`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Notable changes: * build: - Add support for Intel's VTune JIT profiling when compiled with --enable-vtune-profiling. For more information about VTune, see https://software.intel.com/en-us/node/544211. (Chunyang Dai) nodejs#3785. - Properly enable V8 snapshots by default. Due to a configuration error, snapshots have been kept off by default when the intention is for the feature to be enabled. (Fedor Indutny) nodejs#3962. * crypto: - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via crypto.createECDH(curve_name)) with private keys that are not dynamically generated via generateKeys(). The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the setPublicKey() method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) nodejs#3511. - Update root certificates from the current list stored maintained by Mozilla NSS. (Ben Noordhuis) nodejs#3951. - Multiple CA certificates can now be passed with the ca option to TLS methods as an array of strings or in a single new-line separated string. (Ben Noordhuis) nodejs#4099 * tools: Include a tick processor in core, exposed via the --prof-process command-line argument which can be used to process V8 profiling output files generated when using the --prof command-line argument. (Matt Loring) nodejs#4021. PR-URL: nodejs#4181

configure: v8_use_snapshot should be true
37b5ff3
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0.

indutny added lts-watch-v4.x build Issues and PRs related to build files or the CI. labels Nov 22, 2015

indutny mentioned this pull request Nov 22, 2015
VM: Extreme memory growth #3113
Closed

indutny closed this Nov 22, 2015

indutny deleted the build/use-v8-snapshot branch November 22, 2015 02:15

indutny added a commit that referenced this pull request Nov 22, 2015
configure: v8_use_snapshot should be true
91ccbf0
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

bnoordhuis mentioned this pull request Nov 24, 2015
problems with node-test-pull-request nodejs/build#263
Closed

orangemocha mentioned this pull request Nov 24, 2015
ARM cross-compile does not use v8 snapshots nodejs/build#266
Closed

Fishrock123 mentioned this pull request Nov 26, 2015
deps: upgrade npm to 3.5.0 #4032
Closed

joaocgreis mentioned this pull request Dec 2, 2015
configure: respect CC_host in host arch detection #4117
Merged

indutny added a commit that referenced this pull request Dec 5, 2015
configure: v8_use_snapshot should be true
f29c5d6
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

This was referenced Dec 8, 2015
Release proposal: v5.2.0 #4180
Closed
Release proposal: v5.2.0 (Stable) #4181
Merged

indutny added a commit that referenced this pull request Dec 15, 2015
configure: v8_use_snapshot should be true
a29d6d3
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

MylesBorins added lts-landed-on-v4.x and removed lts-watch-v4.x labels Dec 15, 2015

rvagg mentioned this pull request Dec 16, 2015
Track startup speed nodejs/benchmarking#22
Open

MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015

jasnell mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
Closed

indutny added a commit that referenced this pull request Dec 17, 2015
configure: v8_use_snapshot should be true
629d5d1
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

mathiask88 mentioned this pull request Dec 22, 2015
Hint: Startup time improvement microsoft/vscode#1593
Closed

indutny added a commit that referenced this pull request Dec 23, 2015
configure: v8_use_snapshot should be true
49b3c3b
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0. PR-URL: #3962 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Uh oh!

Uh oh!

Conversation

indutny commented Nov 22, 2015

Uh oh!

ofrobots commented Nov 22, 2015

Uh oh!

indutny commented Nov 22, 2015

Uh oh!

indutny commented Nov 22, 2015

Uh oh!

indutny commented Nov 22, 2015

Uh oh!

ChALkeR commented Nov 22, 2015

Uh oh!

indutny commented Nov 22, 2015

Uh oh!

bnoordhuis commented Nov 24, 2015

Uh oh!

orangemocha commented Nov 24, 2015

Uh oh!

indutny commented Nov 24, 2015

Uh oh!

MylesBorins commented Dec 1, 2015

Uh oh!

rvagg commented Dec 1, 2015

Uh oh!

joaocgreis commented Dec 9, 2015

Uh oh!

rvagg commented Dec 10, 2015

Uh oh!

rvagg commented Dec 10, 2015

Uh oh!

fundon commented Dec 10, 2015

Uh oh!

MylesBorins commented Dec 15, 2015

Uh oh!

rvagg commented Dec 15, 2015

Uh oh!

rvagg commented Dec 15, 2015

Uh oh!

MylesBorins commented Dec 15, 2015

Uh oh!

MylesBorins commented Dec 15, 2015

Uh oh!

MylesBorins commented Dec 15, 2015

Uh oh!

rvagg commented Dec 15, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants