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

Set stackTraceLimit to 0 in fileSystemEntryExists #40444

Merged
merged 1 commit into from Sep 16, 2020

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Sep 9, 2020

[This is a cherry-pick of #40043]

The exception thrown by Node.js's fs.statSync function contains a stack
trace that can be expensive to compute. Since this exception isn't used
by fileSystemEntryExists, we can safely set Error.stackTraceLimit to 0
without a change in behavior.


A significant performance improvement was noticed with this change while
profiling tsserver on packages within a proprietary monorepo.
Specifically, my team saw high self time percentages for Node.js's
uvException and handleErrorFromBinding internal functions. These
functions are executed within fs.statSync when it fails to find the
given path.

fs.statSync: https://github.com/nodejs/node/blob/v14.4.0/lib/fs.js#L1030-L1037
handleErrorFromBinding: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/fs/utils.js#L254-L269
uvException: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/errors.js#L390-L443

Measurements

After adding Error.stackTraceLimit = 0, we saw:

  • For a large configured project with 12,565 files, tsserver reached the
    projectLoadingFinish event 48.78% faster. (~46.786s vs ~31.447s)
  • For a medium project with 7,064 files, tsserver was 25.75% faster.
    (~20.897s vs ~16.618s)
  • For a small project with 796 files, tsserver was only a negligible
    3.00% faster. (~3.545s vs ~3.442)

Measurements were taken on macOS 10.15.6, Node.js 14.4.0, and a recent
master commit of TypeScript (610fa28). The average of 3 runs before and
after this change were taken.

I would normally include .cpuprofile and isolate---*.log files, but
can't post them publicly in this case. If there's any other summaries
the TypeScript team would be curious about I can report them.

fs.statSync Misses

Within our monorepo, the fs.statSync misses were mostly searches for
alternative file extensions of module imports.

  • For node_modules imports, a lot of .ts/.tsx lookups failed until the
    .d.ts file was found.
  • Within projects with a lot of JSX files, .ts files were looked for
    before finding the .tsx version.
  • In the medium scale project mentioned above, a total of 38,515
    non-existent files were queried during createProgram.

The exception thrown by Node.js's fs.statSync function contains a stack
trace that can be expensive to compute. Since this exception isn't used
by fileSystemEntryExists, we can safely set Error.stackTraceLimit to 0
without a change in behavior.

---

A significant performance improvement was noticed with this change while
profiling tsserver on packages within a proprietary monorepo.
Specifically, my team saw high self time percentages for Node.js's
uvException and handleErrorFromBinding internal functions. These
functions are executed within fs.statSync when it fails to find the
given path.

https://user-images.githubusercontent.com/906558/90183227-220cb800-dd81-11ea-8d61-f41f89481f46.png

fs.statSync: https://github.com/nodejs/node/blob/v14.4.0/lib/fs.js#L1030-L1037
handleErrorFromBinding: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/fs/utils.js#L254-L269
uvException: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/errors.js#L390-L443

 ## Measurements

After adding Error.stackTraceLimit = 0, we saw:

- For a large configured project with 12,565 files, tsserver reached the
  projectLoadingFinish event 48.78% faster. (~46.786s vs ~31.447s)
- For a medium project with 7,064 files, tsserver was 25.75% faster.
  (~20.897s vs ~16.618s)
- For a small project with 796 files, tsserver was only a negligible
  3.00% faster. (~3.545s vs ~3.442)

Measurements were taken on macOS 10.15.6, Node.js 14.4.0, and a recent
master commit of TypeScript (610fa28). The average of 3 runs before and
after this change were taken.

I would normally include .cpuprofile and isolate-*-*-*.log files, but
can't post them publicly in this case. If there's any other summaries
the TypeScript team would be curious about I can report them.

 ## fs.statSync Misses

Within our monorepo, the fs.statSync misses were mostly searches for
alternative file extensions of module imports.

- For node_modules imports, a lot of .ts/.tsx lookups failed until the
  .d.ts file was found.
- Within projects with a lot of JSX files, .ts files were looked for
  before finding the .tsx version.
- In the medium scale project mentioned above, a total of 38,515
  non-existent files were queried during createProgram.
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 9, 2020
@amcasey
Copy link
Member Author

amcasey commented Sep 9, 2020

FYI @gluxon @DanielRosenwasser

@amcasey amcasey merged commit 23c77c4 into microsoft:release-4.0 Sep 16, 2020
@amcasey amcasey deleted the Cherry40043 branch September 16, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants