Improve PaxTarEntry construction performance - #132013
Conversation
Capture known PAX attributes while validating and inserting them to avoid repeated dictionary lookups during entry construction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 487aab98-d3a1-4f93-b2a0-97ee2950c4cf
|
@EgorBot -linux_arm64 -linux_amd using System.Collections.Generic;
using System.Formats.Tar;
using System.IO;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(Perf_TarWriter).Assembly).Run(args);
public class Perf_TarWriter
{
private static readonly string s_fileName = "file.txt";
private Dictionary<string, string> _extendedAttributes = null!;
private MemoryStream _memoryStream = null!;
[GlobalSetup]
public void Setup()
{
_memoryStream = new MemoryStream();
_extendedAttributes = new Dictionary<string, string>
{
{ "uname", "username" },
{ "gname", "groupname" },
{ "uid", "483745" },
{ "gid", "193783" },
{ "mtime", "1409547224" },
{ "ctime", "1409547225" },
{ "atime", "1409547226" },
{ "MSWINDOWS.rawsd", "AQAAgBQAAAAkAAAAAAAAAAAAAAABAgAAAAAABSAAAAAhAgAAAQIAAAAAAAUgAAAAIQIAAA==" }
};
}
[GlobalCleanup]
public void Cleanup() => _memoryStream.Dispose();
[Benchmark]
public void PaxTarEntry_WriteEntry()
{
PaxTarEntry entry = new(TarEntryType.RegularFile, s_fileName, _extendedAttributes);
_memoryStream.Position = 0;
using TarWriter writer = new(_memoryStream, leaveOpen: true);
writer.WriteEntry(entry);
}
}Note This benchmark request was generated with GitHub Copilot. |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-formats-tar |
There was a problem hiding this comment.
Pull request overview
This PR refactors PAX extended-attribute processing to reduce overhead during PaxTarEntry construction by capturing frequently-used attributes during the initial validation/insertion pass, then applying them to standard header fields without repeated dictionary lookups.
Changes:
- Refactors
TarHeader.ReplaceNormalAttributesWithExtendedto validate + insert extended attributes and capture known PAX/GNU-sparse attribute values in a single enumeration pass. - Adds
string?-based overloads inTarHelpersto parse timestamps / base-10 numbers without an intermediate dictionary lookup at call sites. - Extracts extended-attribute key/value validation into a shared helper (
ValidateExtendedAttribute) reused by both read-path and construction helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs | Adds string? overloads for timestamp and base-10 numeric parsing so callers can avoid extra dictionary probes. |
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs | Reworks extended-attribute application to capture known keys while inserting, reducing repeated TryGetValue calls when applying to standard fields. |
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs | Factors extended-attribute validation into a helper method and reuses it from both insertion paths. |
Suppressed comments (1)
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs:177
- Same as above: this comment says "valid" but the implementation uses long.Parse and will throw on invalid input. Update the comment so it doesn't imply validation/try-parse semantics.
// If the specified fieldName is found in the provided dictionary and is a valid string representation of a number, returns true and sets the value in 'baseTenLong'.
| _mode = mode; | ||
| } | ||
|
|
||
| // The 'size' header field only fits 12 bytes, so the data section length that surpases that limit needs to be retrieved |
| @@ -151,9 +157,16 @@ internal static string GetTimestampStringFromDateTimeOffset(DateTimeOffset times | |||
| // If the specified fieldName is found in the provided dictionary and is a valid string representation of a number, returns true and sets the value in 'baseTenInteger'. | |||
|
Missing test: no case constructs PaxTarEntry from a duplicate-key List to verify the new single-pass loop in ReplaceNormalAttributesWithExtended still throws correctly (existing tests use Dictionary, which can't hold duplicates). |
Summary
Fixes #126154.
Performance
Local Windows x64 BenchmarkDotNet results:
PaxTarEntryconstructionAllocations were unchanged. The full
PaxTarEntry_WriteEntrybenchmark was statistically unchanged on the local Windows machine because writer cost and measurement variance dominate the recovered ~227 ns constructor cost. EgorBot is being requested for cross-platform validation of the tracked benchmark.Validation
dotnet.cmd build src\libraries\System.Formats.Tar\src\System.Formats.Tar.csproj -c ReleasePaxTarEntry_ExtendedAttributes_Tests: 17 passedTarReader_SparseFileTests: 60 passedNote
This pull request description was generated with GitHub Copilot.