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

Optimize governors #2312

Merged
merged 7 commits into from
Jul 15, 2024
Merged

Conversation

lmoureaux
Copy link
Contributor

@lmoureaux lmoureaux commented Jul 6, 2024

Profiling has shown a few low hanging fruits to speed up governors. This implements them.

Profiling governors showed that finding them amounted to 10% of the
runtime. Factoring this out slightly up in the stack reduces the number
of calls.
@lmoureaux lmoureaux requested a review from jwrober July 6, 2024 18:47
@hugomflavio
Copy link
Contributor

Time from entering password to a responsive client decreased from ~1m30s using the master to ~35s using this PR.

@lmoureaux lmoureaux force-pushed the feature/optimize-governors branch from 53d4abf to 569bacd Compare July 6, 2024 21:22
Profiling has shown city_tile_iterate to be expensive, with the
governors spending 20% of their time in it. They are used in a double
loop with output types. Swap the two loops to have the expensive one
outside, speeding up governors by about 15% in LTT.
@lmoureaux lmoureaux force-pushed the feature/optimize-governors branch from 569bacd to 263b439 Compare July 6, 2024 21:56
lmoureaux added 4 commits July 7, 2024 01:06
The code had the concept of "free worked tiles" but the only such tile
has only ever been the city center. Remove this from the code and just
refer to the city center.
This reduces the number of calls to city_tile_output and speeds up
finding the results.
This speeds things up by reducing the frequency at which we evaluate
effects.
Evaluating waste-related effects was found to represent half of the
governors' runtime. Cache them to speed things up.

This optimization breaks if waste depends on the celebration status, but
governors are generally broken in this regard (they also don't
understand how tile outputs might change).
@lmoureaux lmoureaux force-pushed the feature/optimize-governors branch from 82fbb72 to 059db33 Compare July 7, 2024 03:22
This avoids a large number of effect evaluations, speeding things up
significantly.
@jwrober
Copy link
Collaborator

jwrober commented Jul 8, 2024

Code Factor not happy. Correction needed?

@lmoureaux
Copy link
Contributor Author

It's unhappy because I modified a complex function without simplifying it.

@jwrober
Copy link
Collaborator

jwrober commented Jul 8, 2024

Ok Sounds good.

Copy link
Collaborator

@jwrober jwrober left a comment

Choose a reason for hiding this comment

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

Seems to improve performance for me.

@jwrober jwrober merged commit 5c44029 into longturn:master Jul 15, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants