Skip to content
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

Improve performance of Tar library #74281

Merged
merged 21 commits into from
Aug 21, 2022
Merged

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Aug 19, 2022

Low-hanging fruit.

Method Toolchain format Mean Ratio Code Size Allocated Alloc Ratio
Roundtrip \main\corerun.exe Ustar 12.630 us 1.00 11,006 B 17.1 KB 1.00
Roundtrip \pr\corerun.exe Ustar 8.266 us 0.65 7,526 B 5.95 KB 0.35
Roundtrip \main\corerun.exe Pax 51.859 us 1.00 15,875 B 83.99 KB 1.00
Roundtrip \pr\corerun.exe Pax 31.182 us 0.60 11,707 B 31.62 KB 0.38
Roundtrip \main\corerun.exe Gnu 14.264 us 1.00 13,310 B 19.76 KB 1.00
Roundtrip \pr\corerun.exe Gnu 8.183 us 0.57 8,389 B 5.95 KB 0.30
private MemoryStream _archive = new MemoryStream();
private string[] _names;
private MemoryStream[] _streams;

[GlobalSetup]
public void Setup()
{
    _names = Enumerable.Range(0, 10).Select(i => $"HelloWorld{i}.txt").ToArray();
    _streams = _names.Select(s => new MemoryStream(Encoding.UTF8.GetBytes(s))).ToArray();
}

[Benchmark]
[Arguments(TarEntryFormat.Pax)]
[Arguments(TarEntryFormat.Gnu)]
[Arguments(TarEntryFormat.Ustar)]
public void Roundtrip(TarEntryFormat format)
{
    using (TarWriter writer = new TarWriter(_archive, leaveOpen: true))
    {
        for (int i = 0; i < _names.Length; i++)
        {
            _streams[i].Position = 0;
            TarEntry entry = format switch
            {
                TarEntryFormat.Pax => new PaxTarEntry(TarEntryType.RegularFile, _names[i]) { DataStream = _streams[i] },
                TarEntryFormat.Gnu => new GnuTarEntry(TarEntryType.RegularFile, _names[i]) { DataStream = _streams[i] },
                _ => new UstarTarEntry(TarEntryType.RegularFile, _names[i]) { DataStream = _streams[i] },
            };
            writer.WriteEntry(entry);
        }
    }

    _archive.Position = 0;

    using (TarReader reader = new TarReader(_archive, leaveOpen: true))
    {
        TarEntry entry;
        while ((entry = reader.GetNextEntry()) != null)
        {
            entry.DataStream?.CopyTo(Stream.Null);
        }
    }
}

@stephentoub stephentoub added the tenet-performance Performance related issue label Aug 19, 2022
@stephentoub stephentoub added this to the 7.0.0 milestone Aug 19, 2022
@ghost ghost assigned stephentoub Aug 19, 2022
@ghost
Copy link

ghost commented Aug 19, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Low-hanging fruit.

Method Toolchain format Mean Ratio Code Size Allocated Alloc Ratio
Roundtrip \main\corerun.exe Ustar 13.108 us 1.00 11,006 B 17.1 KB 1.00
Roundtrip \pr\corerun.exe Ustar 8.293 us 0.63 7,526 B 6.34 KB 0.37
Roundtrip \main\corerun.exe Pax 51.878 us 1.00 15,772 B 84.12 KB 1.00
Roundtrip \pr\corerun.exe Pax 37.418 us 0.72 11,692 B 45.12 KB 0.54
Roundtrip \main\corerun.exe Gnu 14.515 us 1.00 13,310 B 19.76 KB 1.00
Roundtrip \pr\corerun.exe Gnu 8.004 us 0.55 8,389 B 5.95 KB 0.30
private MemoryStream _archive = new MemoryStream();
private string[] _names;
private MemoryStream[] _streams;

[GlobalSetup]
public void Setup()
{
    _names = Enumerable.Range(0, 10).Select(i => $"HelloWorld{i}.txt").ToArray();
    _streams = _names.Select(s => new MemoryStream(Encoding.UTF8.GetBytes(s))).ToArray();
}

