From cc0c21a21683e19b21de26160d2313f5aac126d3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 7 May 2025 15:44:27 +0900 Subject: [PATCH 1/2] Ensure carousel filters are manifested to lists at each step The original `IEnumerable` flow prioritised slight performance gains, but a filter's implementation could actually make this detrimental to overall performance. I noticed in passing that there were already potentially multiple enumerations, via `updateYPositions` and the final `ToList` call. Rather than faffing around, let's keep things simple and require lists. In benchmarking, the difference is (currently) negiligible. Slight improvement if anything. --- osu.Game/Graphics/Carousel/Carousel.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/osu.Game/Graphics/Carousel/Carousel.cs b/osu.Game/Graphics/Carousel/Carousel.cs index 8d8289422b..82a42fe459 100644 --- a/osu.Game/Graphics/Carousel/Carousel.cs +++ b/osu.Game/Graphics/Carousel/Carousel.cs @@ -265,7 +265,7 @@ namespace osu.Game.Graphics.Carousel // Copy must be performed on update thread for now (see ConfigureAwait above). // Could potentially be optimised in the future if it becomes an issue. - IEnumerable items = new List(Items.Select(m => new CarouselItem(m))); + List items = new List(Items.Select(m => new CarouselItem(m))); await Task.Run(async () => { @@ -274,7 +274,13 @@ namespace osu.Game.Graphics.Carousel foreach (var filter in Filters) { log($"Performing {filter.GetType().ReadableName()}"); - items = await filter.Run(items, cts.Token).ConfigureAwait(false); + var filteredItems = await filter.Run(items, cts.Token).ConfigureAwait(false); + + // To avoid shooting ourselves in the foot, ensure that we manifest a list after each filter. + // + // A future improvement may be passing a reference list through each filter rather than copying each time, + // but this is the safest approach. + items = filteredItems as List ?? filteredItems.ToList(); } log("Updating Y positions"); @@ -292,7 +298,7 @@ namespace osu.Game.Graphics.Carousel Schedule(() => { log("Items ready for display"); - carouselItems = items.ToList(); + carouselItems = items; displayedRange = null; // Need to call this to ensure correct post-selection logic is handled on the new items list. From ebd67768987f4d2d26cc733be53a4c22f68c2165 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 7 May 2025 16:40:23 +0900 Subject: [PATCH 2/2] Change `ICarouselFilter` interface return type rather than manual `ToList` I was hung up on keeping `IEnumerable` but there doesn't seem to be a good reason to do so. --- osu.Game/Graphics/Carousel/Carousel.cs | 3 +-- osu.Game/Graphics/Carousel/ICarouselFilter.cs | 2 +- osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs | 2 +- osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs | 4 ++-- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/osu.Game/Graphics/Carousel/Carousel.cs b/osu.Game/Graphics/Carousel/Carousel.cs index 82a42fe459..e5319703be 100644 --- a/osu.Game/Graphics/Carousel/Carousel.cs +++ b/osu.Game/Graphics/Carousel/Carousel.cs @@ -274,13 +274,12 @@ namespace osu.Game.Graphics.Carousel foreach (var filter in Filters) { log($"Performing {filter.GetType().ReadableName()}"); - var filteredItems = await filter.Run(items, cts.Token).ConfigureAwait(false); + items = await filter.Run(items, cts.Token).ConfigureAwait(false); // To avoid shooting ourselves in the foot, ensure that we manifest a list after each filter. // // A future improvement may be passing a reference list through each filter rather than copying each time, // but this is the safest approach. - items = filteredItems as List ?? filteredItems.ToList(); } log("Updating Y positions"); diff --git a/osu.Game/Graphics/Carousel/ICarouselFilter.cs b/osu.Game/Graphics/Carousel/ICarouselFilter.cs index 570f480aab..a85b44b46a 100644 --- a/osu.Game/Graphics/Carousel/ICarouselFilter.cs +++ b/osu.Game/Graphics/Carousel/ICarouselFilter.cs @@ -18,6 +18,6 @@ namespace osu.Game.Graphics.Carousel /// The items to be filtered. /// A cancellation token. /// The post-filtered items. - Task> Run(IEnumerable items, CancellationToken cancellationToken); + Task> Run(IEnumerable items, CancellationToken cancellationToken); } } diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index a628595477..6fbaa19045 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -36,7 +36,7 @@ namespace osu.Game.Screens.SelectV2 this.getCriteria = getCriteria; } - public async Task> Run(IEnumerable items, CancellationToken cancellationToken) + public async Task> Run(IEnumerable items, CancellationToken cancellationToken) { return await Task.Run(() => { diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs index 22a67321db..2a4f534a47 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs @@ -23,7 +23,7 @@ namespace osu.Game.Screens.SelectV2 this.getCriteria = getCriteria; } - public async Task> Run(IEnumerable items, CancellationToken cancellationToken) => await Task.Run(() => + public async Task> Run(IEnumerable items, CancellationToken cancellationToken) => await Task.Run(() => { var criteria = getCriteria(); @@ -55,7 +55,7 @@ namespace osu.Game.Screens.SelectV2 } return comparison; - })); + })).ToList(); }, cancellationToken).ConfigureAwait(false); } }