Path to adding a Tier 2 build with pointer compression enabled #3204

cjihrig · 2023-02-22T16:42:35Z

This is a follow up to nodejs/TSC#790. The project currently creates unofficial builds for Linux with pointer compression enabled. With pointer compression enabled, the main branch appears to pass the test suite (at least on macOS where I tested). From what I can tell, v19 and earlier are not in such good shape.

We are interested in getting pointer compression into the CI, and eventually hopefully get it to Tier 2 status (on Linux would be enough). cc: @nodejs/releasers.

I think the first step would be getting this into the CI so that we can have more confidence in the current unofficial build. Unfortunately, I don't have the slightest idea what needs to happen to add a new CI target - but I am willing to do the work if someone can provide guidance.

cc: @mhdawson @mcollina

The text was updated successfully, but these errors were encountered:

mhdawson · 2023-02-23T16:27:38Z

I think two questions we should get feedback on to start are:

@nodejs/tsc would it be ok to have pointer compression only enabled for Linux
@nodejs/releasers any concern in respect to adding a compressed pointer release to the shipping binaries?

RafaelGSS · 2023-02-23T16:37:39Z

As a Node.js TSC and Releaser, I'm +1. I don't see it impacting the release workflow directly. However, might make sense to produce a release candidate for LTS versions first (optional)

richardlau · 2023-02-23T16:42:13Z

@nodejs/releasers any concern in respect to adding a compressed pointer release to the shipping binaries?

Yes, I have concerns about confusion to end users in producing a second Linux download, particularly as I seem to recall concerns about ABI compatibility between regular builds and pointer compression enabled ones (think about ecosystem packages using prebuilds, for example).

cjihrig · 2023-02-23T18:16:24Z

@richardlau would you feel better about only adding pointer compression to the CI so that we can at least have more confidence in the existing unofficial build?

richardlau · 2023-02-23T18:27:58Z

@cjihrig my response was specifically to the quoted question. I'm neutral on adding another job to the CI.

mhdawson · 2023-03-02T20:53:11Z

Adding another job to the CI seems resonable to me as we already have machines of the required type available and my first guess is that we would not need additional machines to support the one extra job.

cjihrig · 2023-03-03T18:28:02Z

@mhdawson so what would be the next step to add the CI job? I'm happy to help, but don't know where/how to get started.

mhdawson · 2023-03-15T19:21:58Z

I don't see any objections to adding a CI job, so I'll set up a job Colin can edit/work on and then we can take it from there. It probably will mostly be a clone of some other job.

mhdawson · 2023-03-16T18:27:00Z

Added this job with @cjihrig - https://ci.nodejs.org/job/node-test-commit-linux-pointer-compression.

Just letting it run to confirm it passes and then will add to the daily main run.

mhdawson · 2023-03-16T18:27:57Z

To answer a question people might ask we did not add as a containerized build as I was not sure how to do that and only have it run in the nightly.

mhdawson · 2023-03-16T18:57:30Z

Configured the job so that @cjihrig can edit/modify the job in cases changes/fixes are needed.

mhdawson · 2023-03-17T20:01:55Z

Had to fixup the addition to the daily main job as it did not run today. Believe it should run next time.

cjihrig · 2023-03-31T15:23:55Z

@mhdawson thanks for the help getting the pointer compression job into the daily job. It looks like it has passed every time it has run for the past few weeks with the exception of one time when GitHub changed the key.

As a next step, does it make sense to add pointer compression to the CI for pull requests on main (from my testing, v19 and older do not work properly with pointer compression)?

mhdawson · 2023-04-04T21:58:51Z

The part about only running on v19 and later is one that may require something special. @richardlau I don't think the VersionSelectorSecript can help us on this one right?

richardlau · 2023-04-05T11:45:29Z

That is exactly what the VersionSelectorScript is intended for. However it works on labels (specifically on a build parameter named "nodes"), so you may need to rework the job and/or introduce new labels.

cjihrig · 2023-05-06T20:14:16Z

@mhdawson I don't really know what the VersionSelectorScript is, but is it possible to use it to try to move this forward? Happy to help however I can.

mhdawson · 2023-05-16T18:34:35Z

@cjihrig maybe we can get together and work on it together for an hour or so. Could you send an invitation with some candidate times?

richardlau · 2023-05-16T18:36:53Z

FWIW https://ci.nodejs.org/job/node-test-commit-linux-pointer-compression has been broken since last week.

cjihrig · 2023-05-16T22:02:25Z

@mhdawson will do.

has been broken since last week

All of the failures appear to be test/parallel/test-experimental-shared-value-conveyor.js (recently added by the V8 team for an experimental feature). Easy enough to skip when pointer compression is enabled, but FYI @syg.

jdmarshall · 2023-09-01T21:21:43Z

I'm not sure I understand why pointer compression would be a tier 2 build instead of the default.

How many people are using 4GB of memory in Node, per thread? They're the special case, not the rest of us who are using a handful of gigabytes per process.

kkocdko · 2023-11-02T07:34:33Z

The build after v21 seems broken. https://unofficial-builds.nodejs.org/download/release/v21.1.0/node-v21.1.0-linux-x64-pointer-compression.tar.xz

[kkocdko@klf bin]$ ldd --version
ldd (GNU libc) 2.37
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Written by Roland McGrath and Ulrich Drepper.
[kkocdko@klf bin]$ uname -a
Linux klf 6.4.14-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Sat Sep  2 16:36:06 UTC 2023 x86_64 GNU/Linux
[kkocdko@klf bin]$ ./node 
Segmentation fault (core dumped)
[kkocdko@klf bin]$ pwd
/home/kkocdko/misc/node-v21.1.0-linux-x64-pointer-compression/bin
[kkocdko@klf bin]$

The pointer-compression v21.0.0 error same as above.

The pointer-compression v20.9.0 works fine.

Official v21.1.0 works fine.

The V8 API requires entering an isolate before using it. We were often not doing this, which worked fine in practice. However when (multi-cage) pointer compression is enabled, the correct isolate needs to be active in order to decompress pointers correctly, otherwise it causes crashes. Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where they were missing. Tested by compiling with `--experimental-enable-pointer-compression` locally and running all tests. Refs: nodejs/build#3204 (comment) Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292

kvakil · 2023-11-12T02:21:56Z

The build after v21 seems broken. https://unofficial-builds.nodejs.org/download/release/v21.1.0/node-v21.1.0-linux-x64-pointer-compression.tar.xz

The build was broken by the V8 update. I put up a fix here: nodejs/node#50680

The V8 API requires entering an isolate before using it. We were often not doing this, which worked fine in practice. However when (multi-cage) pointer compression is enabled, the correct isolate needs to be active in order to decompress pointers correctly, otherwise it causes crashes. Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where they were missing. This also introduces RAIIIsolateWithoutEntering which is used in JSONParser to avoid otherwise exposing the Isolate::Scope outside of the class. Tested by compiling with `--experimental-enable-pointer-compression` locally and running all tests. Refs: nodejs/build#3204 (comment) Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292

The V8 API requires entering an isolate before using it. We were often not doing this, which worked fine in practice. However when (multi-cage) pointer compression is enabled, the correct isolate needs to be active in order to decompress pointers correctly, otherwise it causes crashes. Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where they were missing. This also introduces RAIIIsolateWithoutEntering which is used in JSONParser to avoid otherwise exposing the Isolate::Scope outside of the class. Tested by compiling with `--experimental-enable-pointer-compression` locally and running all tests. Refs: nodejs/build#3204 (comment) Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292 PR-URL: #50680 Refs: v8/v8@475c8cd Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

The V8 API requires entering an isolate before using it. We were often not doing this, which worked fine in practice. However when (multi-cage) pointer compression is enabled, the correct isolate needs to be active in order to decompress pointers correctly, otherwise it causes crashes. Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where they were missing. This also introduces RAIIIsolateWithoutEntering which is used in JSONParser to avoid otherwise exposing the Isolate::Scope outside of the class. Tested by compiling with `--experimental-enable-pointer-compression` locally and running all tests. Refs: nodejs/build#3204 (comment) Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292 PR-URL: nodejs#50680 Refs: v8/v8@475c8cd Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

The V8 API requires entering an isolate before using it. We were often not doing this, which worked fine in practice. However when (multi-cage) pointer compression is enabled, the correct isolate needs to be active in order to decompress pointers correctly, otherwise it causes crashes. Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where they were missing. This also introduces RAIIIsolateWithoutEntering which is used in JSONParser to avoid otherwise exposing the Isolate::Scope outside of the class. Tested by compiling with `--experimental-enable-pointer-compression` locally and running all tests. Refs: nodejs/build#3204 (comment) Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292 PR-URL: #50680 Refs: v8/v8@475c8cd Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

mikeduminy · 2024-04-22T16:19:40Z

I'd love to be able to use an official build that enables pointer compression on mac. Would that need a separate build?

mcollina · 2024-04-22T19:22:10Z

yes

ronag · 2024-08-05T19:12:57Z

Did we give up on pointer compression? I've been unable to try it out. Haven't been able to find a recent build nor have been able to figure out how to create a build myself. Hard to give feedback (for anyone) with the current state of things.

richardlau · 2024-08-05T19:23:41Z

Did we give up on pointer compression? I've been unable to try it out. Haven't been able to find a recent build nor have been able to figure out how to create a build myself. Hard to give feedback (for anyone) with the current state of things.

Nobody appears to be paying any attention to it.

https://ci.nodejs.org/job/node-test-commit-linux-pointer-compression/ has been broken for a long time (as far back as we keep build history, but it definitely was broken earlier in the year).
--experimental-enable-pointer-compression causes build failure node#51339 was last updated in January.
Unofficial builds haven't been happening for Node.js 22 (this is likely due to one of the other recipes failing and the build being aborted before the pointer compression build is attempted, but the point is that nobody has noticed/complained): https://unofficial-builds.nodejs.org/logs/202407191352-v22.5.1/

jasnell · 2024-08-05T19:42:01Z

No, we haven't given up and definitely still are paying attention. Cloudflare has engaged a contract with Igalia to look at a path that hopeful would enable, in part, Node.js to enable pointer compression by default in the future.

Specifically, one of the limiting factors with enabling pointer compression to this point has been the strict limit of a single 4gb pointer cage per-process. Limiting the entire process to a single 4gb limit would be too much of a breaking change.

To support the alternative configuration we have in Cloudflare Workers, for a different use case that touches on the same parts of v8, Igalia is working on a way to enable multiple pointer cages per process. We call the new API an IsolateGroup. Each IsolateGroup represents one cage, with N Isolates allowed per IsolateCage. A process would allow any number of IsolateGroups to be created. Each IsolateGroup still has the 4gb limit but the ability to create multiple cages per process is a lot more tractable than a single 4gb cage for the entire process.

While it would still definitely be a semver-major and potentially breaking change, I think this approach would make it far more likely that we can enable pointer compression by default in Node.js or at least provide a release distribution with pointer compression enabled.

ronag · 2024-08-05T19:46:18Z

Specifically, one of the limiting factors with enabling pointer compression to this point has been the strict limit of a single 4gb pointer cage per-process.

FWIW this is not a problem for us. I don't think it's a problem for a majority of users. IMHO two separate builds would be fine. Advanced users can use the pointer compression build.

mcollina · 2024-08-06T07:37:44Z

In the past, I thought we (Platformatic) could sponsor some of this infra work, but startup life got in the way, and this is not a priority anymore. We would definitely use a Node.js build with pointer compression enabled, but the main driver for investing to make it happen is gone.

jasnell · 2024-08-07T19:40:27Z

My hope is that the new IsolateGroups will at least make it less of a potentially breaking change. We have users, however, that depend on extremely large heap sizes that, even with compression, go well beyond the 4gb limit so we have to be careful. Having the IsolateGroup API there should also make it easier for us to transition to this mode in general.

SeanReece · 2024-08-08T02:26:43Z

FWIW this is not a problem for us. I don't think it's a problem for a majority of users.

At the risk of beating a dead horse here I have to agree. The max-old-space-size defaults to 2GB on 64bit systems and I’m willing to bet most node developers have not had a use case to increase this. Even having pointer compression be the default would increase the default heap limit 2x and advanced developers could just switch to the non-compressed build for use cases requiring more heap space.

What @jasnell is working on sounds very interesting and promising. But I hope we can get the pointer compression build back on track regardless.

jdmarshall · 2024-09-05T22:34:20Z

@jasnell What do you mean by well beyond 4GB? If you have lots of object graphs it's entirely possible for 6GB heap to comfortably fit into 4GB with compression enabled.

Also I haven't heard anybody petitioning for eliminating 64 bit pointers. The argument is which is the default case and which is the special case. Right now it's very much backward from reality.

jdmarshall · 2024-09-05T22:36:56Z

FWIW this is not a problem for us. I don't think it's a problem for a majority of users. IMHO two separate builds would be fine. Advanced users can use the pointer compression build.

@ronag I would take the opposite conclusion from this. If 4GB cage isn't a problem for most users, then the advanced users should be the ones looking at the uncompressed build, not the other way around.

jasnell · 2024-09-05T22:39:31Z

I've talked with folks that run in excess of 20gb heaps in certain extreme cases. I think Netflix has also seen this but I'd need to confirm again. I do think the excessively sized heaps are not the norm and experience has shown that the vast majority of cases can fit well within a 4gb compressed heap.

cjihrig mentioned this issue Feb 22, 2023

Looking for feedback: Pointer compression in Node.js nodejs/TSC#790

Closed

cjihrig mentioned this issue Jul 11, 2023

test: skip experimental test with pointer compression nodejs/node#48738

Merged

kvakil mentioned this issue Nov 12, 2023

src: fix pointer compression build nodejs/node#50680

Closed

cjihrig closed this as completed Nov 19, 2023

mcollina reopened this Nov 19, 2023

kkocdko mentioned this issue Dec 4, 2023

Port to NAPI microsoft/node-pty#644

Merged

5 tasks

joyeecheung mentioned this issue Jul 1, 2024

build: enable pointer compression nodejs/node#53666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path to adding a Tier 2 build with pointer compression enabled #3204

Path to adding a Tier 2 build with pointer compression enabled #3204

cjihrig commented Feb 22, 2023

mhdawson commented Feb 23, 2023 •

edited

Loading

RafaelGSS commented Feb 23, 2023

richardlau commented Feb 23, 2023

cjihrig commented Feb 23, 2023

richardlau commented Feb 23, 2023

mhdawson commented Mar 2, 2023

cjihrig commented Mar 3, 2023

mhdawson commented Mar 15, 2023

mhdawson commented Mar 16, 2023

mhdawson commented Mar 16, 2023

mhdawson commented Mar 16, 2023

mhdawson commented Mar 17, 2023

cjihrig commented Mar 31, 2023

mhdawson commented Apr 4, 2023

richardlau commented Apr 5, 2023

cjihrig commented May 6, 2023

mhdawson commented May 16, 2023

richardlau commented May 16, 2023

cjihrig commented May 16, 2023

jdmarshall commented Sep 1, 2023

kkocdko commented Nov 2, 2023 •

edited

Loading

kvakil commented Nov 12, 2023

mikeduminy commented Apr 22, 2024

mcollina commented Apr 22, 2024

ronag commented Aug 5, 2024 •

edited

Loading

richardlau commented Aug 5, 2024

jasnell commented Aug 5, 2024 •

edited

Loading

ronag commented Aug 5, 2024

mcollina commented Aug 6, 2024

jasnell commented Aug 7, 2024

SeanReece commented Aug 8, 2024

jdmarshall commented Sep 5, 2024

jdmarshall commented Sep 5, 2024

jasnell commented Sep 5, 2024

Path to adding a Tier 2 build with pointer compression enabled #3204

Path to adding a Tier 2 build with pointer compression enabled #3204

Comments

cjihrig commented Feb 22, 2023

mhdawson commented Feb 23, 2023 • edited Loading

RafaelGSS commented Feb 23, 2023

richardlau commented Feb 23, 2023

cjihrig commented Feb 23, 2023

richardlau commented Feb 23, 2023

mhdawson commented Mar 2, 2023

cjihrig commented Mar 3, 2023

mhdawson commented Mar 15, 2023

mhdawson commented Mar 16, 2023

mhdawson commented Mar 16, 2023

mhdawson commented Mar 16, 2023

mhdawson commented Mar 17, 2023

cjihrig commented Mar 31, 2023

mhdawson commented Apr 4, 2023

richardlau commented Apr 5, 2023

cjihrig commented May 6, 2023

mhdawson commented May 16, 2023

richardlau commented May 16, 2023

cjihrig commented May 16, 2023

jdmarshall commented Sep 1, 2023

kkocdko commented Nov 2, 2023 • edited Loading

kvakil commented Nov 12, 2023

mikeduminy commented Apr 22, 2024

mcollina commented Apr 22, 2024

ronag commented Aug 5, 2024 • edited Loading

richardlau commented Aug 5, 2024

jasnell commented Aug 5, 2024 • edited Loading

ronag commented Aug 5, 2024

mcollina commented Aug 6, 2024

jasnell commented Aug 7, 2024

SeanReece commented Aug 8, 2024

jdmarshall commented Sep 5, 2024

jdmarshall commented Sep 5, 2024

jasnell commented Sep 5, 2024

mhdawson commented Feb 23, 2023 •

edited

Loading

kkocdko commented Nov 2, 2023 •

edited

Loading

ronag commented Aug 5, 2024 •

edited

Loading

jasnell commented Aug 5, 2024 •

edited

Loading