-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Use char code array in createTextWriter #44037
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
Use char code array in createTextWriter #44037
Conversation
@amcasey, @DanielRosenwasser : one of you mind kicking a perf test? Running the output on the TypeScript compiler |
@typescript-bot perf test this |
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 142f9da. You can monitor the build here. Update: The results are in! |
Thanks, @RyanCavanaugh |
Hm, first glance at the benchmarks is not promising. I suspect whether or not this method is an improvement may depend heavily on how long the input strings to the writer are. Long comments that are preserved will result in redundant decode/encode work relative to the raw string concatenation. Might need a hybrid approach that bypasses the char code array for strings longer than some threshold. My local tests were run with both this change and the change in #44031, and showed an improvement with both over just the source map generator change. Wonder if the allocations in the source map generator are interfering, or if the benchmark cases just have very different characteristics than the compiler source code. |
@RyanCavanaugh Here they are:Comparison Report - master..44037
System
Hosts
Scenarios
Developer Information: |
Well, those results are fascinating. 5% improvement for TFS, 5% slower for Angular, about the same for Monaco and Compiler Unions. |
@rbuckton , @weswigham , do either of you know what the key differences are between the Angular benchmark and the TFS benchmark that might explain the differences in results? |
TFS benchmark is all legacy style namespaces (concatenated into a single output file), angular benchmark is all individual modules. |
@typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 7f98bef. You can monitor the build here. Update: The results are in! |
@weswigham So that means this might be higher overhead per file but scale better for large files? |
Maybe! I couldn't say for sure without microbenchmarking the code to find out, but that's definitely a plausible cause. |
@DanielRosenwasser Here they are:Comparison Report - master..44037
System
Hosts
Scenarios
Developer Information: |
Hm, less pronounced effects than before, but still the same split. |
This looks like a no go so far. I've explored altering |
Inquiring minds wanted to know, based on the performance savings in #44031 , if the same trick would work in
createTextWriter
. This PR aims to answer that question.Fixes #44036