Skip to content

Comments

Eliminate closure allocation by referencing UriString property instead of captured uriString parameter#81948

Merged
jasonmalinowski merged 1 commit intodotnet:mainfrom
nareshjo:dev/nareshjo/DocumentUriAllocation
Jan 9, 2026
Merged

Eliminate closure allocation by referencing UriString property instead of captured uriString parameter#81948
jasonmalinowski merged 1 commit intodotnet:mainfrom
nareshjo:dev/nareshjo/DocumentUriAllocation

Conversation

@nareshjo
Copy link
Contributor

@nareshjo nareshjo commented Jan 9, 2026

This pull request was generated by the VS Perf Rel AI Agent. Please review this AI-generated PR with extra care! For more information, visit our wiki. Please share feedback with TIP Insights

  • Issue: The DocumentUri(string uriString) constructor allocates a closure/display class (<>c__DisplayClass1_0) on every invocation because the lambda () => ParseUri(uriString) captures the uriString parameter instead of using already-assigned instance property, causing unnecessary display class allocations.
    Evidence:
  1. Stack trace shows allocation type TypeAllocated!<>c__DisplayClass1_0 (compiler-generated closure)
  2. Source code at line 28 shows: _parsedUriLazy = new(() => ParseUri(uriString)); which captures the uriString parameter
  3. The UriString property is assigned before the Lazy<> creation, so we can use this.UriString instead to avoid parameter capture
TypeAllocated!<>c__DisplayClass1_0
clr.dll!JIT_New
microsoft.codeanalysis.languageserver.protocol.dll!Roslyn.LanguageServer.Protocol.DocumentUri..ctor
microsoft.codeanalysis.languageserver.protocol.dll!Roslyn.LanguageServer.Protocol.DocumentUriConverter.Read
system.text.json.dll!System.Text.Json.Serialization.JsonConverter`[System.__Canon].TryRead
  • Issue type: AVOID creating lambdas that capture variables on hot path

  • Proposed fix: Change the lambda in DocumentUri(string uriString) constructor to reference the instance property UriString instead of the captured parameter uriString. UriString is a get-only auto-property set in the constructor and never mutated afterward. So this is a minimal and safe fix that eliminates the display class allocation while maintaining identical behavior.

Best practices wiki
See related failure in PRISM
ADO work item

…d of captured uriString parameter in DocumentUri ctor
@nareshjo nareshjo requested a review from a team as a code owner January 9, 2026 16:49
@dotnet-policy-service dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Jan 9, 2026
@jasonmalinowski jasonmalinowski merged commit 83d9ab5 into dotnet:main Jan 9, 2026
27 checks passed
@CyrusNajmabadi
Copy link
Contributor

Won't the new code allocate a delegate too so as to capture this?

@CyrusNajmabadi
Copy link
Contributor

I don't really see why we need a lazy at all. We already store the string. Just have another Nullable string that is lazily computed if null when accessed.

@jasonmalinowski
Copy link
Member

@CyrusNajmabadi We'll allocate the delegate regardless, but we won't need a separate closure to capture the local.

@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jan 9, 2026
@CyrusNajmabadi
Copy link
Contributor

I'd rather us move to no-alloc.

@nareshjo nareshjo deleted the dev/nareshjo/DocumentUriAllocation branch January 9, 2026 20:04
@jasonmalinowski
Copy link
Member

@CyrusNajmabadi Go right ahead! I'll sign off on that.

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

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants