Skip to content

Slice source text instead of copying it line by line for outlining - #20443

Open
xperiandri wants to merge 11 commits into
dotnet:mainfrom
xperiandri:outlining-line-slices
Open

Slice source text instead of copying it line by line for outlining#20443
xperiandri wants to merge 11 commits into
dotnet:mainfrom
xperiandri:outlining-line-slices

Conversation

@xperiandri

@xperiandri xperiandri commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Description

The editor's block structure built the sourceLines array for Structure.getOutliningRanges by calling ToString() on every line, allocating a fresh string for the whole file on each outlining pass, that is once per keystroke.

The scanner now takes ReadOnlyMemory<char>[] and sits behind two entry points:

  • getOutliningRanges keeps its public string[] signature and maps to memory, so FSharp.Compiler.Service consumers are unaffected;
  • getOutliningRangesFromLineSlices is internal and takes the slices the caller already holds. The editor slices the materialized source text once through a new SourceText.GetLinesAsMemory() helper and calls this one.

Inside the scanner, comment detection trims and classifies lines over spans instead of building trimmed strings, and comment groups track line numbers only, since only the first and last lines of a group are ever read back to compute the fold's columns.

ReadOnlySpanCharExtensions in illib mirrors the existing ordinal String helpers so span call sites read the same way.

No public API change: the surface-area baseline is untouched.

Fixes # (no issue)

Checklist

  • Test cases added: the existing StructureTests (39 cases) exercise every fold kind through the shared scanner, reached via the public entry point.
  • Performance benchmarks added in case of performance changes: posted as a comment below. One outlining pass over a 13.6k-line file drops from 5.3 ms / 4.4 MB to 0.9 ms / 1.0 MB.
  • Release notes entry updated: docs/release-notes/.FSharp.Compiler.Service/11.0.100.md (Improved) and docs/release-notes/.VisualStudio/18.vNext.md.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
`src/Compiler` docs/release-notes/.FSharp.Compiler.Service/11.0.100.md
`vsintegration/src` docs/release-notes/.VisualStudio/18.vNext.md

@xperiandri

Copy link
Copy Markdown
Contributor Author

Benchmark

BenchmarkDotNet, both versions of the changed code copied verbatim into a standalone harness (the compiler-internal helpers they use are internal, so the harness redefines the two one-line StartsWithOrdinal shims). before is ServiceStructure.fs at a29233c, after is this branch. The ScopeRange/range construction is stubbed identically on both sides so it does not skew the comparison, and [<GlobalSetup>] asserts both scanners find byte-identical comment blocks before any measurement runs.

Inputs are two real files from this repo: ServiceStructure.fs (1,107 lines) and CheckExpressions.fs (13,653 lines).

BenchmarkDotNet v0.15.4, Windows 11 (10.0.26200.9278)
AMD Ryzen 9 5980HS, 1 CPU, 16 logical and 8 physical cores
.NET SDK 10.0.400, .NET 10.0.11, X64 RyuJIT x86-64-v3
Step File Before After Time Alloc before Alloc after
Build line array ServiceStructure.fs 15.6 us 3.5 us 4.4x 122 KB 17 KB
Comment scan ServiceStructure.fs 77.9 us 43.3 us 1.8x 180 KB 54 KB
Build + scan ServiceStructure.fs 124.3 us 53.8 us 2.3x 302 KB 71 KB
Build line array CheckExpressions.fs 1,923 us 102 us 18.8x 1,753 KB 213 KB
Comment scan CheckExpressions.fs 1,364 us 444 us 3.1x 2,645 KB 780 KB
Build + scan CheckExpressions.fs 5,328 us 930 us 5.7x 4,399 KB 993 KB

"Build + scan" is what one outlining pass pays outside the AST walk, and the editor pays it per keystroke. On the large file that is 4.4 MB of garbage per pass before, 1.0 MB after, a 77% reduction; on the small file 76%.

Where the allocations went:

  • Line array. ToString() per line copied every line of the file. Slicing the already-materialized text allocates only the array of ReadOnlyMemory<char> structs. What is left in the "after" column is that array.
  • Comment scan. TrimStart() per line allocated a trimmed copy of every line just to test two prefixes; the span version trims in place. Comment groups also stored (int * string) tuples per line, now int only.

Variance is high on the multi-modal rows (the harness reports bimodal distributions on the scan benchmarks, and the full-pass rows carry a wide StdDev), so treat the time ratios as order-of-magnitude rather than precise. The allocation numbers are exact and are the substance of the change.

Two caveats worth stating plainly:

  • Measured on .NET 10. FSharp.Compiler.Service ships netstandard2.0, and the editor caller runs on .NET Framework 4.7.2, where ReadOnlyMemory/ReadOnlySpan come from the System.Memory package. No ArrayPool is involved on this path, so the relative picture should hold, but I have not measured the desktop runtime.
  • BenchmarkDotNet's header prints [Host] ... DEBUG. That is the known false positive for F# Release builds; DebuggableAttribute.IsJITOptimizerDisabled on the harness assembly is false.

@github-actions github-actions Bot added ⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen ⚠️ Affects-Build-Infra Tooling check: PR touches build infrastructure ⚠️ Affects-Restore Tooling check: PR touches NuGet packages or feeds labels Sep 3, 2026
@github-actions

This comment has been minimized.

Comment thread src/Compiler/Service/ServiceStructure.fs Outdated
Comment thread src/Compiler/Service/ServiceStructure.fs Outdated
@github-actions

This comment has been minimized.

Comment thread src/Compiler/Utilities/illib.fs
@xperiandri
xperiandri force-pushed the outlining-line-slices branch from 793ffee to 8e642e7 Compare September 4, 2026 16:56
Comment thread src/Compiler/Utilities/illib.fsi
Comment thread src/Compiler/Service/ServiceStructure.fs Outdated
@xperiandri
xperiandri requested a review from T-Gro September 4, 2026 21:52
@github-actions

This comment has been minimized.


/// Returns outlining ranges for given parsed input.
val getOutliningRanges: sourceLines: string[] -> parsedInput: ParsedInput -> seq<ScopeRange>
val getOutliningRanges: sourceLines: ReadOnlyMemory<char>[] -> parsedInput: ParsedInput -> seq<ScopeRange>

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.

🤖🕵️ Keep the public string[] getOutliningRanges entry point. Add a separately named internal memory-based entry for the editor and share the scanner implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in ac31157505.

getOutliningRanges keeps its string[] parameter and maps to memory; getOutliningRangesFromLineSlices is internal and takes the slices, and BlockStructureService calls that one. Both go through the same scanner — the wrapper is one line.

Two things fall out of this, both good: the surface-area baseline and the StructureTests call sites go back to exactly what they were on main, and the release note moves from Breaking Changes to Improved. PR description updated accordingly.

Build clean, StructureTests 42/42 through the public entry point.

@xperiandri
xperiandri force-pushed the outlining-line-slices branch from 1e21647 to b937173 Compare September 9, 2026 15:50
xperiandri and others added 9 commits September 11, 2026 17:56
The editor's block structure built the sourceLines array for
Structure.getOutliningRanges by calling ToString() per line, allocating
a fresh string for the entire file on every outlining pass, once per
keystroke.

getOutliningRanges now takes ReadOnlyMemory<char>[] and slices the
already-materialized source text once (SourceText.GetLinesAsMemory())
instead. ReadOnlySpanCharExtensions in illib mirrors the existing
Ordinal string helpers so span call sites read the same way string call
sites do.

A local recursive function closing over a ReadOnlySpan<char>-typed
sibling cannot be compiled - the CLR disallows instantiating
FSharpFunc<ReadOnlySpan<char>, _> as a closure field (FS0412) - so
commentTypeOf moves to module scope, next to the CommentType it
classifies.

StructureTests.fs slices its own lines the same way at the call site,
and FSharp.Compiler.Service.Tests needs a direct System.Memory
PackageReference: FSharp.Compiler.Service's own reference to it is only
transitive through the net472 ProjectReference's SetTargetFramework
override, mirroring the FSharp.Core pin already in this project for the
same reason.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CommentList kept a copy of every comment line next to its line number,
but the number alone identifies the line in the source array the
function already holds, and only the first and last lines of a group
are ever read back to compute the fold's columns. Store the numbers and
index the source at the end, so grouping comments allocates no tuple
per line.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CheckCodeFormatting flagged illib.fsi for a stray space before the
colon in the ReadOnlySpanCharExtensions signatures; dotnet fantomas
fixes it mechanically, no signature changes. check_release_notes also
requires an entry for changes under vsintegration/src.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Plain_Build_Windows and Plain_Build_Linux both failed with NU1510: on
the .NET Core inner build System.Memory ships with the framework, and
NuGet's package-pruning check treats an unconditional explicit
PackageReference to it as an error. The pin is only needed on net472,
where FSharp.Compiler.Service's own PackageReference to System.Memory
doesn't flow through the netstandard2.0 SetTargetFramework override.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Wrap commentTypeOf's doc comment in <summary>, move the FS0412
rationale into <remarks>, and reference the types through <see cref>
rather than inline code spans. Use the shorthand lambda for the
whitespace check, per review suggestion.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Slicing before searching makes the result relative to the slice, while
the String siblings these mirror return an index into the whole string.
A call ported from the string path would land a column short by
startIndex, and "not found" would come back as -1 from the slice rather
than from the string. Nothing calls them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Restores the startIndex overloads dropped in c9fbf5a, this time
reporting the position in the span they were given rather than in the
slice they searched, which is what the String siblings they mirror
return. A miss still comes back as -1 rather than as startIndex - 1.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A doc comment that carries markup like <see>/<paramref> needs that text
inside <summary> - otherwise it renders as raw text in the generated
XML and in tooltips.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
xperiandri and others added 2 commits September 11, 2026 17:56
getCommentRanges recurses once per line, threading a three-way state
through every call; a reference tuple heap-allocates on each of those
recursive calls, a struct tuple doesn't.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Changing the signature made every FSharp.Compiler.Service consumer of the
outlining API pay for a caller the editor alone has. The scanner now sits
behind two entry points: the public one keeps its string[] parameter and
maps to memory, and getOutliningRangesFromLineSlices takes the slices the
editor already holds.

The public surface is unchanged, so the surface-area baseline and the
StructureTests calls return to what they were, and the release note moves
out of Breaking Changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri
xperiandri force-pushed the outlining-line-slices branch from ac31157 to 9546828 Compare September 11, 2026 16:20
@github-actions github-actions Bot added the ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager label Sep 11, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

xperiandri added a commit to xperiandri/fsharp that referenced this pull request Sep 11, 2026
Reverts the copies of dotnet#20443 that came in with an earlier rebase: "Slice
source text instead of copying it line by line for outlining" and "Track
comment lines by number instead of storing their text". They changed the
public FSharp.Compiler.Service surface (Structure.getOutliningRanges took
ReadOnlyMemory<char>[]) and added a System.Memory reference that fails
restore with NU1510 - neither belongs here, and dotnet#20443 carries that work
on its own.

The Copilot snippets go back to the string[] lines Structure.getOutliningRanges
takes on main. Everything outside the Copilot files is now as on main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Design-Time
Affects-Design-Time: Changes outlining services used by Visual Studio.

Generated by PR Tooling Safety Check · gpt56 3.1M ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ Affects-Build-Infra Tooling check: PR touches build infrastructure ⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager ⚠️ Affects-Restore Tooling check: PR touches NuGet packages or feeds

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

2 participants