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): have process global available in Node context #27562

Conversation

magurotuna
Copy link
Member

This commit makes process global always available in Node context.

process global was previously available explicitly in deno_node, but then got removed in #25291 and made globally available regardless of whether it's in Deno or Node context, so this commit does not have any effect on Deno CLI. However, for users who want to use deno_node ext only, it makes sense to have process available to simulate the Node environment solely.

@magurotuna magurotuna marked this pull request as ready for review January 6, 2025 05:15
@magurotuna magurotuna requested a review from bartlomieju January 6, 2025 05:15
@nathanwhit
Copy link
Member

nathanwhit commented Jan 7, 2025

IIRC this will might have a negative perf impact on usage of the process global in a node context. Not sure how large the impact would be, but we should probably bench it

@magurotuna
Copy link
Member Author

Thanks for pointing that out Nathan. When I discussed this with Bartek, he also mentioned that potentially bad impact, but we decided to go this way assuming that the impact is little. That said, it makes a lot of sense to do a benchmark to make sure.
@bartlomieju do you think we should land this patch after we bench it?

@magurotuna
Copy link
Member Author

magurotuna commented Jan 7, 2025

I made a very simple benchmark where it adds up the values 10k times returned from envLength function in npm:@magurotuna/env-length that just does Object.keys(process.env).length.

https://github.com/magurotuna/process_global_bench

Comparing results with/without process global redundantly set in deno_node, there's almost no difference.

process is NOT enabled in deno_node (b7fb5a5):

❯ hyperfine --warmup 3 'deno_20240107_main_b7fb5a5 run -A main.ts'
Benchmark 1: deno_20240107_main_b7fb5a5 run -A main.ts
  Time (mean ± σ):     477.8 ms ±   4.2 ms    [User: 468.1 ms, System: 8.4 ms]
  Range (min … max):   471.2 ms … 484.7 ms    10 runs

process is enabled in deno_node (this branch):

❯ hyperfine --warmup 3 'deno_20240107_process_in_node run -A main.ts'
Benchmark 1: deno_20240107_process_in_node run -A main.ts
  Time (mean ± σ):     477.8 ms ±  11.9 ms    [User: 470.5 ms, System: 8.3 ms]
  Range (min … max):   462.8 ms … 501.7 ms    10 runs

@magurotuna magurotuna merged commit 1661ddd into denoland:main Jan 8, 2025
17 checks passed
@magurotuna magurotuna deleted the deno-node-0-122-0-patch-add-process-to-node-global-based-on-55d345b branch January 8, 2025 04:15
dsherret pushed a commit to dsherret/deno that referenced this pull request Jan 8, 2025
…and#27562)

This commit makes `process` global always available in Node context.

`process` global was previously available explicitly in `deno_node`, but then
got removed in denoland#25291 and made globally available regardless of whether it's in
Deno or Node context, so this commit does not have any effect on Deno CLI.
However, for users who want to use `deno_node` ext only, it makes sense to have
`process` available to simulate the Node environment individually.

This change may bring some negative performance impact. To measure how large the
impact would be, a very simple benchmark was performed whose results can be
found at https://github.com/magurotuna/process_global_bench.
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.

3 participants