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

Simply create a new file when calling updateFile #3154

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from

Conversation

DanielRosenwasser
Copy link
Member

Previously the code used TypeScript's updateFile, which performs an incremental parse; however, if we don't expect the contents to be similar enough, this wastes a lot of work. Reuse should still be done for untouched files from old versions of the program.

If I measured correctly (big if!), the before is something like this:

Duration: 358.75s, Total samples = 330527.55ms (92.13%)
Showing nodes accounting for 234432.24ms, 70.93% of 330527.55ms total
Dropped 12123 nodes (cum <= 1652.64ms)
      flat  flat%   sum%        cum   cum%
43480.84ms 13.15% 13.15% 43480.84ms 13.15%  (garbage collector)
22431.38ms  6.79% 19.94% 29808.65ms  9.02%  scan
14276.95ms  4.32% 24.26% 16485.47ms  4.99%  explainThemeScope
8470.93ms  2.56% 26.82% 17812.28ms  5.39%  doJSDocScan
7503.03ms  2.27% 29.09% 50711.16ms 15.34%  bind
6731.53ms  2.04% 31.13% 14063.59ms  4.25%  declareSymbol

And the after (with this PR) looks like this:

Duration: 136.13s, Total samples = 104413.98ms (76.70%)
Showing nodes accounting for 66596.18ms, 63.78% of 104413.98ms total
Dropped 10304 nodes (cum <= 522.07ms)
      flat  flat%   sum%        cum   cum%
12840.82ms 12.30% 12.30% 14840.34ms 14.21%  explainThemeScope
10529.17ms 10.08% 22.38% 10529.17ms 10.08%  (garbage collector)
3089.92ms  2.96% 25.34% 24318.65ms 23.29%  tokenizeWithTheme
2659.21ms  2.55% 27.89%  2659.21ms  2.55%  readFileUtf8
2524.28ms  2.42% 30.31% 39548.81ms 37.88%  visit
2190.34ms  2.10% 32.40%  2713.73ms  2.60%  stat

I did run a clean in between steps, so hopefully I did everything correctly - but this is a pretty nice drop!

Previously the code used TypeScript's `updateFile`, which performs an incremental parse;
however, if we don't expect the contents to be similar enough, this wastes a lot of work.
Reuse should still done for untouched files from old versions of the program.
@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 12, 2024

I don't see the same results in CI, and apparently it was right for me to call out that "big if!" - switching back to the v2 branch, running clean, and re-running, I see similar results. So some stuff must be getting cached. That said, this is quite a bit of work being done.

@jakebailey
Copy link
Member

Yeah, locally on rebuild, things are cached. If you're profiling, you kinda want to clean everything and rebuild from scratch to get the "true" time spent in CI.

Comment on lines -1 to -7
type System = import("typescript").System
type CompilerOptions = import("typescript").CompilerOptions
type CustomTransformers = import("typescript").CustomTransformers
type LanguageServiceHost = import("typescript").LanguageServiceHost
type CompilerHost = import("typescript").CompilerHost
type SourceFile = import("typescript").SourceFile
type TS = typeof import("typescript")
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 required to make the fix?

@andrewbranch
Copy link
Member

What are your performance results from? This is a public package that (I guess) we encourage people to use for all kinds of stuff. My guess is that most instances of updateFile in the wild would be small changes that benefit from an incremental parse. Do we have any evidence that our usage, which benefits from this change, is representative of community usage?

@jakebailey
Copy link
Member

jakebailey commented Jun 12, 2024

This was actually from pprof-it gatsby build in this repo, on the thread that does the TS stuff, IIRC.

@andrewbranch
Copy link
Member

Are we calling updateFile when we switch between compiling different twoslash snippets or something? Seems like we should just have/use a different API for that, and let updateFile be for, conceptually, updating a file.

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