Refactor grouping to be much more efficient #31823
Conversation
| @@ -36,7 +36,7 @@ public async Task<IEnumerable<CarouselItem>> Run(IEnumerable<CarouselItem> items | |||
| switch (criteria.Sort) | |||
| { | |||
| case SortMode.Artist: | |||
| comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(ab.BeatmapSet!.Metadata.Artist, bb.BeatmapSet!.Metadata.Artist); | |||
| comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(ab.Metadata.Artist, bb.Metadata.Artist); | |||
There was a problem hiding this comment.
I dunno about these changes... Metadata is technically not supposed to vary within a single set, but it could, and I don't think we check that in any way. Could lead to some dumb shenanigans happening.
At least previously, because of going through the set, it was assured that every beatmap of a set had the same metadata (namely the metadata of the first beatmap in the set).
There was a problem hiding this comment.
The overhead of the set traversal was quite large (in the seconds for the performance test).
I don't see this change as an issue because if/when we change the correlation of metadata to sets/beatmaps, it's a given that we'd be examining every usage with a magnifying glass.
There was a problem hiding this comment.
Just to confirm understanding, the performance gain here is all from avoiding multiple iterations of items, correct?
There was a problem hiding this comment.
The major gain was not using Insert on the list (~80%).
And probably easier code on the eyes too.
This sees a reduction of ~9 seconds to ~2 seconds with 200k beatmap sets.
The remaining largest bottleneck is string comparisons.