Skip to content

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

Open
xperiandri wants to merge 6 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 6 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.

getOutliningRanges now takes ReadOnlyMemory<char>[] and the editor slices the already materialized source text once through a new SourceText.GetLinesAsMemory() helper. Inside the function, 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.

This is a breaking change for FSharp.Compiler.Service consumers that call getOutliningRanges with a string[]; the migration is Array.map (fun line -> line.AsMemory()).

Fixes # (no issue)

Checklist

  • Test cases added: the existing StructureTests (39 cases) exercise every fold kind through the new signature; the public surface area baseline is updated.
  • 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, Breaking Changes.

🤖 Generated with Claude Code

xperiandri and others added 2 commits September 3, 2026 22:38
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>
@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

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
xperiandri and others added 2 commits September 3, 2026 22:54
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>
@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
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>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Build-Infra, Affects-Restore
Affects-Build-Infra: adds PackageReference in test .fsproj
Affects-Restore: new System.Memory package ref changes restore graph

Generated by PR Tooling Safety Check · opus46 3.7M ·

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-Restore Tooling check: PR touches NuGet packages or feeds

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

1 participant