build: add flag to enable pointer compression#30463

mcollina · 2019-11-13T11:31:19Z

This PR adds support for building node with pointer compression enabled. From some preliminary tests, enabling pointer compression shrinks memory usage by 40%.

See: #26756

Note that building with this flag makes some of our addon tests fail:

=== release test === Path: addons/async-hooks-promise/test Command: out/Release/node /home/matteo/repositories/node/test/addons/async-hooks-promise/test.js --- CRASHED (Signal: 11) --- === release test === Path: addons/08_passing_wrapped_objects_around/test Command: out/Release/node /home/matteo/repositories/node/test/addons/08_passing_wrapped_objects_around/test.js --- CRASHED (Signal: 11) --- === release test === Path: addons/new-target/test Command: out/Release/node /home/matteo/repositories/node/test/addons/new-target/test.js --- CRASHED (Signal: 11) --- === release test === Path: addons/make-callback/test Command: out/Release/node /home/matteo/repositories/node/test/addons/make-callback/test.js --- CRASHED (Signal: 11) --- === release test === Path: addons/06_wrapping_c_objects/test Command: out/Release/node /home/matteo/repositories/node/test/addons/06_wrapping_c_objects/test.js --- CRASHED (Signal: 11) --- === release test === Path: addons/07_factory_of_wrapped_objects/test Command: out/Release/node /home/matteo/repositories/node/test/addons/07_factory_of_wrapped_objects/test.js --- CRASHED (Signal: 11) --- === release test-perf-hooks-timerify === Path: addons/non-node-context/test-perf-hooks-timerify Command: out/Release/node /home/matteo/repositories/node/test/addons/non-node-context/test-perf-hooks-timerify.js --- CRASHED (Signal: 11) --- [02:41|% 100|+ 2807|- 7]: Done Makefile:296: recipe for target 'jstest' failed make[1]: *** [jstest] Error 1 Makefile:316: recipe for target 'test' failed make: *** [test] Error 2

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

mcollina · 2019-11-13T11:31:43Z

cc @bmeurer @hashseed @mhdawson @codebytere

addaleax · 2019-11-13T11:43:38Z

Note that building with this flag makes some of our addon tests fail:

Yeah, that’s expected. Enabling pointer compression changes the Node.js ABI, which is IIRC the reason why we haven’t done this so far.

addaleax

This needs a good story for our addon ABI

codebytere · 2019-11-13T11:58:36Z

Electron feels similarly - the memory savings are positive especially considering they're per-isolate but we're concerned about ABI compat of native modules

mcollina · 2019-11-13T12:44:20Z

@gabrielschulhof can you confirm n-api can handle this case?

mcollina · 2019-11-13T12:46:39Z

This needs a good story for our addon ABI

@addaleax my goal for this is to make it easier for folks to experiment with address compression. If you are ok, I'll add the option as "(experimental)" in the ./configure help.

addaleax · 2019-11-13T14:19:37Z

@gabrielschulhof can you confirm n-api can handle this case?

N-API is unaffected.

This needs a good story for our addon ABI
@addaleax my goal for this is to make it easier for folks to experiment with address compression. If you are ok, I'll add the option as "(experimental)" in the ./configure help.

That would be good, but I’d prefer to explicitly call out that this mode does not currently support addons (at all)?

jasnell · 2019-11-14T12:51:23Z

Definite +1 on marking this experimental and documenting the impact on native addons. Assuming those changes, this LGTM

hashseed · 2019-11-14T12:59:38Z

That would be good, but I’d prefer to explicitly call out that this mode does not currently support addons (at all)?

Yeah iirc the plan was to consider having a build flag in V8 that ensures ABI stability regardless of whether pointer compression is enabled or not, by sacrificing some performance, since inline-implemented APIs would need to be replaced by non-inline versions.

mcollina · 2019-11-19T17:43:16Z

Updated @jasnell @addaleax

rvagg

would be good to enable so we can at least start playing with it

node.gyp

nodejs-github-bot · 2019-11-22T19:18:43Z

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

mcollina · 2019-11-22T19:19:15Z

I've removed the config from common.gypi. Please review.

cc @targos

nodejs-github-bot · 2019-11-22T20:38:24Z

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

nodejs-github-bot · 2019-11-30T01:06:21Z

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

nodejs-github-bot · 2019-11-30T17:56:11Z

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

configure.py

jasnell · 2019-12-01T19:21:31Z

I'm wondering whether there should be a warning printed at runtime when the node.js process starts.

nodejs-github-bot · 2019-12-01T22:12:07Z

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

tniessen

LGTM as an experimental build feature.

nodejs-github-bot · 2019-12-08T10:51:12Z

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

nodejs-github-bot · 2019-12-09T08:25:36Z

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

mcollina · 2019-12-09T12:26:59Z

@nodejs/build @nodejs/testing I would need some help here, as tests that seems unrelated are failing, however this should not change the default config.

nodejs-github-bot · 2019-12-10T00:35:33Z

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

BridgeAR · 2019-12-10T00:36:01Z

@mcollina we currently rarely have green builds. There are lots of infrastructure failures and flaky tests. That's what seemed to have happened here as well.

nodejs-github-bot · 2019-12-10T09:55:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27548/ ✔️

Trott · 2019-12-12T00:34:53Z

Landed in 086c7b4

The --experimental-enable-pointer-compression is experimental as it breaks ABI compatibility. PR-URL: nodejs#30463 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

The --experimental-enable-pointer-compression is experimental as it breaks ABI compatibility. PR-URL: #30463 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

mmarchini · 2020-02-21T03:49:40Z

@targos any concerns on backporting this to v12.x? I was testing it today, and if we add the following lines (pulled from dda658c) to common.gypi, it works:

