From b5902a873688d547dd114be1ede034b946e825b5 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Sep 2023 13:29:29 +0900 Subject: [PATCH 1/6] Avoid `MemoryStream` overhead for incoming non-`MemoryStream` in `ImportTask` --- osu.Game/Database/ImportTask.cs | 38 ++++++++++++--------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/osu.Game/Database/ImportTask.cs b/osu.Game/Database/ImportTask.cs index def20bc1fb..bf5b377a07 100644 --- a/osu.Game/Database/ImportTask.cs +++ b/osu.Game/Database/ImportTask.cs @@ -46,9 +46,19 @@ namespace osu.Game.Database /// public ArchiveReader GetReader() { - return Stream != null - ? getReaderFrom(Stream) - : getReaderFrom(Path); + if (Stream == null) + return getReaderFromPath(Path); + + if (Stream is MemoryStream memoryStream) + { + if (ZipUtils.IsZipArchive(memoryStream)) + return new ZipArchiveReader(memoryStream, Path); + + return new LegacyByteArrayReader(memoryStream.ToArray(), Path); + } + + // This isn't used in any current path. May need to reconsider for performance reasons (ie. if we don't expect the incoming stream to be copied out). + return new LegacyByteArrayReader(Stream.ReadAllBytesToArray(), Path); } /// @@ -60,32 +70,12 @@ namespace osu.Game.Database File.Delete(Path); } - /// - /// Creates an from a stream. - /// - /// A seekable stream containing the archive content. - /// A reader giving access to the archive's content. - private ArchiveReader getReaderFrom(Stream stream) - { - if (!(stream is MemoryStream memoryStream)) - { - // This isn't used in any current path. May need to reconsider for performance reasons (ie. if we don't expect the incoming stream to be copied out). - memoryStream = new MemoryStream(stream.ReadAllBytesToArray()); - stream.Dispose(); - } - - if (ZipUtils.IsZipArchive(memoryStream)) - return new ZipArchiveReader(memoryStream, Path); - - return new LegacyByteArrayReader(memoryStream.ToArray(), Path); - } - /// /// Creates an from a valid storage path. /// /// A file or folder path resolving the archive content. /// A reader giving access to the archive's content. - private ArchiveReader getReaderFrom(string path) + private ArchiveReader getReaderFromPath(string path) { if (ZipUtils.IsZipArchive(path)) return new ZipArchiveReader(File.Open(path, FileMode.Open, FileAccess.Read, FileShare.Read), System.IO.Path.GetFileName(path)); From 0657b551964986fc5504a202eaa5e699d1c72f00 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Sep 2023 13:33:25 +0900 Subject: [PATCH 2/6] Avoid `MemoryStream.ToArray` overhead in `LegacyByteArrayReader` --- osu.Game/Database/ImportTask.cs | 2 +- .../IO/Archives/MemoryStreamArchiveReader.cs | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 osu.Game/IO/Archives/MemoryStreamArchiveReader.cs diff --git a/osu.Game/Database/ImportTask.cs b/osu.Game/Database/ImportTask.cs index bf5b377a07..962eab9fa5 100644 --- a/osu.Game/Database/ImportTask.cs +++ b/osu.Game/Database/ImportTask.cs @@ -54,7 +54,7 @@ namespace osu.Game.Database if (ZipUtils.IsZipArchive(memoryStream)) return new ZipArchiveReader(memoryStream, Path); - return new LegacyByteArrayReader(memoryStream.ToArray(), Path); + return new MemoryStreamArchiveReader(memoryStream, Path); } // This isn't used in any current path. May need to reconsider for performance reasons (ie. if we don't expect the incoming stream to be copied out). diff --git a/osu.Game/IO/Archives/MemoryStreamArchiveReader.cs b/osu.Game/IO/Archives/MemoryStreamArchiveReader.cs new file mode 100644 index 0000000000..37ce1e508e --- /dev/null +++ b/osu.Game/IO/Archives/MemoryStreamArchiveReader.cs @@ -0,0 +1,30 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Collections.Generic; +using System.IO; + +namespace osu.Game.IO.Archives +{ + /// + /// Allows reading a single file from the provided memory stream. + /// + public class MemoryStreamArchiveReader : ArchiveReader + { + private readonly MemoryStream stream; + + public MemoryStreamArchiveReader(MemoryStream stream, string filename) + : base(filename) + { + this.stream = stream; + } + + public override Stream GetStream(string name) => new MemoryStream(stream.GetBuffer(), 0, (int)stream.Length); + + public override void Dispose() + { + } + + public override IEnumerable Filenames => new[] { Name }; + } +} From 541cd972e1e19ba641b1e3397bb1b073d53121bc Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Sep 2023 13:36:07 +0900 Subject: [PATCH 3/6] Rename `ArchiveReader` implementations to read better --- osu.Game/Beatmaps/BeatmapImporter.cs | 2 +- osu.Game/Database/ImportTask.cs | 6 +++--- .../{LegacyByteArrayReader.cs => ByteArrayArchiveReader.cs} | 4 ++-- ...yDirectoryArchiveReader.cs => DirectoryArchiveReader.cs} | 6 +++--- ...egacyFileArchiveReader.cs => SingleFileArchiveReader.cs} | 6 +++--- osu.Game/Screens/Play/Player.cs | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) rename osu.Game/IO/Archives/{LegacyByteArrayReader.cs => ByteArrayArchiveReader.cs} (84%) rename osu.Game/IO/Archives/{LegacyDirectoryArchiveReader.cs => DirectoryArchiveReader.cs} (85%) rename osu.Game/IO/Archives/{LegacyFileArchiveReader.cs => SingleFileArchiveReader.cs} (83%) diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index 14719da1bc..b497b8e8dc 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -319,7 +319,7 @@ namespace osu.Game.Beatmaps { DateTimeOffset dateAdded = DateTimeOffset.UtcNow; - if (reader is LegacyDirectoryArchiveReader legacyReader) + if (reader is DirectoryArchiveReader legacyReader) { var beatmaps = reader.Filenames.Where(f => f.EndsWith(".osu", StringComparison.OrdinalIgnoreCase)); diff --git a/osu.Game/Database/ImportTask.cs b/osu.Game/Database/ImportTask.cs index 962eab9fa5..cbc5c9005c 100644 --- a/osu.Game/Database/ImportTask.cs +++ b/osu.Game/Database/ImportTask.cs @@ -58,7 +58,7 @@ namespace osu.Game.Database } // This isn't used in any current path. May need to reconsider for performance reasons (ie. if we don't expect the incoming stream to be copied out). - return new LegacyByteArrayReader(Stream.ReadAllBytesToArray(), Path); + return new ByteArrayArchiveReader(Stream.ReadAllBytesToArray(), Path); } /// @@ -80,9 +80,9 @@ namespace osu.Game.Database if (ZipUtils.IsZipArchive(path)) return new ZipArchiveReader(File.Open(path, FileMode.Open, FileAccess.Read, FileShare.Read), System.IO.Path.GetFileName(path)); if (Directory.Exists(path)) - return new LegacyDirectoryArchiveReader(path); + return new DirectoryArchiveReader(path); if (File.Exists(path)) - return new LegacyFileArchiveReader(path); + return new SingleFileArchiveReader(path); throw new InvalidFormatException($"{path} is not a valid archive"); } diff --git a/osu.Game/IO/Archives/LegacyByteArrayReader.cs b/osu.Game/IO/Archives/ByteArrayArchiveReader.cs similarity index 84% rename from osu.Game/IO/Archives/LegacyByteArrayReader.cs rename to osu.Game/IO/Archives/ByteArrayArchiveReader.cs index 06db02b335..0e2dee3456 100644 --- a/osu.Game/IO/Archives/LegacyByteArrayReader.cs +++ b/osu.Game/IO/Archives/ByteArrayArchiveReader.cs @@ -9,11 +9,11 @@ namespace osu.Game.IO.Archives /// /// Allows reading a single file from the provided byte array. /// - public class LegacyByteArrayReader : ArchiveReader + public class ByteArrayArchiveReader : ArchiveReader { private readonly byte[] content; - public LegacyByteArrayReader(byte[] content, string filename) + public ByteArrayArchiveReader(byte[] content, string filename) : base(filename) { this.content = content; diff --git a/osu.Game/IO/Archives/LegacyDirectoryArchiveReader.cs b/osu.Game/IO/Archives/DirectoryArchiveReader.cs similarity index 85% rename from osu.Game/IO/Archives/LegacyDirectoryArchiveReader.cs rename to osu.Game/IO/Archives/DirectoryArchiveReader.cs index 1503705022..f2012b7b49 100644 --- a/osu.Game/IO/Archives/LegacyDirectoryArchiveReader.cs +++ b/osu.Game/IO/Archives/DirectoryArchiveReader.cs @@ -8,13 +8,13 @@ using System.Linq; namespace osu.Game.IO.Archives { /// - /// Reads an archive from a directory on disk. + /// Reads an archive directly from a directory on disk. /// - public class LegacyDirectoryArchiveReader : ArchiveReader + public class DirectoryArchiveReader : ArchiveReader { private readonly string path; - public LegacyDirectoryArchiveReader(string path) + public DirectoryArchiveReader(string path) : base(Path.GetFileName(path)) { // re-get full path to standardise with Directory.GetFiles return values below. diff --git a/osu.Game/IO/Archives/LegacyFileArchiveReader.cs b/osu.Game/IO/Archives/SingleFileArchiveReader.cs similarity index 83% rename from osu.Game/IO/Archives/LegacyFileArchiveReader.cs rename to osu.Game/IO/Archives/SingleFileArchiveReader.cs index c317cae5fc..79d9c5de71 100644 --- a/osu.Game/IO/Archives/LegacyFileArchiveReader.cs +++ b/osu.Game/IO/Archives/SingleFileArchiveReader.cs @@ -7,14 +7,14 @@ using System.IO; namespace osu.Game.IO.Archives { /// - /// Reads a file on disk as an archive. + /// Reads a single file on disk as an archive. /// Note: In this case, the file is not an extractable archive, use instead. /// - public class LegacyFileArchiveReader : ArchiveReader + public class SingleFileArchiveReader : ArchiveReader { private readonly string path; - public LegacyFileArchiveReader(string path) + public SingleFileArchiveReader(string path) : base(Path.GetFileName(path)) { // re-get full path to standardise diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index 8c5828fc92..60806a73c5 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -1144,14 +1144,14 @@ namespace osu.Game.Screens.Play if (DrawableRuleset.ReplayScore != null) return Task.CompletedTask; - LegacyByteArrayReader replayReader = null; + ByteArrayArchiveReader replayReader = null; if (score.ScoreInfo.Ruleset.IsLegacyRuleset()) { using (var stream = new MemoryStream()) { new LegacyScoreEncoder(score, GameplayState.Beatmap).Encode(stream); - replayReader = new LegacyByteArrayReader(stream.ToArray(), "replay.osr"); + replayReader = new ByteArrayArchiveReader(stream.ToArray(), "replay.osr"); } } From 364094fcf29e1a2e8e9f9af9dbd2dab55b28b2d8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Sep 2023 13:38:23 +0900 Subject: [PATCH 4/6] Inline all archive reader pathing --- osu.Game/Database/ImportTask.cs | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/osu.Game/Database/ImportTask.cs b/osu.Game/Database/ImportTask.cs index cbc5c9005c..6d432aac25 100644 --- a/osu.Game/Database/ImportTask.cs +++ b/osu.Game/Database/ImportTask.cs @@ -47,7 +47,16 @@ namespace osu.Game.Database public ArchiveReader GetReader() { if (Stream == null) - return getReaderFromPath(Path); + { + if (ZipUtils.IsZipArchive(Path)) + return new ZipArchiveReader(File.Open(Path, FileMode.Open, FileAccess.Read, FileShare.Read), System.IO.Path.GetFileName(Path)); + if (Directory.Exists(Path)) + return new DirectoryArchiveReader(Path); + if (File.Exists(Path)) + return new SingleFileArchiveReader(Path); + + throw new InvalidFormatException($"{Path} is not a valid archive"); + } if (Stream is MemoryStream memoryStream) { @@ -70,23 +79,6 @@ namespace osu.Game.Database File.Delete(Path); } - /// - /// Creates an from a valid storage path. - /// - /// A file or folder path resolving the archive content. - /// A reader giving access to the archive's content. - private ArchiveReader getReaderFromPath(string path) - { - if (ZipUtils.IsZipArchive(path)) - return new ZipArchiveReader(File.Open(path, FileMode.Open, FileAccess.Read, FileShare.Read), System.IO.Path.GetFileName(path)); - if (Directory.Exists(path)) - return new DirectoryArchiveReader(path); - if (File.Exists(path)) - return new SingleFileArchiveReader(path); - - throw new InvalidFormatException($"{path} is not a valid archive"); - } - public override string ToString() => System.IO.Path.GetFileName(Path); } } From 57f32b177dbecff2c10cceeaef82e7bc6243d931 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Sep 2023 22:19:18 +0900 Subject: [PATCH 5/6] Fix incorrect handling of non-`MemoryStream` pathway --- osu.Game/Database/ImportTask.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/osu.Game/Database/ImportTask.cs b/osu.Game/Database/ImportTask.cs index 6d432aac25..f02f12f8b7 100644 --- a/osu.Game/Database/ImportTask.cs +++ b/osu.Game/Database/ImportTask.cs @@ -58,16 +58,14 @@ namespace osu.Game.Database throw new InvalidFormatException($"{Path} is not a valid archive"); } - if (Stream is MemoryStream memoryStream) - { - if (ZipUtils.IsZipArchive(memoryStream)) - return new ZipArchiveReader(memoryStream, Path); + if (Stream is not MemoryStream memoryStream) + // Path used primarily in tests (converting `ManifestResourceStream`s to `MemoryStream`s). + memoryStream = new MemoryStream(Stream.ReadAllBytesToArray()); - return new MemoryStreamArchiveReader(memoryStream, Path); - } + if (ZipUtils.IsZipArchive(memoryStream)) + return new ZipArchiveReader(memoryStream, Path); - // This isn't used in any current path. May need to reconsider for performance reasons (ie. if we don't expect the incoming stream to be copied out). - return new ByteArrayArchiveReader(Stream.ReadAllBytesToArray(), Path); + return new MemoryStreamArchiveReader(memoryStream, Path); } /// From c8b18acd4dd1d98170a069240b6666cea3d5da07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 14 Sep 2023 19:36:33 +0200 Subject: [PATCH 6/6] Bring back disposal of stream after copy-out to `MemoryStream` Was there in previous code and got removed in the refactor. I'd rather have it than not. --- osu.Game/Database/ImportTask.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Database/ImportTask.cs b/osu.Game/Database/ImportTask.cs index f02f12f8b7..8f2752020b 100644 --- a/osu.Game/Database/ImportTask.cs +++ b/osu.Game/Database/ImportTask.cs @@ -59,8 +59,11 @@ namespace osu.Game.Database } if (Stream is not MemoryStream memoryStream) + { // Path used primarily in tests (converting `ManifestResourceStream`s to `MemoryStream`s). memoryStream = new MemoryStream(Stream.ReadAllBytesToArray()); + Stream.Dispose(); + } if (ZipUtils.IsZipArchive(memoryStream)) return new ZipArchiveReader(memoryStream, Path);