Skip to content

Make SourceTextData thread-safe with ConcurrentDictionary #20113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xperiandri wants to merge 2 commits into
base: main
Choose a base branch
from
+13 −15
Open

Make SourceTextData thread-safe with ConcurrentDictionary #20113

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b4cbbf
Initial plan
Copilot Aug 2, 2026
8bee41a
Make SourceTextData thread-safe with ConcurrentDictionary
Copilot Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 13 additions & 15 deletions vsintegration/src/FSharp.Editor/LanguageService/Tokenizer.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,30 +459,28 @@ module internal Tokenizer =
&& let lineContents = textLine.Text.ToString(textLine.Span) in
data.HashCode = lineContents.GetHashCode()

// Shared by concurrent editor operations (classification and symbol lookup), so must be thread-safe.
type private SourceTextData(approxLines: int) =
let data = ResizeArray<SourceLineData option>(approxLines)

let extendTo i =
if i >= data.Count then
data.Capacity <- i + 1

for j in data.Count .. i do
data.Add(None)
let data =
ConcurrentDictionary<int, SourceLineData>(Environment.ProcessorCount, approxLines)

member x.Item
with get (i: int) =
extendTo i
data.[i]
match data.TryGetValue(i) with
| true, v -> Some v

@T-Gro T-Gro Aug 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getter allocates a new Some for every cache hit. One million reads allocated 24 MB, while the old getter and voption allocated none. Return voption or expose an allocation-free lookup.

| _ -> None
and set (i: int) v =
extendTo i
data.[i] <- v
match v with
| Some v -> data.[i] <- v
| None -> data.TryRemove(i) |> ignore

member x.ClearFrom(n) =
let mutable i = n
let mutable cont = true

while i < data.Count && data.[i].IsSome do
data.[i] <- None
i <- i + 1
while cont do
let removed, _ = data.TryRemove(i)
if removed then i <- i + 1 else cont <- false

@T-Gro T-Gro Aug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking observation: ConcurrentDictionary makes each individual entry access atomic, which fixes the memory-safety bug. However, the multi-line read-modify-write sequence in getFromRefreshedTokenCache (read [i] -> scanSourceLine -> set [i], chaining lexState, then ClearFrom(endLine+1)) is still not atomic as a whole. Two concurrent scans over overlapping line ranges on the same SourceTextData can interleave and briefly cache an inconsistent lex-state chain. This is a strict improvement over the old code (which could corrupt the list), and it self-heals on the next read via the IsValid/LexStateAtStartOfLine checks, so it is almost certainly acceptable. Worth noting only that the guarantee is container-level, not sequence-level, so the must be thread-safe comment does not imply a coherent snapshot across lines.


/// This saves the tokenization data for a file for as long as the DocumentId object is alive.
/// This seems risky - if one single thing leaks a DocumentId (e.g. stores it in some global table of documents
Expand Down
Loading