diff --git a/common.gypi b/common.gypi index 8c9076b735..856c6aa6ff 100644 --- a/common.gypi+++ b/common.gypi@@ -339,6 +339,12 @@ }], ], }], + ['v8_enable_pointer_compression == 1',{+ 'defines': ['V8_COMPRESS_POINTERS'],+ }],+ ['v8_enable_pointer_compression == 1 or v8_enable_31bit_smis_on_64bit_arch == 1',{+ 'defines': ['V8_31BIT_SMIS_ON_64BIT_ARCH'],+ }], ['OS == "win"',{'defines': [ 'WIN32',

The changes are non-intrusive, since it only impacts builds using --experimental-enable-pointer-compression. I can send a PR to backport it, unless there are other concerns.

tuananh · 2020-02-23T16:02:05Z

where can i download a built version with --experimental-enable-pointer-compression enabled?

mhdawson · 2020-02-24T17:36:10Z

@tuananh I believe you have to build it yourself as it is not in any of the shipping binaries.

richardlau · 2020-06-01T22:26:44Z

@targos any concerns on backporting this to v12.x? I was testing it today, and if we add the following lines (pulled from dda658c) to common.gypi, it works:
diff --git a/common.gypi b/common.gypi index 8c9076b735..856c6aa6ff 100644 --- a/common.gypi+++ b/common.gypi@@ -339,6 +339,12 @@ }], ], }], + ['v8_enable_pointer_compression == 1',{+ 'defines': ['V8_COMPRESS_POINTERS'],+ }],+ ['v8_enable_pointer_compression == 1 or v8_enable_31bit_smis_on_64bit_arch == 1',{+ 'defines': ['V8_31BIT_SMIS_ON_64BIT_ARCH'],+ }], ['OS == "win"',{'defines': [ 'WIN32',
The changes are non-intrusive, since it only impacts builds using --experimental-enable-pointer-compression. I can send a PR to backport it, unless there are other concerns.

If this does get backported it will need to land with #33688 to fix building addons.

mmarchini · 2020-06-02T04:37:30Z

Probably not worth it. We have pointer compressions builds for v14 now, and the pointer compression implementation on v12 has bugs as well as performance issues.

`common.gypi` is used by `node-gyp` to compile addons. Default values must be provided for variables that may not exist on older versions of Node.js so that older versions of Node.js can be used to compile addons for later versions of Node.js. Add default values for `v8_enable_pointer_compression` and `v8_enable_31bit_smis_on_64bit_arch`. PR-URL: nodejs#33688 Refs: nodejs#30463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Signed-off-by: Richard Lau <[email protected]>

`common.gypi` is used by `node-gyp` to compile addons. Default values must be provided for variables that may not exist on older versions of Node.js so that older versions of Node.js can be used to compile addons for later versions of Node.js. Add default values for `v8_enable_pointer_compression` and `v8_enable_31bit_smis_on_64bit_arch`. PR-URL: #33688 Refs: #30463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Signed-off-by: Richard Lau <[email protected]>

jdmarshall · 2023-02-12T17:33:32Z

Why wouldn’t this be useful now that we’re on node 18?

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 13, 2019

addaleax requested changes Nov 13, 2019
View reviewed changes

mcollina force-pushed the enable-pointer-compression branch from d10e3d6 to 4ac9eb8Compare November 19, 2019 17:43

addaleax approved these changes Nov 19, 2019
View reviewed changes

BridgeAR approved these changes Nov 19, 2019
View reviewed changes

rvagg approved these changes Nov 19, 2019
View reviewed changes

richardlau mentioned this pull request Nov 20, 2019
doc: add minutes for meeting 19 Nov 2019 nodejs/build#2055
Merged

targos reviewed Nov 20, 2019
View reviewed changes

node.gyp Outdated Show resolvedHide resolved

cjihrig approved these changes Nov 20, 2019
View reviewed changes

devnexen approved these changes Nov 20, 2019
View reviewed changes

mcollina force-pushed the enable-pointer-compression branch from 4ac9eb8 to 7ec8e6dCompare November 22, 2019 19:17

targos approved these changes Nov 22, 2019
View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019

jasnell reviewed Dec 1, 2019
View reviewed changes

configure.py Outdated Show resolvedHide resolved

addaleax mentioned this pull request Dec 6, 2019
Looking for feedback: Pointer compression in Node.js nodejs/TSC#790
Closed

tniessen approved these changes Dec 7, 2019
View reviewed changes

Trott approved these changes Dec 10, 2019
View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 10, 2019

Trott closed this Dec 12, 2019

MylesBorins mentioned this pull request Dec 13, 2019
v13.4.0 proposal #30937
Merged

targos added the dont-land-on-v12.x label Jan 14, 2020

This was referenced Jun 1, 2020
build: fix compiling addons with older versions of Node.js #33688
Merged
node-gyp uses some current process information even when a target version is supplied nodejs/node-gyp#1004
Closed

richardlau mentioned this pull request Jul 14, 2020
v12.18.3 proposal #34343
Merged

Uh oh!

build: add flag to enable pointer compression#30463

build: add flag to enable pointer compression #30463

Uh oh!

Conversation

mcollina commented Nov 13, 2019

Checklist

Uh oh!

mcollina commented Nov 13, 2019

Uh oh!

addaleax commented Nov 13, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere commented Nov 13, 2019

Uh oh!

mcollina commented Nov 13, 2019

Uh oh!

mcollina commented Nov 13, 2019

Uh oh!

addaleax commented Nov 13, 2019

Uh oh!

jasnell commented Nov 14, 2019

Uh oh!

hashseed commented Nov 14, 2019

Uh oh!

mcollina commented Nov 19, 2019

Uh oh!

rvagg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 22, 2019

Uh oh!

mcollina commented Nov 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 22, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

Uh oh!

jasnell commented Dec 1, 2019

Uh oh!

nodejs-github-bot commented Dec 1, 2019

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 8, 2019

Uh oh!

nodejs-github-bot commented Dec 9, 2019

Uh oh!

mcollina commented Dec 9, 2019

Uh oh!

nodejs-github-bot commented Dec 10, 2019

Uh oh!

BridgeAR commented Dec 10, 2019

Uh oh!

nodejs-github-bot commented Dec 10, 2019• edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 12, 2019

Uh oh!

mmarchini commented Feb 21, 2020

Uh oh!

tuananh commented Feb 23, 2020

Uh oh!

mhdawson commented Feb 24, 2020

Uh oh!

richardlau commented Jun 1, 2020

Uh oh!

mmarchini commented Jun 2, 2020

Uh oh!

jdmarshall commented Feb 12, 2023

Uh oh!

Reviewers

mcollina commented Nov 22, 2019•
edited
Loading

nodejs-github-bot commented Dec 10, 2019•
edited by Trott
Loading