Skip to content
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

configure: v8_use_snapshot should be true #3962

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Nov 22, 2015

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.
@indutny indutny added lts-watch-v4.x build Issues and PRs related to build files or the CI. labels Nov 22, 2015
@ofrobots
Copy link
Contributor

LGTM.

@indutny
Copy link
Member Author

indutny commented Nov 22, 2015

@indutny
Copy link
Member Author

indutny commented Nov 22, 2015

CI is green, landing.

@indutny
Copy link
Member Author

indutny commented Nov 22, 2015

Landed in 91ccbf0, thank you!

@indutny indutny closed this Nov 22, 2015
@indutny 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
`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
Copy link
Member

ChALkeR commented Nov 22, 2015

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

@indutny
Copy link
Member Author

indutny commented Nov 22, 2015

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
Copy link
Member

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

@orangemocha
Copy link
Contributor

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

@indutny
Copy link
Member Author

indutny commented Nov 24, 2015

Looking

@MylesBorins
Copy link
Contributor

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

@rvagg
Copy link
Member

rvagg commented Dec 1, 2015

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

indutny added a commit that referenced this pull request Dec 5, 2015
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0.

PR-URL: #3962
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
rvagg added a commit that referenced this pull request Dec 9, 2015
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
rvagg added a commit that referenced this pull request Dec 9, 2015
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
Copy link
Member

@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
Copy link
Member

rvagg commented Dec 10, 2015

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
Copy link
Member

rvagg commented Dec 10, 2015

@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
Copy link
Contributor

fundon commented Dec 10, 2015

Really?
Nice!

@MylesBorins
Copy link
Contributor

@rvagg is this good to land in LTS now?

@rvagg
Copy link
Member

rvagg commented Dec 15, 2015

I think yes, absolutely @thealphanerd

@rvagg
Copy link
Member

rvagg commented Dec 15, 2015

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

@MylesBorins
Copy link
Contributor

sgtm

indutny added a commit that referenced this pull request Dec 15, 2015
`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
Copy link
Contributor

@MylesBorins
Copy link
Contributor

@rvagg looks like arm is compiling fine to me

@rvagg
Copy link
Member

rvagg commented Dec 15, 2015

lgtm for v4, fips failures all look unrelated

@jasnell jasnell mentioned this pull request Dec 17, 2015
indutny added a commit that referenced this pull request Dec 17, 2015
`v8_use_snapshot` should be either `true` or `false`, not 1 or 0.

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

PR-URL: #3962
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants