Improve performance of UriIdentityService (#273108) #273111
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the performance of the UriIdentityService by optimizing its internal caching mechanism. The changes reduce object creation overhead and improve cache cleanup efficiency.
Key changes:
- Migrated from SkipList to native Map with string keys to reduce URI object creation
- Refactored path casing logic into a separate PathCasingCache class
- Replaced full-sort cache cleanup with quickSelect algorithm to find median value
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/platform/uriIdentity/common/uriIdentityService.ts | Refactored UriIdentityService to use Map with string keys, extracted PathCasingCache class, and optimized cache cleanup with quickSelect |
| src/vs/platform/uriIdentity/test/common/uriIdentityService.test.ts | Added test to verify cache eviction respects access time when cache overflows |
src/vs/platform/uriIdentity/test/common/uriIdentityService.test.ts
Outdated
Show resolved
Hide resolved
| Entry._clock = 0; | ||
| const times = [...this._canonicalUris.values()].map(e => e.time); | ||
| const median = quickSelect( | ||
| Math.floor(times.length / 2), | ||
| times, | ||
| (a, b) => a - b); | ||
| for (const [key, entry] of this._canonicalUris.entries()) { | ||
| if (entry.time < median) { | ||
| this._canonicalUris.delete(key); | ||
| } else { | ||
| return 0; | ||
| entry.time = 0; | ||
| } | ||
| }); | ||
|
|
||
| Entry._clock = 0; | ||
| this._canonicalUris.clear(); | ||
| const newSize = this._limit * 0.5; | ||
| for (let i = 0; i < newSize; i++) { | ||
| this._canonicalUris.set(entries[i][0], entries[i][1].touch()); | ||
| } |
There was a problem hiding this comment.
The entry.time values are being reset to 0 after quickSelect has already used them to calculate the median, but entries with time < median are deleted while keeping others. This creates an inconsistency where the median calculation is based on old times, but the comparison uses those same old times. Additionally, after deletion, the remaining entries all have time = 0, which means the next cache cleanup will not properly distinguish access patterns. Consider resetting Entry._clock and entry.time values only after the cleanup is complete, or adjust the logic to maintain proper time tracking.
There was a problem hiding this comment.
Not sure if I can correctly understand this AI generated commit. The logic behind the change if the following:
- We calculate median based on current items
- Then we remove items with time <= median and reset it to
0for those which left. - This logic utilizes the fact that we clean up exactly the half of the cache.
- All remain items will have time
0. - On next cleanup all new items will have time >= 1. Items left from previous cleanup will have time === 0 if they have not been touched and >= 1 if they have been touched. If they have not been touched we do not care in their order, since all of them will be trimmed.
- So the algorithm guarantees that amount of items with time === 0 is always strictly less then a half, and this is enough for correct trim.
Yavanosta
force-pushed
the
UriIdentityService-perf
branch
from
42f61f3 to
528a459
Compare
Yavanosta
force-pushed
the
UriIdentityService-perf
branch
from
528a459 to
01f4c01
Compare
4 participants
This CL improves perfrormance of UriIdentityService:
quickSelectto pick median value instead of sorting the entire cache.Changes measured on my local machine (MacBook Air M4):
153ms->18ms297ms->81ms297ms->17msFixes #273108