GeezerVison

TypeScript Bot (@typescript-bot) perf test this

Heya Jack Williams (@jack-williams), I've started to run the perf test suite on this PR at 391a3c8. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

So I know essentially zero about JS runtimes, but is one consideration here that certain runtimes only support inline-caches for one shape, while v8 supports up to four? I'm not sure that a possible regression introduced by this change would be observed testing with node.

I've been quite keen to learn more about the details around the perf design of the compiler, so any insights here would be a welcomed learning experience (for me).

I’m no expert either. Just tried to apply some things I learned from talk recordings, and digging through the heap profiler.

But mostly all engines agree that its best to initialize all properties at least in the same order to avoid shape changes. Which TS is notoriously bad at, since it just throws on additional properties onto Node and friends whenever it wants to…

I also do see a ton of stuff when running with node --trace-deopt related to wrong map (= shape change), which reminds me, I didn’t compare that output before/after my changes.

Also interesting to actually run the compiler with --no-opt, which basically disables the JIT, and compare the different phases. I did that once and noticed that while the Parse step had a speedup of ~6x with JIT, Check only had a speedup of ~2-3x, which means something is throwing off the optimizer. Or at least that is my guess.

All of these things are really badly documented though and its pretty much guesswork to figure out what all that means :-(

And regarding your note that this would only be observed when testing with node: I would argue running tsc on node is the >90% usecase, so IMO its a good idea to optimize for that :-)

Jack Williams (@jack-williams)
The results of the perf run you requested are in!

wow, those results are really encouraging, 6-8% wins on memory usage. total time seems to be ±1%, so not really conclusive.

What is the call to gc for? Will that mislead the results when comparing against master?

What is the call to gc?

That trigger a manual gc when run with node --expose-gc. The purpose is to get more stable numbers from the process.memoryUsage() below, otherwise you have high variance due to random gc. That’s why I was asking if the perf infra already uses that, or that it maybe should.
So the call has no effect when the testing infra is not running node with the --expose-gc flag.

I'd consider opening an issue with the questions and discussion points you list. It would probably get more visibility by those on the team, and also for other users of the tracker.

I'd consider opening an issue with the questions and discussion points you list. It would probably get more visibility by those on the team, and also for other users of the tracker.

Sure!
I opened #33432 for discussion about Node types, and #33433 for discussion about using gc().

Does anyone know what the "Lines" diagnostic is for? Is it the number of lines emitted?

My source files are about 30k lines total.

But when I build, "Lines" diagnostic says 5,266,173 and takes 300s (5 minutes)

Arpad Borsos (@Swatinem)

How many LOC do your source files have? How simple or complex are the types in your "super huge project"?

For reference, when building my composite project, my total stats are,

Source LOC  :     30,368
Files       :     74,285
Lines       :  5,266,173
Nodes       : 26,671,299
Identifiers :  7,810,628
Symbols     : 13,598,594
Types       :  5,053,944
Memory Used : 31,723,111K
Check time  :        218.41s
Emit time   :         69.38s
Total time  :        300.57s

I'm very interested in knowing if your changes will improve the memory usage without negatively affecting check times for me.

The total stats are huge because it's building 42 sub projects. Those stats are about a few weeks old. I am at 52 sub projects now =/

The perf tests don't show much of a change in check times; +3.64% at worst.
But I feel like those test "normal" usage and I tend to fall into "unusual" usage.

If we pack this, I can try running it on my project and report back with the results.

This might be a bit off-topic for this PR, but maybe something is going wrong with your dependencies. Take a look at tsc --listFiles. When you talk about sub-projects, make sure their dependencies have the correct version and are correctly deduped using something like yarn workspaces. Yarn has some nasty bugs around that and will fail to dedupe semver compatible packages and you might end up with multiple copies of it.
Also, some dependencies are really badly structured. Yesterday I found out that aws-sdk alone is pulling in 300kLOC, which alone causes ~250M memory usage when checking our project. See aws/aws-sdk-js#2846

Oh, and is your tsc srsly using 32G of ram? How is that even possible?

I'm not using any fancy tooling. Just regular tsc with a composite project set up. So, unless tsc itself is bugged, I should be good in that area.

I'll try out --listFiles when I get home.

It's using 32G total for all 42 sub projects. Not in one compilation =P it's basically creating 42 ts programs, I think.

All 42 sub projects use the same node_modules folder. So, not too many worries about dependencies duplicating all over the place. (I know it's still possible to have duplicates like using two different major versions of the same package indirectly)

Jack Williams (@jack-williams) can we have this packed? =x

Ryan Cavanaugh (@RyanCavanaugh) Ron Buckton (@rbuckton) I had done initial change for LS only as part of #9529 and as part of decreasing memory usage for Salsa.. We had then decided to only do this for LS at that time, don't remember the reason for that though.

Well this has conflicts now, should be trivial to fix. I would suggest to just land a very minimal version first, which only copy-pastes the Node constructor for all the already defined types.

I would be willing to further work on this to add shortcuts, remove unused props, maybe better organize the different types, etc. if someone wants to mentor me.

However, I’m a bit… well… disappointed that this has been sitting idle for almost 3 months now, so I don’t really want to waste my time if this isn’t going anywere… 😒

Sheetal Nandi (@sheetalkamat) Ron Buckton (@rbuckton) Ryan Cavanaugh (@RyanCavanaugh) any guidance?

Arpad Borsos (@Swatinem) I apologize for not looking into this sooner. The memory wins are appreciated, and the few cases of perf losses are either within the margin-of-error or targeting NodeJS 8.x which is now outside of Node's LTS maintenance schedule. If you are willing to resolve the conflicts, I can take a thorough review in the next few days.

Awesome! I’m gone now for an extended weekend, will rebase monday evening. :-)

The change in utilities.ts is a fairly trivial conflict to resolve, and the change in tsc.ts can just be dropped per my comment above. I can easily resolve the conflicts myself.

I can easily resolve the conflicts myself.

go ahead ;-)

Oh, while you are at it…

There is also an allocator for SourceFile pre-defined already. Make sure to also turn that one into its own constructor function. My experiments also have shown that that can further shrink Node.

Arpad Borsos (@Swatinem)

There is also an allocator for SourceFile pre-defined already.

I'll look into that one in a separate PR, we also have one for PrivateIdentifier now as well.

Hmm. The GitHub resolve conflicts web UI seems to have messed with the linebreaks in utilities.ts and tsc.ts. I'll have to fix that manually.

Arpad Borsos (@Swatinem): Thanks! I've merged these changes.