Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stardew Valley: reuse simplified rules #2393

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

black-sliver
Copy link
Member

@black-sliver black-sliver commented Oct 28, 2023

giving 33% speedup in set_rules
giving 20% speedup in set_rules

Instead of creating new rules in simplify(), we try to replace the current one and remember if it was simplified already. This has the effect of simplification-caching in some of the dicts, like item_rules.

@black-sliver black-sliver marked this pull request as draft October 28, 2023 13:15
@ScootyPuffJr1 ScootyPuffJr1 added the is: refactor/cleanup Improvements to code/output readability or organizization. label Oct 28, 2023
this allows skipping multiple simplifications of the same object, e.g. item_rules
also update the logic simplification tests to be a proper unittest.TestCase
@black-sliver black-sliver changed the title Stardew Valley: cache simplified item rules Stardew Valley: reuse simplified rules Oct 28, 2023
@black-sliver black-sliver marked this pull request as ready for review October 28, 2023 20:03
Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be possible to declare the _simplified field on the parent object, and have some classes like True_() initialize the value to be immediately true. I am not sure this would provide any noticeable improvement in performance, but it could reduce duplication a tiny bit.

Either way, looks good.

@black-sliver
Copy link
Member Author

It would probably be possible to declare the simplified field on the parent object, and have some classes like True() initialize the value to be immediately true. I am not sure this would provide any noticeable improvement in performance, but it could reduce duplication a tiny bit.

True_, False_ and base already have the fastest possible implementation of simplify(), adding _simplified there would not help. The only pure-python idea I'd have is adding another, identical simplify() to True_ and False_ so the dict lookup is a bit faster, however you trade "few instructions" for "possible cache miss" and the difference will not be noticeable anyway.

@black-sliver black-sliver merged commit d9b076a into ArchipelagoMW:main Oct 29, 2023
12 checks passed
@black-sliver black-sliver deleted the sdv-cache-item-rules branch October 29, 2023 12:20
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
this allows skipping multiple simplifications of the same object, e.g. item_rules
also update the logic simplification tests to be a proper unittest.TestCase
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
this allows skipping multiple simplifications of the same object, e.g. item_rules
also update the logic simplification tests to be a proper unittest.TestCase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants