Avoid allocations in AbstractSyntaxIndex<>.GetIndexAsync

[sharwell]

Further improves a scenario that also suggested it will be improved by #73493.

Based on allocations observed in AB#2090900.

[ToddGrun]

// Avoid capturing index on the fast path by making a copy for the slow path

Could this be moved to a local fn to avoid the allocation instead?

[sharwell]

It could, but it wouldn't be less net code. Creating a temporary local is pretty standard practice for this.

[ToddGrun]

OK, I've never noticed it done this way whereas I've seen it done the other way a couple times.

[CyrusNajmabadi]

The NET part lgtm. I don't get the value in the local copy. We'll still have the closure allocation there afaict

[sharwell]

The NET part lgtm. I don't get the value in the local copy. We'll still have the closure allocation there afaict

The problem isn't the closure allocation when used. The problem is it still creates a closure allocation for index when s_documentToIndex.TryGetValue returns true, and these lambdas are never used.

[CyrusNajmabadi]

How does the local copy help there? Can you show the il diff? Thanks!

[sharwell]

Here's a SharpLab example:

https://sharplab.io/#v2:C4LglgNgPgAgTARgLACgYAYAEMEDoBKArgHbBgC2AprgMID25ADpJQE4DKbAbmAMaUBnANyoM2BAFYRKUQGZscTDUwBvVJg2ZGrMFwCGwSuIBsmVpT0ATOsQgBPJTctgyNvRADqFgNYAVPQBGEJQAPHQBAFaUvMAANJjhUTEA/AB8mAIA+ta8hFSkvnQAksSWlAAemAC8mMSUAO4AFACU0uqa2nSGMZSWJtgAHJgAau6ElDDGYZHRwGmYAOKUwCVl5QCCAnbEvI3tmpqJs5g5eZSkzfuqVwcA9LeYnEZgAGaY9Ubu5lYOvHq8AAtepg9MRMGBShV3i4AZhgACwAITmBzDETnRcvlgODIeVcBoim8BHR3kY/mCIoQBMAbpp7tgAOzg7H1GG1ElWZyuYjud50VjeXC0jSvTCNACEWVOWMKqwquF8rDsS2Aowg40a0vOcQShGx+lYOLWzUuKAOBzUZvN1oha2q2AAnItlnLyh5+d42JttrtmrQbC8wABzQjmdb1PQuRovdwCSitYXm0WNW1Qqo1YiECAQU3WvOaGBMzPZtpW/MaekABTojCzBiMdFDmD+gME0PhcIRSMsBj0QrL5fp6xgABZm3pGMBQxCg0aoTZO0YY9StAZYQEHOQ9N4ZyDmzWHC9+YuMhA6PVV/DEwcpRizgVirjcCq1RqtaR4plqulU+UEwODgAX1QRNCzncppCAkCB06bpDD6HBTBgIZXwmKYjhSdIVVdehGDsb0dj2ADDhmNF32AXNNEtfN6SecE3g+EEIG+Sxfn+IE+lBcD21heFEWRVFsXI8D8UwQkMhJRjyUwSlqUTekwJcHj2RBSwuTANwID5AV+3zZNJWyO8ZUfNYFSVF8xkoTUjO1eJG31PRDV/E1E2o8sRVxe0YCdbDcXdAUvS2Qi/XoYhAxDMMIyjGMIDjf93JFN4U089NaizHNrzzMDiwgUsEswKsazrQxdUNFsgSRVkOz47te10hKh1HcdJ2nYhZ1/BIwXhJc9BXRg10wDdMC3Hc2r3XgD0wI9DW609z0vAFMo0A1wNwhwal/SCEtvTFtVlJ8LPVKzyM/b9VoPeK82AmRiI0RTcS2jRrquWDZmBRDsHQ0i5iwl0/I9QKfRaRMqnSSZcAAMVYBh8EELNgGmJIfsaOoLww4AWn/a6gA===

The key difference is the "original" code has a newobj before calling TryGetValue:

IL_000a: ldarg.0
IL_000b: newobj instance void C/'<>c__DisplayClass1_0'::.ctor()
// ... omitted for brevity ...
IL_002b: callvirt instance bool class [System.Runtime]System.Runtime.CompilerServices.ConditionalWeakTable`2<object, object>::TryGetValue(!0, !1&)
IL_0030: brtrue IL_00d9

While the "copy" code only has it after calling TryGetValue:

IL_0017: callvirt instance bool class [System.Runtime]System.Runtime.CompilerServices.ConditionalWeakTable`2<object, object>::TryGetValue(!0, !1&)
IL_001c: brtrue IL_00cd

// sequence point: hidden
IL_0021: ldarg.0
IL_0022: newobj instance void C/'<>c__DisplayClass2_0'::.ctor()