Skip to content

Cap the amount of text copied into the stringbuilder in JsonRpcEventSource.Format#1187

Merged
AArnott merged 1 commit intomicrosoft:mainfrom
ToddGrun:dev/toddgrun/CapStringBuilderAllocationSizeInJsonRpcEventSource.Format
May 7, 2025
Merged

Cap the amount of text copied into the stringbuilder in JsonRpcEventSource.Format#1187
AArnott merged 1 commit intomicrosoft:mainfrom
ToddGrun:dev/toddgrun/CapStringBuilderAllocationSizeInJsonRpcEventSource.Format

Conversation

@ToddGrun
Copy link
Copy Markdown
Member

@ToddGrun ToddGrun commented May 7, 2025

Speedometer test shows two LOH allocations occurring in this method when opening a large cs file, when the value passed into JsonRpcEventSource.Format is a JsonDocument.

  1. The ToString call on the document
  2. The StringBuilder.Append call using the ToString result

It would be non-trivial to get rid of the first allocation, however, the second can be removed by just calling the StringBuilder.Append overload that takes in a range from the given string to copy into the StringBuilder.

Here's is the allocation from the speedometer test that this is attempting to improve:

image

…ource.Format

Speedometer test shows two LOH allocations occurring in this method when opening a large cs file, when the value passed into the method is a JsonDocument.

1) The ToString call on the document
2) pushing the results of the ToString call into the StringBuilder

It would be non-trivial to get rid of the first allocation, however, the second can be removed by just calling the StringBuilder.AppendOverload that takes in a range from the given string to copy into the StringBuilder.
Copy link
Copy Markdown
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks.

I don't anticipate a runtime perf benefit because this code only runs when an ETW trace is collected that specifies the streamjsonrpc ETW provider explicitly. Still, it may make our perf runs a little faster and a little more 'true' to customer perception.

@AArnott
Copy link
Copy Markdown
Member

AArnott commented May 7, 2025

/azp run

@AArnott AArnott enabled auto-merge May 7, 2025 22:19
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants