Stardew Valley: Improve default options performances by grouping rules before counting them

[Jouramie]

What is this fixing or adding?

This improves performances when filling Stardew Valley seed. The biggest gain is when playing with Museumsanity set to milestones (default), which improves performances by ~10%.

This works because a lot of artifacts have the same condition. Some rules only need to be evaluated once, but count toward multiple artifacts.

How was this tested?

Ran our performance tests, filling ~80 seeds with multiple settings.

If this makes graphical changes, please attach screenshots.

N/A

[agilbert1412]

Lots of that is Python black magic, but none of it seems wrong. And the gains are pretty cool!

[Jouramie]

Moving this back to draft because the change I did to support 3.8 seems to drop performances. I'll have to check again.

[agilbert1412]

I approve the code as-is, but if the performance gain is not there anymore due to 3.8, I agree with drafting the PR

[black-sliver]

damn you python 3.8 I wanted to use slots :(

if you can measure an actual performance gain, this can be done using kwarg unpacking

if sys.version_info >= (3,10,0):
     dataclass_args = { "slots": True }
else:
     dataclass_args = {}

@dataclass(..., **dataclass_args)
class SomeClass:
    ...

Slots can be slower though. If they are on par, going slots is the better choice for memory.

Mom can we have python? We already have python at home son.

You can monkey patch the counter for py<3.10 and then enjoy the fast implementation on 3.10. Depending on how often the counter changes, and how often it is read, you could also cache the total in the counter.

[Jouramie]

Slots can be slower though. If they are on par, going slots is the better choice for memory.

I'll mesure it. I'm not even sure it's used at its full capacity because of the inheritance.

You can monkey patch the counter for py<3.10 and then enjoy the fast implementation on 3.10. Depending on how often the counter changes, and how often it is read, you could also cache the total in the counter.

The counter is not expected to change, so I'll just cache it. In the end, the 3.10 probably won't change anything.

[Jouramie]

Turns out I was comparing runs against the 6.x.x branch, which also has more performance improvements, still being worked on. I updated the screenshot of the benchmarking comparing this branch against main, and we can see the improvement is still there.

I will measure and add slots if it improves performances significantly enough in a different PR.

[Jouramie]

And turns out again that this is a huge bug in my implementation, because evaluate_while_simplifying return a tuple and not a boolean. Anyway, the performance gains are not as much as I first thought, but this still generates the default settings faster.

[agilbert1412]

Is this now included in #3478 ? @Jouramie if so can you close this one?

[Jouramie]

Yes it's included, good catch!