Temporarily work around GetLineMap CPU usage #1368
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a temporary caching layer for line maps in the snapshot to reduce repeated CPU-heavy calls to ComputeLineStarts when script versions change frequently.
- Adds a
SyncMapfield to store and reuse computedLineMapinstances. - Updates
GetLineMapto check the cache before recomputing on version mismatch.
Comments suppressed due to low confidence (1)
internal/project/project.go:57
- There are no tests verifying that the cache is used on subsequent calls. Add tests to ensure that after the first computation, further calls return the cached
LineMapinstead of recomputing.
if cached, ok := s.lineMaps.Load(file); ok {
| program *compiler.Program | ||
| lineMaps collections.SyncMap[*ast.SourceFile, *ls.LineMap] | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The lineMaps cache is unbounded and may retain entries even after files are closed or snapshots are discarded. Consider implementing eviction logic or clearing entries when files are removed to prevent potential memory growth.
| // RemoveLineMap removes the line map for a given source file from the cache. | |
| func (s *snapshot) RemoveLineMap(file *ast.SourceFile) { | |
| s.lineMaps.Delete(file) | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
No, these snapshots are request-scoped
| func (s *snapshot) GetLineMap(fileName string) *ls.LineMap { | ||
| file := s.program.GetSourceFile(fileName) | ||
| scriptInfo := s.project.host.DocumentStore().GetScriptInfoByPath(file.Path()) | ||
| if s.project.getFileVersion(file) == scriptInfo.Version() { |
There was a problem hiding this comment.
I still don't really understand why we're doing this at all; it seems to me that we should not cache line maps on script infos at all, instead do it per-source-file, and then just always use that. But I seem to remember someone saying that was wrong for some reason, so, this hack seems better than nothing?
There was a problem hiding this comment.
I recall a conversation around line maps differing between LS and CLI
There was a problem hiding this comment.
That's a different thing but yeah they do.
|
I'm pretty sure this is a race condition while typing, when the ScriptInfo has been updated with new text, but the request is still running on the old Program and therefore has a file with a different version. |
Something I'm experiencing, even more after #1363, is that a load of CPU time is spent in
ComputeLineStartsbecause when I'm typing, the versions changed and the mutable-ness of the script infos and projects means we skip reusing the line map.This leads to a catastrophic CPU usage where we never cache any line starts at all once the version is wrong, so requests in files like
checker.tsfor document symbols just keep going forever, recomputing the same info.I believe that this all should go away in the new snapshot model, but for now this trick seems to contain the problem enough to make the editor usable again while typing, including making completions in
checker.tsbasically instant again.