[Benchmark]
[Arguments(TarEntryFormat.Pax)]
[Arguments(TarEntryFormat.Gnu)]
[Arguments(TarEntryFormat.Ustar)]
public void Roundtrip(TarEntryFormat format)
{
    using (TarWriter writer = new TarWriter(_archive, leaveOpen: true))
    {
        for (int i = 0; i < _names.Length; i++)
        {
            _streams[i].Position = 0;
            TarEntry entry = format switch
            {
                TarEntryFormat.Pax => new PaxTarEntry(TarEntryType.RegularFile, _names[i]) { DataStream = _streams[i] },
                TarEntryFormat.Gnu => new GnuTarEntry(TarEntryType.RegularFile, _names[i]) { DataStream = _streams[i] },
                _ => new UstarTarEntry(TarEntryType.RegularFile, _names[i]) { DataStream = _streams[i] },
            };
            writer.WriteEntry(entry);
        }
    }

    _archive.Position = 0;

    using (TarReader reader = new TarReader(_archive, leaveOpen: true))
    {
        TarEntry entry;
        while ((entry = reader.GetNextEntry()) != null)
        {
            entry.DataStream?.CopyTo(Stream.Null);
        }
    }
}
Author: stephentoub
Assignees: stephentoub
Labels:

area-System.IO, tenet-performance

Milestone: 7.0.0

// The checksum accumulator first adds up the byte values of eight space chars, then the final number
// is written on top of those spaces on the specified span as ascii.
// At the end, it's saved in the header field and the final value returned.
internal int WriteChecksum(int checksum, Span<byte> buffer)
internal static int WriteChecksum(int checksum, Span<byte> buffer)
Copy link
Member Author

@stephentoub stephentoub Aug 20, 2022

Choose a reason for hiding this comment

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

@carlossanlop, even before my changes, is this method functionally correct? The input and output spans are the same length, but the output span has two characters at the end reserved, so are we frequently losing digits from the checksum of the checksum is large enough?

(I didn't want to mess with the logic if it was already buggy, but this should be changed to just span.CopyTo rather than an open-coded loop.)

Copy link
Member

Choose a reason for hiding this comment

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

I'd have to investigate this with a header whose fields have ascii characters with the highest possible values, so that when they get added up for the checksum, its final value would go beyond the checksum field length, excluding the two reserved characters at the end.

return string.Format(PaxHeadersFormat, dirName, processId, fileName, trailingSeparator);
return _typeFlag is TarEntryType.Directory or TarEntryType.DirectoryList ?
$"{dirName}/PaxHeaders.{Environment.ProcessId}/{fileName}" :
$"{dirName}/PaxHeaders.{Environment.ProcessId}/{fileName}{Path.DirectorySeparatorChar}";
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. this condition is reversed... but no tests are failing as a result. I'll fix the inversion, but it seems like a test gap that should be addressed.

Copy link
Member

Choose a reason for hiding this comment

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

I can add a test for that.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thank you so much for your help, @stephentoub. No blocking comments from me. I'll address your question to me separately.

The CI passed so I'll merge it so I can submit this as an RC1 backport.

// The checksum accumulator first adds up the byte values of eight space chars, then the final number
// is written on top of those spaces on the specified span as ascii.
// At the end, it's saved in the header field and the final value returned.
internal int WriteChecksum(int checksum, Span<byte> buffer)
internal static int WriteChecksum(int checksum, Span<byte> buffer)
Copy link
Member

Choose a reason for hiding this comment

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

I'd have to investigate this with a header whose fields have ascii characters with the highest possible values, so that when they get added up for the checksum, its final value would go beyond the checksum field length, excluding the two reserved characters at the end.

@carlossanlop carlossanlop merged commit ca6bbf1 into dotnet:main Aug 21, 2022
@carlossanlop
Copy link
Member

/backport to release/7.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2897330001

// "XX attribute=value\n"
// where "XX" is the number of characters in the entry, including those required for the count itself.
int length = 3 + Encoding.UTF8.GetByteCount(attribute) + Encoding.UTF8.GetByteCount(value);
length += CountDigits(length);
Copy link
Member Author

@stephentoub stephentoub Aug 21, 2022

Choose a reason for hiding this comment

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

This was meant to model the logic previously there, but I think this logic needs to be tweaked, and it should be more like:

int digitCount = CountDigits(length) ;
length += digitCount;
length += CountDigits(length) - digitCount; // account for possible digit length increase

Do we have tests for this stuff? Do we validate that the archives at produce are readable by other tools? Our own code appears to ignore this length when reading archives (maybe it shouldn't?)

Copy link
Member Author

@stephentoub stephentoub Aug 21, 2022

Choose a reason for hiding this comment

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

Are we even sure the length is supposed to include itself? Have you seen that being done in archives produced by other tools? That would be a very strange format design.

Copy link
Member

@carlossanlop carlossanlop Aug 22, 2022

Choose a reason for hiding this comment

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

From man tar 5:

The extended attributes themselves are stored as a series of text-format lines encoded in the portable UTF-8 encoding. Each line consists of a decimal number, a space, a key string, an equals sign, a value string, and a new line. The decimal number indicates the length of the entire line, including the initial length field and the trailing newline. An example of such a field is:

25 ctime=1084839148.1212\n

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's strange design.

I did verify that the tar tool was able to read our archives containing extended attributes entries.

Since we know the length of the data section, I didn't feel it was too important to verify that the length of each extended attribute entry was correct, considering that we need to look for a mandatory newline char suffix. Maybe if we checked the length number and advanced the position instead of searching for the newline, we could improve perf a bit. I can investigate if this is true and determine if we need to submit a PR to verify the length number.

for (int i = 0; i < dest.Length; i++)
{
char ch = dest[i];
if (ch == Path.DirectorySeparatorChar || ch == Path.AltDirectorySeparatorChar)
Copy link
Contributor

Choose a reason for hiding this comment

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

If on Unix both these constants are /

internal const char DirectorySeparatorChar = '/';
internal const char AltDirectorySeparatorChar = '/';

we could skip the cycle on Unix. On Windows we could do only one check \ in the cycle.

Copy link
Member

Choose a reason for hiding this comment

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

This code is the same on both platforms.

@EgorBo just out of curiosity, would the JIT in theory be able to legally collapse such a thing? ie, if (ch == '/' || ch == '/') ..

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is the same on both platforms.

It makes no sense to replace / with / on Unix.

Copy link
Member

Choose a reason for hiding this comment

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

I get that, but what are you proposing -- duplicate this method for Unix and Windows so they can be different? Is this code path that hot?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not super hot, but it can be improved. I need to push up a fix anyway, so I'll do so.

Comment on lines +744 to +750
while (true)
{
digits[i] = (byte)('0' + (remaining % 8));
remaining /= 8;
if (remaining == 0) break;
i--;
}
Copy link
Contributor

@iSazonov iSazonov Aug 22, 2022

Choose a reason for hiding this comment

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

SharpLab show that there are no optimizations with shifts as I'd expect. So why not unsafe convert the long to span[8]?

carlossanlop pushed a commit to carlossanlop/runtime that referenced this pull request Aug 23, 2022
* Avoid unnecessary byte[] allocations

* Remove unnecessary use of FileStreamOptions

* Clean up Dispose{Async} implementations

* Clean up unnecessary consts

Not a perf thing, just readability.

* Remove MemoryStream/Encoding.UTF8.GetBytes allocations, unnecessary async variants, and overhaul GenerateExtendedAttributesDataStream

* Avoid string allocations in ReadMagicAttribute

* Avoid allocation in WriteAsOctal

* Improve handling of octal

* Avoid allocation for version string

* Removing boxing and char string allocation in GenerateExtendedAttributeName

* Fix a couple unnecessary dictionary lookups

* Replace Enum.HasFlag usage

* Remove allocations from Write{Posix}Name

* Replace ArrayPool use with string.Create

* Replace more superfluous ArrayPool usage

* Remove ArrayPool use from System.IO.Compression.ZipFile

* Fix inverted condition

* Use generic math to parse octal

* Remove allocations from StringReader and string.Split

* Remove magic string allocation for Ustar when not V7

* Remove file name and directory name allocation in GenerateExtendedAttributeName
carlossanlop pushed a commit to carlossanlop/runtime that referenced this pull request Aug 23, 2022
* Avoid unnecessary byte[] allocations

* Remove unnecessary use of FileStreamOptions

* Clean up Dispose{Async} implementations

* Clean up unnecessary consts

Not a perf thing, just readability.

* Remove MemoryStream/Encoding.UTF8.GetBytes allocations, unnecessary async variants, and overhaul GenerateExtendedAttributesDataStream

* Avoid string allocations in ReadMagicAttribute

* Avoid allocation in WriteAsOctal

* Improve handling of octal

* Avoid allocation for version string

* Removing boxing and char string allocation in GenerateExtendedAttributeName

* Fix a couple unnecessary dictionary lookups

* Replace Enum.HasFlag usage

* Remove allocations from Write{Posix}Name

* Replace ArrayPool use with string.Create

* Replace more superfluous ArrayPool usage

* Remove ArrayPool use from System.IO.Compression.ZipFile

* Fix inverted condition

* Use generic math to parse octal

* Remove allocations from StringReader and string.Split

* Remove magic string allocation for Ustar when not V7

* Remove file name and directory name allocation in GenerateExtendedAttributeName
carlossanlop added a commit that referenced this pull request Aug 24, 2022
* Improve performance of Tar library (#74281)

* Avoid unnecessary byte[] allocations

* Remove unnecessary use of FileStreamOptions

* Clean up Dispose{Async} implementations

* Clean up unnecessary consts

Not a perf thing, just readability.

* Remove MemoryStream/Encoding.UTF8.GetBytes allocations, unnecessary async variants, and overhaul GenerateExtendedAttributesDataStream

* Avoid string allocations in ReadMagicAttribute

* Avoid allocation in WriteAsOctal

* Improve handling of octal

* Avoid allocation for version string

* Removing boxing and char string allocation in GenerateExtendedAttributeName

* Fix a couple unnecessary dictionary lookups

* Replace Enum.HasFlag usage

* Remove allocations from Write{Posix}Name

* Replace ArrayPool use with string.Create

* Replace more superfluous ArrayPool usage

* Remove ArrayPool use from System.IO.Compression.ZipFile

* Fix inverted condition

* Use generic math to parse octal

* Remove allocations from StringReader and string.Split

* Remove magic string allocation for Ustar when not V7

* Remove file name and directory name allocation in GenerateExtendedAttributeName

* fix tar strings (#74321)

* Fix some corner cases in TarReader (#74329)

* Fix a few Tar issues post perf improvements (#74338)

* Fix a few Tar issues post perf improvements

* Update src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

* Skip directory symlink recursion on TarFile archive creation (#74376)

* Skip directory symlink recursion on TarFile archive creation

* Add symlink verification

* Address suggestions by danmoseley

Co-authored-by: carlossanlop <[email protected]>

* SkipBlockAlignmentPadding must be executed for all entries (#74396)

* Set modified timestamps on files being extracted from tar archives (#74400)

* Add tests for exotic external tar asset archives, fix some more corner case bugs (#74412)

* Remove unused _readFirstEntry. Remnant from before we created PaxGlobalExtendedAttributesEntry.

* Set the position of the freshly copied data stream to 0, so the first user access of the DataStream property gives them a stream ready to read from the beginning.

* Process a PAX actual entry's data block only after the extended attributes are analyzed, in case the size is found as an extended attribute and needs to be overriden.

* Add tests to verify the entries of the new external tar assets can be read. Verify their DataStream if available.

* Add copyData argument to recent alignment padding tests.

* Throw an exception sooner and with a clearer message when a data section is unexpected for the entry type.

* Allow trailing nulls and spaces in octal fields.
Co-authored-by: @am11 Adeel Mujahid <[email protected]>

* Throw a clearer exception if the unsupported sparse file entry type is encountered. These entries have additional data that indicates the locations of sparse bytes, which cannot be read with just the size field. So to avoid accidentally offseting the reader, we throw.

* Tests.

* Rename to TrimLeadingNullsAndSpaces

Co-authored-by: carlossanlop <[email protected]>

* Remove Compression changes, keep changes confined to Tar.

* Fix build failure due to missing using in TarHelpers.Windows

Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Dan Moseley <[email protected]>
Co-authored-by: Adeel Mujahid <[email protected]>
Co-authored-by: carlossanlop <[email protected]>
Co-authored-by: David Cantú <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants