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 4ac9eb8 Compare 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 Show resolved Hide 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 7ec8e6d Compare 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 resolved Hide 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add flag to enable pointer compression #30463

build: add flag to enable pointer compression #30463

mcollina commented Nov 13, 2019

mcollina commented Nov 13, 2019

addaleax commented Nov 13, 2019

addaleax left a comment

codebytere commented Nov 13, 2019

mcollina commented Nov 13, 2019

mcollina commented Nov 13, 2019

addaleax commented Nov 13, 2019

jasnell commented Nov 14, 2019

hashseed commented Nov 14, 2019

mcollina commented Nov 19, 2019

rvagg left a comment

nodejs-github-bot commented Nov 22, 2019

mcollina commented Nov 22, 2019 •

edited

Loading

nodejs-github-bot commented Nov 22, 2019

nodejs-github-bot commented Nov 30, 2019

nodejs-github-bot commented Nov 30, 2019

jasnell commented Dec 1, 2019

nodejs-github-bot commented Dec 1, 2019

tniessen left a comment

nodejs-github-bot commented Dec 8, 2019

nodejs-github-bot commented Dec 9, 2019

mcollina commented Dec 9, 2019

nodejs-github-bot commented Dec 10, 2019

BridgeAR commented Dec 10, 2019

nodejs-github-bot commented Dec 10, 2019 •

edited by Trott

Loading

Trott commented Dec 12, 2019

mmarchini commented Feb 21, 2020

tuananh commented Feb 23, 2020

mhdawson commented Feb 24, 2020

richardlau commented Jun 1, 2020

mmarchini commented Jun 2, 2020

jdmarshall commented Feb 12, 2023

build: add flag to enable pointer compression #30463

build: add flag to enable pointer compression #30463

Conversation

mcollina commented Nov 13, 2019

Checklist

mcollina commented Nov 13, 2019

addaleax commented Nov 13, 2019

addaleax left a comment

Choose a reason for hiding this comment

codebytere commented Nov 13, 2019

mcollina commented Nov 13, 2019

mcollina commented Nov 13, 2019

addaleax commented Nov 13, 2019

jasnell commented Nov 14, 2019

hashseed commented Nov 14, 2019

mcollina commented Nov 19, 2019

rvagg left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 22, 2019

mcollina commented Nov 22, 2019 • edited Loading

nodejs-github-bot commented Nov 22, 2019

nodejs-github-bot commented Nov 30, 2019

nodejs-github-bot commented Nov 30, 2019

jasnell commented Dec 1, 2019

nodejs-github-bot commented Dec 1, 2019

tniessen left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 8, 2019

nodejs-github-bot commented Dec 9, 2019

mcollina commented Dec 9, 2019

nodejs-github-bot commented Dec 10, 2019

BridgeAR commented Dec 10, 2019

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

Trott commented Dec 12, 2019

mmarchini commented Feb 21, 2020

tuananh commented Feb 23, 2020

mhdawson commented Feb 24, 2020

richardlau commented Jun 1, 2020

mmarchini commented Jun 2, 2020

jdmarshall commented Feb 12, 2023

mcollina commented Nov 22, 2019 •

edited

Loading

nodejs-github-bot commented Dec 10, 2019 •

edited by Trott

Loading