Restore synchronous TarWriter fast path - #131993
Conversation
Restore the Span-based synchronous write implementation removed by the sync/async adapter refactor. This keeps header buffers and data padding on the stack and avoids routing synchronous WriteEntry calls through ArrayPool, Memory<byte>, generic adapters, and an async ValueTask state machine. The adapter-based asynchronous write path remains unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00ba5cd3-e0c2-4b68-83b7-4e2d099813bd
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR changes the synchronous TarWriter.WriteEntry(TarEntry) path to avoid going through the async WriteEntryCoreAsync<SyncReadWriteAdapter> pipeline, aiming to eliminate overhead that can show up in microbenchmarks (as referenced by the linked perf regression issue).
Changes:
- Replaces the sync
WriteEntry(TarEntry)implementation with a dedicated synchronousWriteEntryInternalthat usesstackallocand direct header-write methods. - Adds synchronous
TarHeader.WriteAs*entry-writing helpers (and shared helpers for seekable/unseekable data streams, data copy, and padding) to mirror the async flow without adapters/state machines.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.cs | Routes sync WriteEntry(TarEntry) to a new synchronous internal implementation. |
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs | Introduces synchronous header/data writing helpers parallel to existing async implementations. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
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. |
alinpahontu2912
left a comment
There was a problem hiding this comment.
I think it's fine, but I wonder what is better long-term. The perf regression was caused by the stackallocs that can't be used in async contexts so the adapter that was used for deduplication couldn't use them. @rzikm what do you think?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs:279
- In the PAX + unseekable data stream path,
_dataOffsetis set while writing intotempStream(so it becomes relative to the temp stream, e.g., 512), but it is never updated to the final offset inarchiveStreambefore copying. This makesTarEntry.DataOffsetincorrect afterTarWriter.WriteEntrywhenarchiveStreamis seekable andDataStream.CanSeek == false.
Update _dataOffset based on the current archiveStream.Position (start of the entry header) before copying tempStream into the archive (and consider mirroring the same fix in WriteAsPaxCoreAsync for parity).
CollectExtendedAttributesFromStandardFieldsIfNeeded();
extendedAttributesHeader.WriteAsPaxExtendedAttributes(archiveStream, buffer, ExtendedAttributes, isGea: false, globalExtendedAttributesEntryNumber: -1);
buffer.Clear();
tempStream.CopyTo(archiveStream);
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs:796
WriteEmptyPaddingalways stackallocs a full 512-byte buffer and clears it, even when the required padding is much smaller. This does extra stack usage and memory clearing work per entry.
Since paddingAfterData is already bounded to <= TarHelpers.RecordSize, stackalloc only the needed length.
Span<byte> zeros = stackalloc byte[TarHelpers.RecordSize];
zeros = zeros.Slice(0, paddingAfterData);
zeros.Clear();
archiveStream.Write(zeros);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs:797
- WriteEmptyPadding always stackallocs a full 512-byte buffer even though padding is at most 511 bytes (and often much smaller). This adds avoidable stack pressure in a hot path, especially now that the sync fast path also stackallocs a 512-byte header buffer per entry.
Consider stackalloc’ing exactly the required padding length instead.
private void WriteEmptyPadding(Stream archiveStream)
{
int paddingAfterData = TarHelpers.CalculatePadding(_size);
if (paddingAfterData != 0)
{
Debug.Assert(paddingAfterData <= TarHelpers.RecordSize);
Span<byte> zeros = stackalloc byte[TarHelpers.RecordSize];
zeros = zeros.Slice(0, paddingAfterData);
zeros.Clear();
archiveStream.Write(zeros);
}
Restores the stack-allocated, span-based synchronous write path removed by #129282. The adapter-based asynchronous implementation remains unchanged.
Fixes #130041