process: improve cwd performance by BridgeAR · Pull Request #27224 · nodejs/node · GitHub

This caches the current working directory and only updates the variable
if process.chdir() is called.

This improves the process.chdir performance by factor 10 and path.resolve by
up to 66%. That again is used a lot in cjs modules.

I guess this is currently not possible since native modules could change the working directory as well, not only JS code. Or is that not a blocker / does not apply?
Do we have a way to detect if any modules are loaded or not? If so, we could have a fallback for the moment a native module is loaded.
And since process.chdir can not be called in workers: how does that work in combination with native modules that could change the working directory? Can we cache the working directory in case we use workers?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

I think the native addons situation does make this semver-major, at least, although it’s unlikely that many native addons change the current working directory.

I’m not sure what the most expensive part of caling cwd() is, but we could probably at least cache the JS string instance on the native side.

Can we cache the working directory in case we use workers?

With the approach in this PR, you’d have to notify workers when the cwd changes – for low overhead, you could e.g. use SharedArrayBuffer + Atomics to increase a counter for each call to process.chdir(), for example.

@joyeecheung I tried to add a SharedArrayBuffer to add a counter between the main thread and workers but I am not sure how to actually reach /node/lib/internal/bootstrap/node.js from /node/lib/internal/main/worker_thread.js and /node/lib/internal/worker.js from /node/lib/internal/bootstrap/node.js.

I circumvented this issue by only improving the performance for the main thread for now but it would actually be nice to improve workers as well. process.cwd() is commonly used during bootstrap.

@BridgeAR I think what you need is communication between worker.js and main/worker_thread.js? Or you can expose a wrapper in process/execution.js. Why do you need to go through bootstrap.js? A SharedArrayBuffer containing something like cwd is not supposed to be part of the snapshot.

@joyeecheung the cwd and chdir functions needs access to the shared buffer:

// Worker thread
let cachedCwd = realMethods.cwd();
let lastCounter = -1;
function cwd() {
  const currentCounter = Atomics.load(counter, 0);
  if (currentCounter === lastCounter)
    return cachedCwd;
  lastCounter = currentCounter;
  cachedCwd = realMethods.cwd();
  return cachedCwd;
}

// Main thread only.
// This buffer would have to be shared and passed through to each worker.
const counter = new Uint64Array(new SharedArrayBuffer(8));

function chdir(cwd) {  
  Atomics.add(counter, 0, 1);
  realMethods.chdir(cwd);
}

I could theoretically override the two functions in case the first worker is started and to use a shared buffer at that point but AFAIK we do not want to override the functions anymore at that point, right?

Local benchmark:

// Before
console.time(); for (var i = 0; i < 1e7; i++) process.cwd(); console.timeEnd()
// default: 5596.328ms

// After
console.time(); for (var i = 0; i < 1e7; i++) process.cwd(); console.timeEnd()
// default: 477.811ms

@BridgeAR You could delay the creation of process.cwd and process.chdir until pre-execution; or export the SAB in e.g. process/execution.js (and send it in worker.js whenever a new worker is spawned), then in main_thread_only.js and worker_thread_only.js, wrap the two variants of methods differently in wrapProcessMethods.

um, wait, in case of workers, I suppose the SAB has to be sent together with the LOAD_SCRIPT message, at that point you've already need to run multiple methods that need process.cwd() to do their job, so I guess the best you can do is to cache only after that (it should be fine to override process.cwd() during LOAD_SCRIPT).

I just updated the PR to also work with workers.

This caches the current working directory and only updates the variable if `process.chdir()` is called. PR-URL: nodejs#27224 Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Landed in 1d022e8 🎉

This caches the current working directory and only updates the variable if `process.chdir()` is called. PR-URL: nodejs#27224 Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This makes sure nodejs#27224 is possible to being backported in a semver-patch way.

This caches the current working directory and only updates the variable if `process.chdir()` is called. PR-URL: #27224 Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Backport-PR-URL: #27483

This makes sure #27224 is possible to being backported in a semver-patch way. PR-URL: #27483 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

This commit improves the lookups of `ignore` and `pending` configs by reducing the amount of iterations required over those config properties. tl;dr: - cached the config lookups for `pending` and `ignore` to reduce the amount of times we have to re-iterate the `pending` or `ignore` arrays provided by the config. - cache string lookups for normalized moduleId paths since This was done in three steps: First Step The normalized `moduleId` was cached. Running ember-template-lint on a big codebase (668 template files) took 12.94 seconds to run. I used this command in the app's codebase directory to profile: ``` node --prof ../ember-template-lint/bin/ember-template-lint.js . ``` Next, I generated a graph with the [flamebearer][flamebearer] npm package: ``` node --prof-process --preprocess -j isolate*.log | flamebearer ``` <img width="1188" alt="Screen_Shot_2020-08-13_at_9_25_14_AM" src="https://user-images.githubusercontent.com/1275021/90181853-f29c8200-dd65-11ea-8163-c2e3f94a5e00.png"> Profiling showed that ~19% of the CPU time was spent inside of the `statusForModule` function. Upon further investigation, I noticed that the [normalized moduleId based on the current working directory was being generated in a loop][3], creating a new string for potentially every item in the `pending` or `ignore` config. Our app has no `ignore` config, but does have a lot of `pending` entries. Moving the `fullPathModuleId` to a variable outside the loop took the runtime from 12.94 seconds to 11.04 seconds, about ~2 seconds of savings on its own! An important thing to keep in mind for the next two steps: `statusForModule` is called _at least once per rule_ to determine if the rule should be ignored or allowed to fail (or removed from the pending file if now passing). This means that any functions ran or strings created by `statusForModule` are, in the worst case scenario, calculated `numberOfFiles * pendingRuleConfigItems * ignoreConfigItems` times if the loop doesn't find the config for the file early, as it has to search the entire array of `pending` or `ignore` items for every file. Rather than rejoining the paths, they are instead generated once and stored in a cache object. This reduces the number of strings generated and time generating the same strings over and over. Caching the result of `process.cwd` (since it seems unlikely to change once the program is started) potentially has a nice performance side effect for users on Node 10 (supported until April 2021) as [process.cwd was not cached until Node 12.2](nodejs/node#27224). Second Step Lookups for `ignore` and `pending` were separated out into different caches. This was done because while [`statusForModule` will check for a function to run][1] instead of a string, in practice this is only true for `ignore` rules. The reason `ignore` rules need the function check is that they are always a function due to being converted from [strings to functions via the micromatch module][2]. Caching the lookup of `ignore` rules reduced the need to run the functions for every rule/file. Third Step Last, but not least: `pending` rules always seem to be a list of `object`s or `string`s (as generated by `--print-pending`) and having a list that potentially has a function in it doesn't seem likely given that the recommendation from ember-template-lint is to copy/paste the output of `--print-pending`. After re-profiling, the runtime went from 12.94 seconds to 9.04 seconds on my machine. The `get-config` file no longer showed up in the profile. <img width="1116" alt="Screen Shot 2020-08-13 at 12 07 40 PM" src="https://user-images.githubusercontent.com/1275021/90181902-0647e880-dd66-11ea-9cd4-cc2778198139.png"> [1]: https://github.com/ember-template-lint/ember-template-lint/blob/5937b63bed30380b4bce0f96b19061658a176840/lib/get-config.js#L376-L377 [2]: https://github.com/ember-template-lint/ember-template-lint/blob/5937b63bed30380b4bce0f96b19061658a176840/lib/get-config.js#L280 [3]: https://github.com/ember-template-lint/ember-template-lint/blob/5937b63bed30380b4bce0f96b19061658a176840/lib/get-config.js#L374 [flamebearer]: https://github.com/mapbox/flamebearer

BridgeAR requested review from addaleax and joyeecheung April 14, 2019 15:49

This comment has been minimized.

Sign in to view

BridgeAR added performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem. labels Apr 14, 2019

mihalskiy approved these changes Apr 14, 2019

View reviewed changes

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 14, 2019

jdalton reviewed Apr 14, 2019

View reviewed changes

Comment thread lib/internal/bootstrap/node.js Outdated

BridgeAR marked this pull request as ready for review April 14, 2019 23:56

This comment has been minimized.

Sign in to view

jdalton reviewed Apr 15, 2019

View reviewed changes

Comment thread lib/internal/bootstrap/node.js Outdated

joyeecheung reviewed Apr 15, 2019

View reviewed changes

Comment thread lib/internal/bootstrap/node.js Outdated

This comment has been minimized.

Sign in to view

jdalton approved these changes Apr 15, 2019

View reviewed changes

joyeecheung reviewed Apr 15, 2019

View reviewed changes

Comment thread lib/internal/bootstrap/node.js Outdated

This comment has been minimized.

Sign in to view

joyeecheung mentioned this pull request Apr 15, 2019

src: disallow calling env-dependent methods during bootstrap #27234

Closed

2 tasks

This comment has been minimized.

Sign in to view

bnoordhuis reviewed Apr 16, 2019

View reviewed changes

Comment thread lib/internal/worker.js Outdated

Comment thread lib/internal/worker.js Outdated

addaleax approved these changes Apr 16, 2019

View reviewed changes

Comment thread test/parallel/test-worker-relative-path-double-dot.js Outdated

Comment thread lib/internal/worker.js Outdated

Comment thread lib/internal/main/worker_thread.js Outdated

Comment thread lib/internal/main/worker_thread.js Outdated

targos approved these changes Apr 26, 2019

View reviewed changes

BridgeAR closed this Apr 26, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 29, 2019

process: compatibility patch to backport 1d022e8

bbc38b7

This makes sure nodejs#27224 is possible to being backported in a semver-patch way.

BridgeAR mentioned this pull request Apr 29, 2019

[v12.x] backport #27224 as semver-patch #27483

Closed

4 tasks

targos added the backported-to-v12.x label Apr 30, 2019

targos mentioned this pull request May 6, 2019

v12.2.0 proposal #27578

Merged

BridgeAR mentioned this pull request Dec 20, 2019

for ... of replacing for(;;) in library networking code? #31024

Closed

BridgeAR deleted the improve-cwd-performance branch January 20, 2020 12:02

fivetanley mentioned this pull request Aug 13, 2020

Cache pending/ignore config lookups to speed up linting of larger codebases ember-template-lint/ember-template-lint#1466

Merged

bnoordhuis mentioned this pull request Sep 17, 2023

Snapshotting is persisting the cwd #49675

Closed

giulioAZ mentioned this pull request Feb 3, 2026

fix: eliminate TOCTOU Race Condition (Hackerone report #3407207) #61664

Merged

process: improve cwd performance by BridgeAR · Pull Request #27224 · nodejs/node · GitHub

Uh oh!

Uh oh!

process: improve cwd performance - #27224

process: improve cwd performance#27224BridgeAR wants to merge 3 commits intonodejs:masternodejs/node:masterfrom BridgeAR:improve-cwd-performanceBridgeAR/node:improve-cwd-performanceCopy head branch name to clipboard

Conversation

BridgeAR commented Apr 14, 2019

Checklist

Uh oh!

This comment has been minimized.

addaleax commented Apr 14, 2019

Uh oh!

Uh oh!

BridgeAR commented Apr 14, 2019

Uh oh!

This comment has been minimized.

Uh oh!

joyeecheung commented Apr 15, 2019

Uh oh!

BridgeAR commented Apr 15, 2019

Uh oh!

Uh oh!

BridgeAR commented Apr 15, 2019

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

joyeecheung commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Apr 15, 2019

Uh oh!

This comment has been minimized.

BridgeAR commented Apr 15, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR commented Apr 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

process: improve cwd performance#27224
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:improve-cwd-performance

joyeecheung commented Apr 15, 2019 •
edited
Loading