Cache expression type when checking assertion #54224
Conversation
|
@typescript-bot perf test this faster |
|
@typescript-bot perf test this faster |
|
Heya @gabritto, I've started to run the abridged perf test suite on this PR at 1175676. You can monitor the build here. Update: The results are in! |
|
@gabritto Here they are:Comparison Report - main..54224
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @gabritto, I've started to run the perf test suite on this PR at 1175676. You can monitor the build here. Update: The results are in! |
gabritto
changed the title
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@gabritto Here they are:
CompilerComparison Report - main..54224
System
Hosts
Scenarios
TSServerComparison Report - main..54224
System
Hosts
Scenarios
StartupComparison Report - main..54224
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @gabritto, I've started to run the abridged perf test suite on this PR at c790579. You can monitor the build here. Update: The results are in! |
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at c790579. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at c790579. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at c790579. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at c790579. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at c790579. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at c790579. You can monitor the build here. Update: The results are in! |
|
Looks like there's going to be a memory bump for Angular, but I think that is a worthwhile tradeoff. |
|
I was doing the comparison of the results of this PR (#54224 (comment)) with the ones in the original PR that introduces the regression (#53261 (comment)). It's tricky to compare those directly, but it seems like this PR takes care of most of the regression on |
|
@gabritto Here they are:Comparison Report - main..54224
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
I think that's fine - but I have some ideas we can try. Two optimizations that would be interesting to try in follow-up PRs would be:
|
| } | ||
| const links = getNodeLinks(node); | ||
| links.assertionExpressionType = checkExpression(expression, checkMode); |
There was a problem hiding this comment.
By moving the checkExpression for the as const case into the if, it's no longer used in the deferred-check case at all; so you shouldn't actually need to cache it like this (which is a tad unsafe, so if it's avoidable, I'd like to avoid it), since you'll either do it once early or once deferred.
There was a problem hiding this comment.
So what're you thinking @weswigham? Place this in an explicit else?
There was a problem hiding this comment.
Or I guess, keep it in the same as before, but then cache it on links unconditionally?
That makes sense, you're calling it twice in some cases anyway now, right?
There was a problem hiding this comment.
If I understand correctly, that's what I was doing in https://github.com/microsoft/TypeScript/pull/53261/files/45124b1447f2ff88bdf2f9e65f0e1766a80a70a3, and that's what caused xstate checking perf to increase by 10-13% (#53261 (comment)).
There was a problem hiding this comment.
@gabritto but aren't you calling whoops, duh, it's an early return.checkExpression twice now?
There was a problem hiding this comment.
I guess there's no "checking twice" but there's some code deduplication - checkExpression is going to happen anyway. I'd rather we got rid of that and go back to having exprType
That said, I don't see why avoiding deferral is a problem in that case.
There was a problem hiding this comment.
I'd rather we got rid of that and go back to having exprType
I wouldn't. The CheckMode-independent unconditional cache is unsafe and could lead to weird inconsistencies between signature checking states, which can lead to odd editor/command line error inconsistencies. We do it in one or two other places for perf reasons, but it's to be avoided where possible.
|
@DanielRosenwasser Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
We can do this, but the impact should be a minor memory pressure improvement - perf-wise it should be neutral, unless it just barely puts us over the GC line in a project. I'll be surprised if this does too much. If it does.... I'd just call the original change unlucky(?), since almost any new temporary object could have triggered the increased cost.
That would require actually checking the expression early, which we can entirely avoid in the non-as-const case which is what we needed to avoid to prevent the circularity error in the first place. For the expression type, anyway. For the assertion type, sure; but, again, just deferring a node's check is pretty free (all it means is we add an id to a set and come back to it later) - in fact, usually it's perf-positive to defer a node check because it prevents us from repeating the work when we check an expression tree multiple times during signature resolution (by only doing it once at the end once the signature types are locked in). Avoiding the deferral isn't something I think'll actually be too useful in terms of performance - mostly just complicated. Who knows, I could be wrong, but my hopes aren't high. Back on this fix, though - If |
|
It's also possible that the original fix actually just removed circularity errors in |
If I'm not mistaken, I think the expression already is checked early, and unconditionally. The deferred work is checking if the two types are comparable in either direction. |
You're right, it is. I'm just thinking we can actually defer the expression check now, too in the non-as-const case, which should be a savings. |
That sounds like a reasonable explanation for the weird |
I just checked - the |
|
@typescript-bot perf test this faster |
|
Heya @gabritto, I've started to run the abridged perf test suite on this PR at 5b536d5. You can monitor the build here. Update: The results are in! |
|
Ok, I compiled |
src/compiler/checker.ts
Outdated
| checkSourceElement(typeNode); | ||
| const type = getTypeFromTypeNode(typeNode); | ||
| // See if we need to check whether the expression and asserted types are comparable. | ||
| if (!isErrorType(type) && !((exprType.flags | type.flags) & (TypeFlags.AnyOrUnknown | TypeFlags.Never))) { |
There was a problem hiding this comment.
IMO, this is silly - this is quite literally what the first lines of isTypeAssignableTo (via isSimpleTypeRelatedTo) does, and all doing it early avoids is putting a node id into a set (in exchange for duplicating this comparison in the cases where that isn't avoided, which is far and away more common).
|
@gabritto Here they are:Comparison Report - main..54224
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Yep, like Wesley pointed out, latest changes didn't really help. I'll revert. |
|
Seems reasonable! I wanted to have that as an experiment in a separate PR anyway. |
| @@ -34523,6 +34523,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
| return getRegularTypeOfLiteralType(exprType); | |||
| } | |||
| const links = getNodeLinks(node); | |||
There was a problem hiding this comment.
| const links = getNodeLinks(node); | |
| function checkAssertionWorker(node: JSDocTypeAssertion | AssertionExpression, checkMode: CheckMode | undefined) { | |
| if (isConstTypeReference(type)) { | |
| if (!isValidConstAssertionArgument(expression)) { | |
| error(expression, Diagnostics.A_const_assertions_can_only_be_applied_to_references_to_enum_members_or_string_number_boolean_array_or_object_literals); | |
| } | |
| return getRegularTypeOfLiteralType(checkExpression(expression, checkMode)); | |
| } | |
| checkSourceElement(type); | |
| checkNodeDeferred(node); | |
| return getTypeFromTypeNode(type); | |
| } |
and no changes in checkAssertionDeferred from current main should result in the same number of checkExpression calls as the unsafe cache here, but without needing an unsafe cache, is what I'm trying to say. :P
There was a problem hiding this comment.
So
- getting rid of
assertionExpressionTypeonNodeLinksentirely - just only doing
checkExpressioninconstcontexts and in the deferred check.
Makes sense.
|
Hey @DanielRosenwasser, the results of running the DT tests are ready. |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
Experiment to see if we can revert the perf impact of #53261.