build: re-enable V8 snapshots #1663

trevnorris · 2015-05-08T18:57:00Z

bnoordhuis · 2015-05-08T19:07:09Z

configure

@@ -581,7 +582,7 @@ def configure_arm(o):

  # Print warning when snapshot is enabled and building on armv6
  if is_arch_armv6() and options.with_snapshot:


options.unused_with_snapshot? Or maybe revert the rename.

On second thought: not options.with_snapshot is probably more accurate.

remind me, did you want me to disable snapshots by default for armv6?

Good question. Maybe enable it, remove the warning and see what the CI says?

silverwind · 2015-05-08T19:32:09Z

configure

-  # Print warning when snapshot is enabled and building on armv6
-  if is_arch_armv6() and options.with_snapshot:
-    warn('when building on ARMv6, don\'t use --with-snapshot')
-


The default is still to build without snapshot, right? I'm sorry, but the diff doesn't make it obvious for me.

The new commit is to test CI and see if building/testing on armv6 w/ snapshot enabled works. It's just a test commit for the CI.

@bnoordhuis I noticed on the CI job that there's no armv6 listed. Just armv7/8. So this config optin would be tested, right?

pi1-raspbian-wheezy at least is armv6.

@rvagg can we get those RPis renamed at some point to follow the naming schema arch-distro-version like the rest of the CI jobs, to avoid confusion.

@silverwind ah, thanks for pointing that out. missed that one.

trevnorris · 2015-05-08T19:45:55Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/656/

Thanks @indutny

trevnorris · 2015-05-08T21:05:49Z

@bnoordhuis Seems armv6 build now works w/ snapshots. I'm troubleshooting the win failures, but they're not from this PR. LGTY? (well, after the squash that is)

bnoordhuis · 2015-05-09T10:38:00Z

There are two other places in configure that use options.with_snapshot that don't seem to have been updated.

ChALkeR · 2015-05-09T23:20:46Z

This also sligthly decreases the memory usage, from ~0.8 MiB to ~0.3 MiB per an empty context.
The speedup is about 10-25 times.

piscisaureus · 2015-05-09T23:23:12Z

This makes building with snapshots the default again - is that intended?
The reason that snapshots are currently disabled by default is because of a security consideration.

indutny · 2015-05-09T23:24:13Z

@piscisaureus security consideration is no longer the case with a newer v8, this is why we revert that change.

ChALkeR · 2015-05-09T23:47:07Z

For the simple repeated (async) «var x = {}; vm.createContext(x);» test RSS with this patch does not reach 1.4 GiB now (as it did before), but stays around ~700 MiB (~800 MiB peak), heapUsed does not go above 480 MiB (was at least 880 MiB without this patch). That's still not perfect, but a bit better than it was.

jbergstroem · 2015-05-10T00:20:14Z

@indutny oh, I didn't know security was "fixed". Got more info on that? (bonus point for commit link :)

ChALkeR · 2015-05-10T05:41:58Z

@jbergstroem The discussion was at the issue this pr fixes: #1631 (comment)

trevnorris · 2015-05-10T19:31:26Z

@bnoordhuis don't see the two places you're talking about. I see:

  o['variables']['v8_use_snapshot'] = b(options.with_snapshot)

Which is correct, since snapshot will have been enabled. And:

  if target_arch != host_arch and options.with_snapshot:
    o['variables']['want_separate_host_toolset'] = 1
  else:
    o['variables']['want_separate_host_toolset'] = 0

Which I believe is still correct because want_separate_host_toolset should be 1 if snapshot is enabled.

bnoordhuis · 2015-05-11T10:12:28Z

Oh, I see what I missed now... --without-snapshot is options.with_snapshot with store_false.

It's kind of confusing because AFAIK that makes it the only option that works that way. Let me go over the PR one more time.

bnoordhuis · 2015-05-11T10:14:07Z

configure

-    action='store_true',
-    dest='unused_without_snapshot',
+    action='store_false',
+    dest='with_snapshot',


I think it would be clearer / more idiomatic if you named it 'without_snapshot' with action='store_false' (and you can drop the default=True in that case.)

I think you meant action='store_true'. Otherwise it'll always be false.

That is indeed what I meant. :-)

trevnorris · 2015-05-11T17:10:18Z

@bnoordhuis Switched the variable name, and does seem to make more sense that way. PTAL.

bnoordhuis · 2015-05-11T17:15:12Z

configure

    o['variables']['want_separate_host_toolset'] = 0
+  else:
+    o['variables']['want_separate_host_toolset'] = 1


I suspect that the logic is wrong here because it will set want_separate_host_toolset = 1 when target_arch == host_arch.

oh you're correct. I naively switched the statements. Has been fixed.

bnoordhuis · 2015-05-11T17:33:43Z

configure

    o['variables']['want_separate_host_toolset'] = 0
+  else:
+    o['variables']['want_separate_host_toolset'] = 1


This still doesn't look quite correct me. Host/target toolsetting should be enabled when cross-compiling with snapshots enabled so I think you could write this as:

cross_compiling = target_arch != host_arch want_snapshots = not options.without_snapshot o['variables']['want_separate_host_toolset'] = int(cross_compiling and want_snapshots)

The double negation reads kind of iffy but it is what it is.

Thanks. Had to make a white space change to fit at 80, but made the change.

Also re-enable use of snapshots for armv6. Fixes: nodejs#1631

bnoordhuis · 2015-05-11T18:52:41Z

LGTM if the CI is happy. I would explain in the commit log that enabling snapshots is no longer a security liability.

trevnorris · 2015-05-11T19:00:52Z

