Skip to content

Restore synchronous TarWriter fast path - #131993

Open
iremyux wants to merge 5 commits into
dotnet:mainfrom
iremyux:writeentry-perf-regression-ff08290b
Open

Restore synchronous TarWriter fast path#131993
iremyux wants to merge 5 commits into
dotnet:mainfrom
iremyux:writeentry-perf-regression-ff08290b

Conversation

@iremyux

@iremyux iremyux commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Restores the stack-allocated, span-based synchronous write path removed by  #129282. The adapter-based asynchronous implementation remains unchanged.

Fixes #130041

iremyux and others added 2 commits August 4, 2026 19:24
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
Copilot AI review requested due to automatic review settings August 7, 2026 11:58
@azure-pipelines

Copy link
Copy Markdown
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.

@iremyux iremyux changed the title Writeentry perf regression ff08290b Restore synchronous TarWriter fast path Aug 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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 synchronous WriteEntryInternal that uses stackalloc and 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>
Copilot AI review requested due to automatic review settings August 7, 2026 12:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@iremyux
iremyux marked this pull request as ready for review August 7, 2026 13:34
@azure-pipelines

Copy link
Copy Markdown
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.

@iremyux
iremyux requested a review from alinpahontu2912 August 7, 2026 13:36
@iremyux iremyux added this to the 11.0.0 milestone Aug 7, 2026

@alinpahontu2912 alinpahontu2912 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Comment thread src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.cs Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 15:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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, _dataOffset is set while writing into tempStream (so it becomes relative to the temp stream, e.g., 512), but it is never updated to the final offset in archiveStream before copying. This makes TarEntry.DataOffset incorrect after TarWriter.WriteEntry when archiveStream is seekable and DataStream.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

  • WriteEmptyPadding always 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);
            }

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
            }

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Perf] Linux/x64: 4 Regressions on 6/29/2026 9:51:56 AM +00:00

3 participants