Control flow for element access expressions #31478
Conversation
Draft version, just want to see how performance is
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at bb3edc8. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..31478
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/compiler/checker.ts
Outdated
| let assumeUninitialized = false; | ||
| if (strictNullChecks && strictPropertyInitialization && node.expression.kind === SyntaxKind.ThisKeyword) { | ||
| const declaration = prop && prop.valueDeclaration; | ||
| if (declaration && isInstancePropertyWithoutInitializer(declaration)) { |
There was a problem hiding this comment.
Could this be unified with the similar code in checkPropertyAccessExpressionOrQualifiedName? Maybe a shared worker, like getFlowTypeOfReferenceWithImpliedInitialType?
There was a problem hiding this comment.
yes! It is basically a cut+paste+rename, so it is next on my list after I find out how much this hurts performance.
|
Hm, no change in perf? That can't be right. I'm going to add a micro-benchmark to the perf test suite and report its results. |
|
I just wrote a stress test for this code. It's of the form: function stress(o: { [s: string]: "aa" | "ab" | "ac" | ... }) {
o["x"] = o["z"] === "de" ? "ff" : "ip";
// many more lines like this...
return [o['x'], o['y'], o['z']] as const
}It scales quite well; check time at 100 lines was 0.28 seconds, at 1000 lines it was 0.5 and at 10,000 it was 0.6. Memory increases from 128 MB to 157 MB which is not bad. What didn't work as well was code like this: function stress(o: { [s: string]: "aa" | "ab" | "ac" | ... }, x: string, y: string, z: string) {
o[x] = o[z] === "de" ? "ff" : "ip";
// many more lines like this...
return [o[x], o[y], o[z]] as const
}It scales linearly or a little worse, up to 5.5 seconds and 178 MB for 16,000 lines. I'll see if I can get a newer code base or two added to our performance suite although I'm not sure what is a good test for this change. |
|
Update: with more data points, it's pretty clear that the second example scales exponentially, not linearly. |
|
Well, this PR is as ready to go as it'll ever be. Any opinions @ahejlsberg? @rbuckton any idea for good code bases that would show off real-world perf? My synthetic benchmark is intentionally unrepresentative. |
|
From the brief discussion in the 6/28 design meeting (#32357), @rbuckton raised the question of performance on a stress test, which is a variant of the first where identifiers of literal type are substituted for literals. I expect performance to be about the same. function stress(o: { [s: string]: "aa" | "ab" | "ac" | ... }, x: 'x', y: 'y', z: 'z') {
o[x] = o[z] === "de" ? "ff" : "ip";
// many more lines like this...
return [o[x], o[y], o[z]] as const
} |
|
I did some additional performance investigation, using the same synthetic stress test as above with 3 variants: Variant 1 - Literal, Narrowingfunction stress(o: { [s: string]: "aa" | "ab" | "ac" | ... }) {
o["x"] = o["z"] === "de" ? "ff" : "ip";
// many more lines like this...
return [o['x'], o['y'], o['z']] as const
}Variant 2 - Primitive, Non-Narrowingfunction stress(o: { [s: string]: "aa" | "ab" | "ac" | ... }, x: string, y: string, z: string) {
o[x] = o[z] === "de" ? "ff" : "ip";
// many more lines like this...
return [o[x], o[y], o[z]] as const
}Variant 3 - Literal Type, Non-Narrowingfunction stress(o: { [s: string]: "aa" | "ab" | "ac" | ... }, x: 'x', y: 'y', z: 'z') {
o[x] = o[z] === "de" ? "ff" : "ip";
// many more lines like this...
return [o[x], o[y], o[z]] as const
}The most important result is that in real-world scenarios, this change never makes a significant difference in compile time: the second two variants don't actually create flow nodes in the binder, so control flow quits early as there's nothing to analyze. This means that no narrowing happens, of course. Inserting flow nodes only for element accesses with literal access expressions was a performance decision from quite some time ago. As far as I know, we have no reason to revisit that decision. The first variant does narrow, but only 1000 lines. After that, control flow throws an error "The containing function or module body is too large for control flow analysis." So I cut the stress tests down to 1000 lines and did a before/after comparison of them: As you can see, the worst case is less than 50% increase per very-long function. I think that's acceptable given the improvement in narrowing for the common case. |
Fixes #28081
I did some additional performance investigation, using the same synthetic stress test as above with 3 variants:
Variant 1 - Literal, Narrowing
Variant 2 - Primitive, Non-Narrowing
Variant 3 - Literal Type, Non-Narrowing
The most important result is that in real-world scenarios, this change never makes a significant difference in compile time: the second two variants don't actually create flow nodes in the binder, so control flow quits early as there's nothing to analyze. This means that no narrowing happens, of course. Inserting flow nodes only for element accesses with literal access expressions was a performance decision from quite some time ago. As far as I know, we have no reason to revisit that decision.
The first variant does narrow, but only 1000 lines. After that, control flow throws an error "The containing function or module body is too large for control flow analysis."
So I cut the stress tests down to 1000 lines and did a before/after comparison of them:
As you can see, the worst case is less than 50% increase per very-long function. I think that's acceptable given the improvement in narrowing for the common case.