Change strains list to be sorted list in StrainSkill

[Givikap120]

Part of this PR - #29482

Sorted list is implemented from scratch because inbuilt C# SortedList class is not a List but Dictionary and therefore not suitable for this task.
This is changing Summing time complexity for progressive calculation from O(n^2 * logn) to O(nlogn), significantly cutting the time of TimedDifficultyAttributes calculation.
More detailed info is written on original PR.

[peppy]

I'm feeling deja vu on this change...

[peppy]

i dunno.

[peppy]

The zero-count part of this check is super confusing to me. What scenario is this supposed to handle..?

[Givikap120]

This function is called whenever DifficultyValue is called
So this covers pretty much every scenario except progressive calculations, preserving the same behavior as before (just straight up sort all the strains)

[peppy]

Is there a reason we're making two new lists here?

[Givikap120]

Yes, strains is what's gonna be returned with this function and modified in OsuStrainSkill (in the diffspike nerf)
when savedSortedStrains will be potentially used in next call
This is a bit of the performance loose if next call never happens, but I think one extra array copy will not make a big difference in that case

[peppy]

Is there a reason for returning a new copy of the list each time? Is it going to be modified?

[Givikap120]

strains is going to be modified, yes

[smoogipoo]

This is too complex for me, because it's hard to prove anything's correct. If this is the best that can be done code-wise then I prefer the existing implementation/performance.

[smoogipoo]

I'm going to close this one here. I don't have any suggestions, sorry, just... Keep things simple?

[Givikap120]

I'm going to close this one here. I don't have any suggestions, sorry, just... Keep things simple?

This thing was made for optimisation, it cuts execution time for Timed Attributes calculation by 90%

[smoogipoo]

Maybe so, but I'm not merging optimisations at the cost of maintainability, and this is not something I'd want to maintain. Maybe there's a way you can refactor it into a custom object that's fit for this role like LimitedCapacityQueue was, but I would like to avoid (and will not merge) bitfiddling 6 different conditions like this.

[Givikap120]

Maybe so, but I'm not merging optimisations at the cost of maintainability, and this is not something I'd want to maintain. Maybe there's a way you can refactor it into a custom object that's fit for this role like LimitedCapacityQueue was, but I would like to avoid (and will not merge) bitfiddling 6 different conditions like this.

The reason why I made 4 conditions is because:

• I wanted normal diffcalc (not timed) to be the same (because otherwise it will be slower compared to current ver)
• strains are not only added but edited too (specifically - last strain), so just inserting it in normal SortedList won't work

But I understand why you don't want to deal with it, as the performance issue is only really bad on graveyard 60 minutes map (2.5 minutes to wait for pp counter to init)
On ranked maps current worst-case is 15 seconds (down to 1 second with this PR)

[smoogipoo]

I'm not saying the optimisations are bad or unnecessary. I'm saying the code is unmaintainable.

I don't understand where the O(n^2) part is even coming from. Is it because progressive runs this code over and over again? That's an issue for another castle - the whole progressive thing is a gigantic hack and barely an MVP, and needs to be restructured from the ground up. That much was evident even in #29989 .

Rather than hacking existing data structures, try to think about how things could be re-architected to accommodate for the new requirements.

[Givikap120]

I'm not saying the optimisations are bad or unnecessary. I'm saying the code is unmaintainable.

I don't understand where the O(n^2) part is even coming from. Is it because progressive runs this code over and over again? That's an issue for another castle - the whole progressive thing is a gigantic hack and barely an MVP, and needs to be restructured from the ground up. That much was evident even in #29989 .

Rather than hacking existing data structures, try to think about how things could be re-architected to accommodate for the new requirements.

Sort is O(nlogn). This sort runs n times (1 time for each object). Therefore time complexity is O(n^2 * logn). This is by far the worst time complexity out of all diffcalc.
Close second is O(n^2) also in progressive calculations, for copying, calculating combo and finding the objects. The last 2 are fixed in #29989

[smoogipoo]

This sort runs n times (1 time for each object).

Only during progressive calc right?

[Givikap120]

This sort runs n times (1 time for each object).

Only during progressive calc right?

Yes