Typescript: Too many deopts

[amcasey]

Tracking issue for https://twitter.com/jasnell/status/1326219768064454656

@jasnell is seeing too many deopts while building with TS + Babel + Webpack.

[amcasey]

@rbuckton or @weswigham might know if we're already working on the specific issues you're seeing, @jasnell.

[weswigham]

I just essentially see "up to 2gb of memory is used and compilation is at least 14s", which doesn't seem abnormal for a large build (and don't see anything in the referenced thread about deopts). There anything in particular you think we may have been working on?

[jasnell]

Thanks @amcasey :-) ... yeah, just to give an idea, in this one build I'm seeing 130 Severity 1 deopts, 72 Severity 2, and 215 Severity 3. Inline cache transitions are 8772, 2582, and 1896 for Sev 1, 2 and 3 respectively. This is running in Node.js 12.x but I see similar numbers of Node.js 14.x. TypeScript version is 4.0.5. I'm using deoptigate (npm i -g deoptigate) to profile the optimizations.

The most significant issues I'm seeing are "wrong map" deopts that are causing functions to go polymorphic or megamorphic. I'm not sure there's going to be a lot that can be done about those. I am, however, seeing a few out of bounds reads and "wrong call target" deopts with quite a few of the callback functions.

[jasnell]

@weswigham ... don't worry about the memory chart there at the top of thread, that's really to be expected in this kind of app, it's just fun to look at lol. The deopts are where I'm seeing the most concern.

[amcasey]

Sorry, @weswigham, I should have provided more context. The graphs look normal to me too, but low-hanging deopts are always interesting.

[amcasey]

@jasnell Any chance you can tell us more about where you're seeing these deopts? You should be able to share the deoptigate output without revealing too much about your own code, right? It should just point at locations in TS?

[jasnell]

It's massive, I'll pick out bits... here's one:

https://github.com/microsoft/TypeScript/blob/8ed251d0c79920f1a745349dc7c5ff190b115450/src/compiler/utilities.ts#L3096-L3104

[jasnell]

In the above we're getting an eager deopt in two places, both on the name. The first occurs on the initial access to name.kind (that's the first one in the screenshot above). The second is on the name.argumentExpression access.

[weswigham]

Hm, the last time we specifically culled v8 deopts was when we adjusted field initialization order for most objects to be the same under most circumstances, so it's been awhile. There are definitely certain objects we _expect_ to be megamorphic, though, like SymbolLinks and NodeLinks. I experimented with inverting those cache structures to use sparse arrays rather than metamorphic property accesses awhile ago and saw no meaningful real perf improvements in the latest versions of node. So some specifics could help.

[jasnell]

Yeah, I think a lot of these are going to be necessary. Node.js 15 makes a number of improvements internally so it might be worth revisiting. The only problem there is that deoptigate appears to be broken on Node.js 15. I haven't had a chance to investigate why yet but it's not picking up everything.

I would, however, look to see if you can do something about the wrong call target deopts, and there's one out of bounds read here: https://github.com/microsoft/TypeScript/blob/8ffb7f083daa1af68b8950896db42271c47bbd2c/src/compiler/path.ts#L131

I'm trying to encourage the customer I'm working with to open a PR to submit a fix for that one. It's simple... there's a check to see if path is zero length, but not a check to see if length > 1 before checking the code point.

[jasnell]

Here's the actual deoptigate output... deoptigate.zip

[DanielRosenwasser]

There's a ton of unprotected chatCodeAt calls that we might just want to invest in a helper that either checks against a given value or returns a NUL.

[rbuckton]

Also, keep in mind that any code that uses ts.Node is probably going to be megamorphic. Even though we usually switch on node.kind, that .kind lookup will always be megamorphic because it doesn't matter that every Node subtype has the same initial set of properties, they are all different "maps" in v8.

[jasnell]

@rbuckton ... yep, for those I don't think there's anything that can or should be done really. If nothing else, however, it would be good to give a pass at scrubbing out the other deopts.

[amcasey]

I'll be the first to admit that I'm not a deopt expert, but I didn't see anything particularly alarming in the trace (Edge was really unhappy with the size of the linked one, so I made one from another project I find generally useful for performance analysis). The two biggest buckets were node property accesses, which we've discussed at length and could only change with a major investment, and array length (because or helpers operate on many array types). I'll create a PR addressing some of the low-hanging fruit (e.g. charCodeAt), but I'm not convinced the difference will show up in the perf run output.

[amcasey]

To summarize, I tried:

1. Adding bounds checks in front of getCharCode accesses flagged by deoptigate
2. Adding bounds checks in places I observed an instrumented version of the scanner (hot code) going out of range
3. Reviewing deopts in functions with self time > 1%

At the moment, it appears that the deopts that can be fixed without deep changes to the compiler have relatively little perf impact. We're also too far from "clean" to fix them on hygienic grounds.

Having said that, this remains an area of interest for the TS team and we do periodically make a sweep for particularly deleterious deopts. By the same token, if we get a PR that speeds up the perf tests, we'll be very enthusiastic.

Thanks again for starting this conversation, @jasnell!