Only use resolutionStart when pushing resolution stack #58824
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
requested review from
DanielRosenwasser,
weswigham,
ahejlsberg and
RyanCavanaugh
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
The perf tests seem to have a decent amount of variance - can you give a clue on what sorts of savings you're seeing specifically on arktype (the report in #58805)? |
|
Just finished running this: |
|
@jakebailey @DanielRosenwasser Awesome, this seems to totally address the regression on my end: 5.4.5:
{
"checkTime": 8.74,
"types": 479804,
"instantiations": 2612248
}
5.5.1-rc
{
"checkTime": 9.92,
"types": 514330,
"instantiations": 2812921
}
fix-58805
{
"checkTime": 8.66,
"types": 483064,
"instantiations": 2622889
}Thanks for taking this so seriously. It's ridiculously impressive how well your team integrates real-world perf feedback from across the ecosystem into your dev process. |
|
Hmm, I'm not 100% sure about this one. The whole idea with |
|
@jakebailey Do we know what led to this change impacting ArkType more than the other repos that were tested? If it stems from some obscure pattern that is unlikely to exist in other projects, then identifying it will (hopefully) give me a chance to refactor the affected code. If it stems from something more ubiquitous, it may be worth weighing the more consistent errors @ahejlsberg mentioned against the perf cost if there is no way to provide both. |
|
I didn't have time to debug further, but my impression is that there's something very specific to signatures here; the new stack resetting happens as a part of resolving a signature, but this check is checking for a resolving signature, more or less. Makes me think that we're pushing onto the stack then shortly after, resetting it such that we forget that we're resolving. (Which is probably clear given the fact that it works once we look past the new start.) So, it sort of feels it may be the case that there's some way to avoid that. I plan to look harder when I have a little more time. |
|
FWIW this regression is measurable with ts-perf (not surprising); 5.4 vs main: |
|
Any additional thoughts on either potential plans to address this or ideas about what the culprit may be in the ArkType repo that's causing the performance issues? Is there maybe a broader suite of repos that can be checked for perf changes stemming from this change? As mentioned, I'm happy to refactor some internal types if need be to accommodate these changes, but I don't have very good intuitions about what code is most likely responsible for the delta. |
Jake might, but the only advice I have at the moment is to try |
|
@DanielRosenwasser Would getting some more data on the broader perf impact of that change be valuable so that it's clear the regression that occurred in ArkType is a clear outlier? From what I understood, the core fix was already an edge case and Jake's PR (which had negligible overhead) apparently still addresses the majority of those cases. I understand wanting to guarantee ideal behavior as often as possible, but a very noticeable performance hit that could potentially occur in other repos as well seems like a hefty price to pay in this case for that extra bit of certainty. |
|
I am continuing to look at this; doing some extra tracing of the code involved here to try and compare before/after. |
|
I've instrumented some basic tracing of the push/pop of signatures and Here's an example (not "the" example, but one that can fit in one screenshot):
On the left is We can see that we This PR works because it allows If this PR isn't acceptable, then I'm not totally sure what the right fix will be. I was thinking that maybe the stack could only be reset back up until the signature being resolved, but we're resetting in I also think that using the stack like this for non push-pop circularity checking reasons may itself be the bad thing, but changing that seems challenging. Maybe the resetting needs to be somewhere other than Any ideas, people smarter than me? 😄 @ahejlsberg |
|
Here is the trace: https://gist.github.com/jakebailey/6718e643e3399316962bd8edf53f4558 If you're looking for "expensive" calls, I would search the "main.txt" for |
|
I am however able to recover maybe 50% of the loss by doing what we do for the variance reset and only doing it once; the comment in the code for the |
Unfortunately that's not the case, only Anders PR addressed the bug, we tested the other proposed fixes and they did not work except for small isolated cases. I would not pick perf over correctness |
Fixes #58805
#58337 resets the stack by moving its start up temporarily. However, there are other parts of the checker other than the push/pop circular error code which rely on the stack. It looks like for #58805,
isResolvingReturnTypeOfSignaturein particular stops knowing that a particular return type is being resolved, causing us to start doing extra work that won't go anywhere."Fix" this by always using the full stack for these non-circularity-checking stack users.
To make this more clear, the second commit refactors the code a bit into clear functions that show which do and don't use the full stack.
Perhaps this isn't the right fix; this passes all tests, but maybe we shouldn't be using the stack for anything but circularity checking in push/pop anyway. That's a more invasive change.