build: re-enable V8 snapshots

[trevnorris]

Fixes: #1631

R=@bnoordhuis

[bnoordhuis]

options.unused_with_snapshot? Or maybe revert the rename.

[bnoordhuis]

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

[trevnorris]

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

[bnoordhuis]

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

[trevnorris]

will try.

[silverwind]

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

[trevnorris]

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.

[trevnorris]

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

[silverwind]

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.

[trevnorris]

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

[trevnorris]

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

Thanks @indutny

[trevnorris]

@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]

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

[ChALkeR]

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]

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]

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

[ChALkeR]

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]

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

[ChALkeR]

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

[trevnorris]

@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]

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]

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.)

[trevnorris]

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

[bnoordhuis]

That is indeed what I meant. :-)

[trevnorris]

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

[bnoordhuis]

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

[trevnorris]

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

[bnoordhuis]

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.

[trevnorris]

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

[bnoordhuis]

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

[trevnorris]

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

[trevnorris]

Edit: Actually landed in 36cdc7c.