Thanks @bnoordhuis for all the reviews. Landed in 8736a8e.

Snapshots had been previously disabled because of a security vunerability. This has been fixed (ref: #1631 (comment)) Also, re-enable snapshots for ARMv6 builds. There were previous build issues that have been fixed. Fixes: #1631 PR-URL: #1663 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

trevnorris · 2015-05-11T19:01:57Z

Edit: Actually landed in 36cdc7c.

PR-URL: nodejs#1679 Notable Changes: * win,node-gyp: the delay-load hook for windows addons has now been correctly enabled by default, it had wrongly defaulted to off in the release version of 2.0.0 (Bert Belder) nodejs#1433 * os: tmpdir()'s trailing slash stripping has been refined to fix an issue when the temp directory is at '/'. Also considers which slash is used by the operating system. (cjihrig) nodejs#1673 * tls: default ciphers have been updated to use gcm and aes128 (Mike MacCana) nodejs#1660 * build: v8 snapshots have been re-enabled by default as suggested by the v8 team, since prior security issues have been resolved. This should give some perf improvements to both startup and vm context creation. (Trevor Norris) nodejs#1663 * src: fixed preload modules not working when other flags were used before --require (Yosuke Furukawa) nodejs#1694 * dgram: fixed send()'s callback not being asynchronous (Yosuke Furukawa) nodejs#1313 * readline: emitKeys now keeps buffering data until it has enough to parse. This fixes an issue with parsing split escapes. (Alex Kocharin) * cluster: works now properly emit 'disconnect' to cluser.worker (Oleg Elifantiev) nodejs#1386 events: uncaught errors now provide some context (Evan Lucas) nodejs#1654

Snapshots had been previously disabled because of a security vunerability. This has been fixed (ref: nodejs#1631 (comment)) Also, re-enable snapshots for ARMv6 builds. There were previous build issues that have been fixed. Fixes: nodejs#1631 PR-URL: nodejs#1663 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis reviewed May 8, 2015
View reviewed changes

brendanashworth added the build Issues and PRs related to build files or the CI. label May 8, 2015

silverwind reviewed May 8, 2015
View reviewed changes

trevnorris force-pushed the reenable-snapshots branch from faed1f3 to eda89af Compare May 10, 2015 19:27

bnoordhuis reviewed May 11, 2015
View reviewed changes

trevnorris force-pushed the reenable-snapshots branch from eda89af to a455238 Compare May 11, 2015 17:08

bnoordhuis reviewed May 11, 2015
View reviewed changes

trevnorris force-pushed the reenable-snapshots branch from a455238 to 6979842 Compare May 11, 2015 17:23

bnoordhuis reviewed May 11, 2015
View reviewed changes

build: re-enable V8 snapshots

61154d7

Also re-enable use of snapshots for armv6. Fixes: nodejs#1631

trevnorris force-pushed the reenable-snapshots branch from 6979842 to 61154d7 Compare May 11, 2015 18:19

trevnorris closed this May 11, 2015

Fishrock123 mentioned this pull request May 12, 2015

Release proposal: v2.0.2 #1679

Merged

Fishrock123 mentioned this pull request Jun 19, 2015

Reoncile v0.12 with new CI nodejs/node-v0.x-archive#25541

Closed

trevnorris deleted the reenable-snapshots branch March 28, 2016 22:21

snyk-bot mentioned this pull request Mar 25, 2021

[Snyk] Security upgrade snyk from 1.437.3 to 1.465.0 turkdevops/node#100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: re-enable V8 snapshots #1663

build: re-enable V8 snapshots #1663

trevnorris commented May 8, 2015

bnoordhuis May 8, 2015

bnoordhuis May 8, 2015

trevnorris May 8, 2015

bnoordhuis May 8, 2015

trevnorris May 8, 2015

silverwind May 8, 2015

trevnorris May 8, 2015

trevnorris May 8, 2015

silverwind May 8, 2015

trevnorris May 8, 2015

trevnorris commented May 8, 2015

trevnorris commented May 8, 2015

bnoordhuis commented May 9, 2015

ChALkeR commented May 9, 2015

piscisaureus commented May 9, 2015

indutny commented May 9, 2015

ChALkeR commented May 9, 2015

jbergstroem commented May 10, 2015

ChALkeR commented May 10, 2015

trevnorris commented May 10, 2015

bnoordhuis commented May 11, 2015

bnoordhuis May 11, 2015

trevnorris May 11, 2015

bnoordhuis May 11, 2015

trevnorris commented May 11, 2015

bnoordhuis May 11, 2015

trevnorris May 11, 2015

bnoordhuis May 11, 2015

trevnorris May 11, 2015

bnoordhuis commented May 11, 2015

trevnorris commented May 11, 2015

trevnorris commented May 11, 2015

		@@ -581,7 +582,7 @@ def configure_arm(o):

		# Print warning when snapshot is enabled and building on armv6
		if is_arch_armv6() and options.with_snapshot:

build: re-enable V8 snapshots #1663

build: re-enable V8 snapshots #1663

Conversation

trevnorris commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented May 8, 2015

trevnorris commented May 8, 2015

bnoordhuis commented May 9, 2015

ChALkeR commented May 9, 2015

piscisaureus commented May 9, 2015

indutny commented May 9, 2015

ChALkeR commented May 9, 2015

jbergstroem commented May 10, 2015

ChALkeR commented May 10, 2015

trevnorris commented May 10, 2015

bnoordhuis commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 11, 2015

trevnorris commented May 11, 2015

trevnorris commented May 11, 2015