Cache property name widening contexts - #40732
Conversation
When creating a new `WideningContext` for a given property, check whether there's an existing `WideningContext` with the same `parent` and `propertyName`. Otherwise, we won't adequately leverage the cache of computed `siblings` stored on the `WideningContext`.
|
TypeScript Bot (@typescript-bot) perf test this |
|
Heya Andrew Casey (@amcasey), I've started to run the perf test suite on this PR at 647421f. You can monitor the build here. Update: The results are in! |
|
Here's a cleaned up and annotated repro. |
|
Andrew Casey (@amcasey) Here they are:Comparison Report - master..40732
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Andrew Casey (@amcasey) seems like this is ready to merge? Or does it need to wait for 4.2? |
|
Nathan Shively-Sanders (@sandersn) I was saving it for 4.2, but only because there's no rush. |
|
With #40778 the motivating example no longer creates a large union type, so I wonder if this PR is still worth doing? |
|
Anders Hejlsberg (@ahejlsberg) I'm a little concerned that it'll bite us again in the future if we leave it quadratic, but I agree that there's no urgency. Your call. |
|
Andrew Casey (@amcasey) The logic only kicks in for fresh object literal types and once #40778 goes in I'm not sure there's anything one would commonly (or even uncommonly) write that generates large unions of fresh object literals. So I vote we hold off. |
When creating a new
WideningContextfor a given property, check whetherthere's an existing
WideningContextwith the sameparentandpropertyName. Otherwise, we won't adequately leverage the cache ofcomputed
siblingsstored on theWideningContext.On my box, check time drops from 10 seconds to 0.3 seconds for this example.