[StaticWebAssets] Cleanup ApplyCompressionNegotiation.cs and ResolveCompressedAssets.cs #48082
Conversation
ghost
added
Area-AspNetCore
|
Thanks for your PR, @@javiercn. |
javiercn
changed the base branch from
javiercn/reuse-original-task-item-for-unmodified-assets-and-endpoints
to
main
javiercn
force-pushed
the
javiercn/more-cleanups
branch
from
4c1beb7 to
9e1de78
Compare
| @@ -115,16 +115,22 @@ public override bool Execute() | |||
| // Process the final set of candidate assets, deduplicating assets to be compressed in the same format multiple times and | |||
| // generating new a static web asset definition for each compressed item. | |||
| var formats = SplitPattern(Formats); | |||
| var assetsToCompress = new List<ITaskItem>(); | |||
| var assetsToCompress = new ITaskItem[matchingCandidateAssets.Count * formats.Length]; | |||
There was a problem hiding this comment.
Instead of creating a list and having to turn it into an array, we create an array of the max size we expect to hold and return that directly.
This takes advantage of the fact that MSBuild will ignore any item that is null when reading the outputs from a task, which save us from having to reallocate the list and convert it back into an array of contiguous elements
javiercn
changed the title
javiercn
changed the title
| @@ -141,6 +141,25 @@ public StaticWebAssetEndpointProperty[] EndpointProperties | |||
|
|
|||
| public static IEqualityComparer<StaticWebAssetEndpoint> RouteAndAssetComparer { get; } = new RouteAndAssetEqualityComparer(); | |||
|
|
|||
| internal static IDictionary<string, List<StaticWebAssetEndpoint>> ToAssetFileDictionary(ITaskItem[] candidateEndpoints) | |||
| { | |||
| var result = new Dictionary<string, List<StaticWebAssetEndpoint>>(candidateEndpoints.Length / 2); | |||
There was a problem hiding this comment.
Why the / 2? I'm guessing it's because we expect there to be multiple endpoints per asset file, but why 2 specifically? Do we know that will give us a conservative upper bound for the number of entries?
There was a problem hiding this comment.
Yes, this is because for a given file there is the fingerprinted endpoint and the non-fingerprinted endpoint. For compressed files there's also the versions that have the encoding selector, so when we are grouping them, we are going to have a few less than half of them.
|
|
||
| var preservedEndpoints = new Dictionary<(string, string), StaticWebAssetEndpoint>(); | ||
| var compressionHeadersByEncoding = new Dictionary<string, StaticWebAssetEndpointResponseHeader[]>(2); |
There was a problem hiding this comment.
Is 2 the expected maximum capacity? If so, might a dictionary be overkill instead of, e.g., a fixed-size buffer and linear search?
There was a problem hiding this comment.
That's fair, but the main goal here was to remove the allocations. We can do a further pass in the future if needed
| ], | ||
| EndpointProperties = relatedEndpointCandidate.EndpointProperties | ||
| }; | ||
| var headers = new List<StaticWebAssetEndpointResponseHeader>(7); |
There was a problem hiding this comment.
Why 7? How would we know when to update this in the future?
There was a problem hiding this comment.
It's the number of headers we add at most by default.
javiercn
force-pushed
the
javiercn/more-cleanups
branch
from
ac3e381 to
829e3e3
Compare
Uh oh!
There was an error while loading. Please reload this page.