-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
fix(ext/node): fix vm memory usage and context initialization #23976
Conversation
e6b9229
to
22690cd
Compare
22690cd
to
848dabd
Compare
Remove usage of rusty_v8 annex slots for storing `ContextState` and `ModuleMap` and instead store them directly in a dedicated embedder field. Needed for denoland/deno#23976
This comment was marked as resolved.
This comment was marked as resolved.
// Uncomment for remote debugging | ||
// { | ||
// name: "Setup tmate session", | ||
// if: [ | ||
// "(matrix.job == 'test' || matrix.job == 'bench') &&", | ||
// "matrix.profile == 'release' && (matrix.use_sysroot ||", | ||
// "github.repository == 'denoland/deno')", | ||
// ].join("\n"), | ||
// uses: "mxschmitt/action-tmate@v3", | ||
// }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this left on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, ive used it quite a few times for segfaults in CIs
let mut external_references = Vec::with_capacity(7); | ||
let mut external_references = Vec::with_capacity(14); | ||
|
||
vm::GETTER_MAP_FN.with(|getter| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to remove this thread local hack - maybe the problem is now gone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #22441
Fixes #23913
Fixes #23852
Fixes #23917