-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[StaticWebAssets] Process collection properties with amortized allocation #49682
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.
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for your PR, @@javiercn. |
3ab44b5
to
bc59fb1
Compare
38a718b
to
0bbc810
Compare
…tions This change significantly improves performance of static web asset endpoint processing by optimizing JSON serialization and deserialization operations: Key optimizations: - Added reusable List<T> collections to eliminate repeated allocations - Implemented JsonWriterContext for efficient JSON serialization with buffer reuse - Added string interning for well-known header names, selector names, and property values - Introduced direct string property setters to bypass expensive array recreations - Used ArrayPool and stack allocation for UTF-8 encoding buffers - Added optimized PopulateFromMetadataValue methods that populate existing lists Performance improvements (typical): - FromMetadataValue operations: 40-60% faster, 80-90% less memory allocation - ToMetadataValue operations: 5-17% faster with same memory usage - StaticWebAssetEndpointProperty: 56% faster deserialization, 17% faster serialization - StaticWebAssetEndpointResponseHeader: 56% faster deserialization, 15% faster serialization - StaticWebAssetEndpointSelector: 60% faster deserialization, 10% faster serialization These optimizations reduce build-time overhead when processing large numbers of static web assets in ASP.NET Core applications.
0bbc810
to
67f679f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces optimized collection processing for StaticWebAssets serialization and deserialization to reduce memory allocations and improve performance. The changes add new methods for populating existing collections and reuse well-known values during serialization/deserialization.
Key Changes:
- Introduces
PopulateFromMetadataValue
methods that reuse existing collections instead of creating new arrays - Adds string interning for well-known header names, values, selector names, and property names
- Implements pooled JSON writers and array buffer writers for reduced allocations
Reviewed Changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/StaticWebAssetsSdk/Tasks/Data/StaticWebAssetEndpointProperty.cs | Adds PopulateFromMetadataValue method and optimized serialization with string interning |
src/StaticWebAssetsSdk/Tasks/Data/StaticWebAssetEndpointSelector.cs | Adds PopulateFromMetadataValue method and optimized serialization for selectors |
src/StaticWebAssetsSdk/Tasks/Data/StaticWebAssetEndpointResponseHeader.cs | Adds PopulateFromMetadataValue method and optimized serialization for response headers |
src/StaticWebAssetsSdk/Tasks/Data/WellKnown*.cs | New classes containing interned strings for common values to reduce allocations |
src/StaticWebAssetsSdk/Tasks/Utils/JsonWriterContext.cs | Reusable JSON writing context with pooled buffers |
test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/*Test.cs | Comprehensive test coverage for new functionality |
src/StaticWebAssetsSdk/benchmarks/*.cs | Benchmark classes to measure performance improvements |
src/StaticWebAssetsSdk/Tasks/Data/StaticWebAssetEndpointProperty.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
var result = JsonSerializer.Deserialize(value, _jsonTypeInfo); | ||
Array.Sort(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the sort here? Is the sortedness of the endpoint properties utilized anywhere or is it just for the sake of having a predictable and deterministic ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Predictable deterministic ordering for when we write them into files.
src/StaticWebAssetsSdk/Tasks/Data/StaticWebAssetEndpointProperty.cs
Outdated
Show resolved
Hide resolved
// Expect to be positioned at start of array | ||
if (reader.TokenType != JsonTokenType.StartArray) | ||
{ | ||
reader.Read(); // Move to start array if not already there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert here that the token type is now JsonTokenType.StartArray
after reading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do Debug.Assert
here, but that won't help customers as it will be stripped out on Release
builds. In general, this is trusted data that we read and write, so we can expect that it's in the right shape.
} | ||
} | ||
|
||
public static void PopulateFromMetadataValue(ref Utf8JsonReader reader, List<StaticWebAssetEndpointProperty> properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the performance gains from this method primarily due to the fact that we're reusing the properties
list to reduce allocations? Or is it that the STJ source generator doesn't generally produce code as optimized/tailored code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We save in a bunch of places:
- Decoding properties: We skip because we map the utf8 bits to the well-known strings.
- Allocations: We reuse the strings everywhere, which makes string comparisons O(1) instead of O(N) (Because equal strings share the same memory address).
- Perfect hashing: We don't have to check things on a dictionary or write an list of
if
s we can just check for well-known stuff way more efficiently.
return JsonSerializer.Serialize(properties, _jsonTypeInfo); | ||
} | ||
|
||
internal static string ToMetadataValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this yields up to a 17% perf improvement on microbenchmarks. Do you have measurements or predictions on what the impact of this optimization would be on a typical build? Just want to have an idea on what the tradeoff is between performance and maintainability here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens on the main loop of a bunch of Tasks, like DefineStaticWebAssetEndpoints, ApplyCompressionNegotiation, GenerateStaticWebAssetsManifest that happen on incremental builds.
It's hard to predict the results here until we have merged it as we can't compare it against a released SDK (It has been PGOd, which gives it an additional boost).
But in general, anything where we can remove allocations and speed up items going through the main loop should be worth it.
As for maintainability, I had the same concern, but with Copilot it is actually much more straightforward to maintain this code. We can if we want to, add explicit instructions to ensure that it updates these paths.
internal static JsonWriterContext CreateWriter() | ||
{ | ||
var context = new JsonWriterContext(); | ||
return context; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this helper necessary? Can't the caller just new-up their own JsonWriterContext()
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new-ing up a JsonWritterContext is expensive, so it's worth caching
/// <returns>The interned header value or null if not well-known</returns> | ||
public static string TryGetInternedHeaderValue(ReadOnlySpan<byte> headerValueSpan) | ||
{ | ||
return headerValueSpan.Length switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could be a good application for generated code (e.g., using a T4 template). That way this wouldn't all have to be hard-coded. Although I don't see that kind of thing happening already in this repo. This is good as-is, just a passing thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually used copilot instead to generate this. Essentially asked it:
- Write a minimal perfect hash for matching against these values.
This is essentially faster than having a dictionary and hashing over the string.
var internedName = WellKnownEndpointPropertyNames.TryGetInternedPropertyName(reader.ValueSpan); | ||
property.Name = internedName ?? reader.GetString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the interning happens
Address feedback Co-authored-by: Copilot <[email protected]>
Performance Results by Component
1. StaticWebAssetEndpointProperty Benchmarks
.NET 10.0 Results
.NET Framework 4.8.1 Results
2. StaticWebAssetEndpointResponseHeader Benchmarks
.NET 10.0 Results
.NET Framework 4.8.1 Results
3. StaticWebAssetEndpointSelector Benchmarks
.NET 10.0 Results
.NET Framework 4.8.1 Results