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

[Player] cache find_spell lookups #6563

Open
wants to merge 1 commit into
base: shadowlands
Choose a base branch
from

Conversation

seanpeters86
Copy link
Member

This is roughly a 33% increase in speed when testing locally for shadow priest. I added a debug line when the cache is hit and it seems like almost all of these are for items or special effects. There were over 300,000 cache hits in a single iteration when testing locally.

@scamille
Copy link
Member

My gut feeling says that we might want to rather fix the underlying code that calls find_spell so often instead of potentially increasing memory usage so significantly.

On the other hand, a 33% increase does sound lovely. How many iterations and thread did you use when getting this number?

@seanpeters86
Copy link
Member Author

@scamille the root cause of most of these issues was fixed here: 7df2bff

so really the question is do we leave this in as insurance in case it happens again?

Was talking with @lgkern and @Saeldur about this and i think it would make sense to refactor this into something that simply alerts on change if you have added a significant amount of find_spells compared to previous or something.

@Dorovon
Copy link
Contributor

Dorovon commented May 27, 2022

One option might be to add a counter in debug builds with an assert (or maybe just a warning) that goes off if some excessive number of lookups occur.

@navv1234
Copy link
Contributor

I don't mind having a base level cache for this stuff at least, but it's just a bit sad that we have gone from having a relatively fast (O(1)) access back in the day to having an issue with the id-based find_spell() performance-wise.

@scamille
Copy link
Member

One option might be to add a counter in debug builds with an assert (or maybe just a warning) that goes off if some excessive number of lookups occur.

I like the idea. I think we could just generally warn on any find_spell after combat begin on iteration>0. Sure there might be some false positives for things with lazy initialization, but it would be interesting to see how bad it is.

@lgkern
Copy link
Contributor

lgkern commented May 28, 2022

Yea, that was one idea we bounced around, to have a way for it to fail tests when find_spell is being called during iterations, that would make it easy to detect, at least after a couple of callings.

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.

5 participants