FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

configure: v8_use_snapshot should be true - #3962

Closed
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:build/use-v8-snapshot
ConversationCommits (1)ChecksFiles changed
Closed

configure: v8_use_snapshot should be true#3962
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:build/use-v8-snapshot

Conversation

indutny commented Nov 22, 2015

Copy link
Copy Markdown
Member

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

R=@bnoordhuis

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
Closed

ofrobots commented Nov 22, 2015

Copy link
Copy Markdown
Contributor

LGTM.

indutny commented Nov 22, 2015

Copy link
Copy Markdown
Member Author

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

indutny commented Nov 22, 2015

Copy link
Copy Markdown
Member Author

CI is green, landing.

indutny commented Nov 22, 2015

Copy link
Copy Markdown
Member Author

Landed in 91ccbf0, thank you!

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>

ChALkeR commented Nov 22, 2015

Copy link
Copy Markdown
Member

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

indutny commented Nov 22, 2015

Copy link
Copy Markdown
Member Author

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 mentioned this pull request Nov 24, 2015
Closed

bnoordhuis commented Nov 24, 2015

Copy link
Copy Markdown
Member

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

orangemocha commented Nov 24, 2015

Copy link
Copy Markdown
Contributor

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

indutny commented Nov 24, 2015

Copy link
Copy Markdown
Member Author

Looking

orangemocha mentioned this pull request Nov 24, 2015
Closed
Fishrock123 mentioned this pull request Nov 26, 2015
Closed

MylesBorins commented Dec 1, 2015

Copy link
Copy Markdown
Contributor

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

rvagg commented Dec 1, 2015

Copy link
Copy Markdown
Member

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

joaocgreis mentioned this pull request Dec 2, 2015
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
Closed
Merged
rvagg added a commit that referenced this pull request Dec 9, 2015
2015-12-09, Version 5.2.0 (Stable)
65b86c0 
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
2015-12-09, Version 5.2.0 (Stable)
6ca5ea3 
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 commented Dec 9, 2015

Copy link
Copy Markdown
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 commented Dec 10, 2015

Copy link
Copy Markdown
Member

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 commented Dec 10, 2015

Copy link
Copy Markdown
Member

@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 commented Dec 10, 2015

Copy link
Copy Markdown
Contributor

Really?
Nice!

MylesBorins commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

@rvagg is this good to land in LTS now?

rvagg commented Dec 15, 2015

Copy link
Copy Markdown
Member

I think yes, absolutely @thealphanerd

rvagg commented Dec 15, 2015

Copy link
Copy Markdown
Member

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

MylesBorins commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

sgtm

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 commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

@rvagg looks like arm is compiling fine to me

rvagg commented Dec 15, 2015

Copy link
Copy Markdown
Member

lgtm for v4, fips failures all look unrelated

rvagg mentioned this pull request Dec 16, 2015
Open
jasnell mentioned this pull request Dec 17, 2015
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
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
2015-12-09, Version 5.2.0 (Stable)
4c5e32c 
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL