Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Node.js v20+ support #15

Conversation

christianvuerings
Copy link
Collaborator

@christianvuerings christianvuerings commented Nov 7, 2024

Changes

Related PRs

Notes

We still see the following warning:

@ljharb
Copy link
Collaborator

ljharb commented Nov 7, 2024

I think it’d be ideal to do the CI conversion in a separate PR, at least, to ensure nothings breaking (also the test suite conversion, i think, after the breaking change lands?)

@christianvuerings christianvuerings force-pushed the christian--nodejs-v22-support branch from 6281d5d to e1b1e25 Compare November 7, 2024 15:00
src/memwatch.cc Outdated
@@ -160,6 +160,7 @@ NAN_GC_CALLBACK(memwatch::after_gc) {
s_stats.gcProcessWeakCallbacksTime += gcTime;
return;

case kGCTypeMinorMarkCompact:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes

../src/memwatch.cc:149:12: warning: enumeration value 'kGCTypeMinorMarkCompact' not handled in switch [-Wswitch]

Copy link

@jparise jparise Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also determine whether the case below should also consider kGCTypeMinorMarkCompact (i.e. does the "compaction" accounting behavior apply to both types):

if (type == kGCTypeMarkSweepCompact) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like v8 / chromium renamed kGCTypeMinorMarkCompact to kGCTypeMinorMarkSweep
chromium/chromium@257c88d#diff-11920fd94088cac09a3e53d31a3a3565501b9692fa7842fd98168536e279e002R174
https://chromium-review.googlesource.com/c/chromium/src/+/4669537

https://wingolog.org/archives/2023/12/07/the-last-5-years-of-v8s-garbage-collector

Originally called minor mark-compact or MinorMC in the commit logs, it was renamed to minor mark-sweep or MinorMS to indicate that it doesn’t actually compact. (V8’s mark-compact old-space doesn’t have to compact: V8 usually chooses to just mark in place. But we call it a mark-compact space because it has that capability.)

Also noticed that we already have this warning in the current version of our package so going to pull this change out for now to not cause any regressions: https://github.com/airbnb/node-memwatch/actions/runs/11726571196/job/32665528031#step:4:166

../src/memwatch.cc:149:11: warning: enumeration value ‘kGCTypeMinorMarkCompact’ not handled in switch [-Wswitch]
  149 |     switch(type) {
      |           ^

@christianvuerings christianvuerings force-pushed the christian--nodejs-v22-support branch from e1b1e25 to 4364d4c Compare November 7, 2024 15:55
@christianvuerings
Copy link
Collaborator Author

I think it’d be ideal to do the CI conversion in a separate PR, at least, to ensure nothings breaking (also the test suite conversion, i think, after the breaking change lands?)

@ljharb Updated this PR to only include the breaking change

See christianvuerings#4 (same branch as this one) which shows that the build + tests now pass in Node.js >=18 (all LTS versions)
image

@ljharb
Copy link
Collaborator

ljharb commented Nov 7, 2024

I'm not qualified to review C++ code, but tests are passing and it LGTM! Once another maintainer approves it I'm happy to land it.

@christianvuerings christianvuerings merged commit d903a65 into airbnb:master Nov 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'IdleNotificationDeadline' is deprecated: Use MemoryPressureNotification()
4 participants