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

fix(ext/node): fix vm memory usage and context initialization #23976

Merged
merged 21 commits into from
Jun 12, 2024
Merged
12 changes: 11 additions & 1 deletion .github/workflows/ci.generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { stringify } from "jsr:@std/yaml@^0.221/stringify";
// Bump this number when you want to purge the cache.
// Note: the tools/release/01_bump_crate_versions.ts script will update this version
// automatically via regex, so ensure that this line maintains this format.
const cacheVersion = 95;
const cacheVersion = 96;

const ubuntuX86Runner = "ubuntu-22.04";
const ubuntuX86XlRunner = "ubuntu-22.04-xl";
Expand Down Expand Up @@ -688,6 +688,16 @@ const ci = {
].join("\n"),
env: { CARGO_PROFILE_DEV_DEBUG: 0 },
},
// 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",
// },
Comment on lines +691 to +700
Copy link
Member

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?

Copy link
Member Author

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

{
name: "Build release",
if: [
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,8 @@ jobs:
path: |-
~/.cargo/registry/index
~/.cargo/registry/cache
key: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
restore-keys: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
key: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
restore-keys: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
if: '!(matrix.skip)'
- name: Restore cache build output (PR)
uses: actions/cache/restore@v4
Expand All @@ -380,7 +380,7 @@ jobs:
!./target/*/*.zip
!./target/*/*.tar.gz
key: never_saved
restore-keys: '95-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-'
restore-keys: '96-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-'
- name: Apply and update mtime cache
if: '!(matrix.skip) && (!startsWith(github.ref, ''refs/tags/''))'
uses: ./.github/mtime_cache
Expand Down Expand Up @@ -669,7 +669,7 @@ jobs:
!./target/*/gn_out
!./target/*/*.zip
!./target/*/*.tar.gz
key: '95-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
key: '96-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
publish-canary:
name: publish canary
runs-on: ubuntu-22.04
Expand Down
2 changes: 1 addition & 1 deletion ext/node/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ fn current_mode(scope: &mut v8::HandleScope) -> Mode {
};
let mut buffer = [MaybeUninit::uninit(); 2048];
let str = v8_string.to_rust_cow_lossy(scope, &mut buffer);
if node_resolver.in_npm_package_with_cache(str) {
if str.starts_with("node:") || node_resolver.in_npm_package_with_cache(str) {
Mode::Node
} else {
Mode::Deno
Expand Down
63 changes: 62 additions & 1 deletion ext/node/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ mod resolution;
pub use ops::ipc::ChildPipeFd;
pub use ops::ipc::IpcJsonStreamResource;
pub use ops::v8::VM_CONTEXT_INDEX;
use ops::vm;
pub use ops::vm::create_v8_context;
pub use ops::vm::init_global_template;
pub use ops::vm::ContextInitMode;
pub use package_json::PackageJson;
pub use path::PathClean;
pub use polyfill::is_builtin_node_module;
Expand Down Expand Up @@ -641,7 +645,64 @@ deno_core::extension!(deno_node,
global_template_middleware = global_template_middleware,
global_object_middleware = global_object_middleware,
customizer = |ext: &mut deno_core::Extension| {
let mut external_references = Vec::with_capacity(7);
let mut external_references = Vec::with_capacity(14);

vm::GETTER_MAP_FN.with(|getter| {
Copy link
Member

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

Copy link
Member

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

external_references.push(ExternalReference {
named_getter: *getter,
});
});
vm::SETTER_MAP_FN.with(|setter| {
external_references.push(ExternalReference {
named_setter: *setter,
});
});
vm::DELETER_MAP_FN.with(|deleter| {
external_references.push(ExternalReference {
named_getter: *deleter,
},);
});
vm::ENUMERATOR_MAP_FN.with(|enumerator| {
external_references.push(ExternalReference {
enumerator: *enumerator,
});
});
vm::DEFINER_MAP_FN.with(|definer| {
external_references.push(ExternalReference {
named_definer: *definer,
});
});
vm::DESCRIPTOR_MAP_FN.with(|descriptor| {
external_references.push(ExternalReference {
named_getter: *descriptor,
});
});

vm::INDEXED_GETTER_MAP_FN.with(|getter| {
external_references.push(ExternalReference {
indexed_getter: *getter,
});
});
vm::INDEXED_SETTER_MAP_FN.with(|setter| {
external_references.push(ExternalReference {
indexed_setter: *setter,
});
});
vm::INDEXED_DELETER_MAP_FN.with(|deleter| {
external_references.push(ExternalReference {
indexed_getter: *deleter,
});
});
vm::INDEXED_DEFINER_MAP_FN.with(|definer| {
external_references.push(ExternalReference {
indexed_definer: *definer,
});
});
vm::INDEXED_DESCRIPTOR_MAP_FN.with(|descriptor| {
external_references.push(ExternalReference {
indexed_getter: *descriptor,
});
});

global::GETTER_MAP_FN.with(|getter| {
external_references.push(ExternalReference {
Expand Down
17 changes: 17 additions & 0 deletions ext/node/ops/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,23 @@ use deno_core::v8;

use super::vm_internal as i;

pub use i::create_v8_context;
pub use i::init_global_template;
pub use i::ContextInitMode;

pub use i::DEFINER_MAP_FN;
pub use i::DELETER_MAP_FN;
pub use i::DESCRIPTOR_MAP_FN;
pub use i::ENUMERATOR_MAP_FN;
pub use i::GETTER_MAP_FN;
pub use i::SETTER_MAP_FN;

pub use i::INDEXED_DEFINER_MAP_FN;
pub use i::INDEXED_DELETER_MAP_FN;
pub use i::INDEXED_DESCRIPTOR_MAP_FN;
pub use i::INDEXED_GETTER_MAP_FN;
pub use i::INDEXED_SETTER_MAP_FN;

pub struct Script {
inner: i::ContextifyScript,
}
Expand Down
Loading