perf: use append instead of Concat in appendLookupLocations #632
Conversation
This reduces alloc/GC churn as append in theory can expand a slice without having to copy the original. Loosely this appears to massively improve performance inside ResolveModuleName.
|
@andrewbranch would know better but IIRC there's some hazard in this specific case where the way the initial slice comes in could mean that the slices are shared between two things. We'd need to audit to make sure that the initial slice is a copy/clipped so that these appends are safe. |
Gotcha, I did a check for this before raising and I couldn't see any case where the caller was both consuming and providing the slice into this method, i.e. only one thing ends up holding the slice Also worth noting that we aren't appending to the input here, rather appending the input to the existing value. So even if something does retain the input, we're effectively copying it in this case still anyway |
|
To make me feel better, can you check to see if you do |
|
Can do, I strongly suspect it won't retain all the perf improvements tho as by definitions we'd be removing the extra space we're currently appending into |
This reduces alloc/GC churn as append in theory can expand a slice without having to copy the original. Loosely this appears to massively improve performance inside ResolveModuleName. In one example running the LSP locally it drops initialization time from ~90 seconds to ~20 seconds.
I believe this is still safe given the usage of mutex's, but also my understanding of go threading is not the best, so if this isn't safe lemme know. But it appears to be fine both locally and in the test suite.
Before
After
