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

Idea / Proposal for caching stuff? #3989

Open
wants to merge 7 commits into
base: thewarwithin
Choose a base branch
from

Conversation

syrifgit
Copy link
Collaborator

@syrifgit syrifgit commented Oct 18, 2024

Not necessarily ready to be a real PR, just looking for input.

Part 1 - Caching duration of an aura which is re-used

Does doing this make sense to reduce overhead across many iterations? The only time the duration would change is during a talent change, so this could maybe be reduced even further?

If it does make sense, would you be interesting in larger scale refactoring of this type where it can be done relatively easily? I'd be happy to pick away at it over time.

( Also ignore the numbers not matching, I am using the anniversary patch numbers in this example but on the current live file.)

Part 2 - caching last tick during reset_precast events

cache previous tick to avoid recalculation?

(ignore the beast cleave thing, same as above, anniversary patch vs live file, not relevant to the idea)

@johnnylam88
Copy link
Contributor

For something like your example of caching an aura duration, I think you're better off just using a common function for all of them and just memoizing the return value, although you'd want to forget the memoized return value in case of a Hekili:ForceUpdate().

@Hekili
Copy link
Owner

Hekili commented Oct 19, 2024

There's more than can be tracked proactively and used reactively, for sure. The trouble is determining if it will actually payoff as an optimization ahead of time.

Tick times actually change a lot because of so many haste effects triggering and expiring in combat.

The addon very nearly tracks the last tick (even for non-targets) via updateTarget (I think), given that it takes the opportunity to reset the damage-based target detection timer when those events occur. It's a really archaic part of the addon's code, though.

In the long term, supporting multiple 'actors' and having actual aura data for all active enemies is something that I want to do. I just don't want to sink the hours into it and find out it's too computationally expensive to bother.

@Hekili
Copy link
Owner

Hekili commented Oct 19, 2024

As far as this caching goes, class.auras.X.duration is outside the current tag/reset system, but your approach is similar to how caching and resetting is completed by other state data.

Duration will sometimes explicitly need to be left uncached (i.e., combo points impacting duration need to be calculated dynamically at each step in case of combo point changes).

There may be an approach where setting these durations once when applicable events occur, if there's nothing else truly dynamic about the duration. I.e., a hook in SpecializationChanged to recalculate based on talent changes, or updateGear based on gear changes.

This would require a lot more fiddly config for adding/updating auras and catching the right influential events. In which case, I'd want to be confident that we're saving a worthwhile amount of CPU time before implementing.

@syrifgit
Copy link
Collaborator Author

Do you know of a way I can specifically check the calls relating to auras, or even better to duration checks, when cpu profiling with engine dump and frame info dump? Maybe something I can add somewhere for testing purposes?

@Hekili
Copy link
Owner

Hekili commented Oct 19, 2024

You could create a wrapper function in RegisterAura that replaces the supplied duration function and stores the information.

local table_for_duration_profiling = {}

Replace:

        -- This is a shared buff that can come from anyone, give it a special generator.
        --[[ if data.shared then
            a.generate = Aura_DetectSharedAura
        end ]]

with:

    if type( data.duration ) == "function" then
        local function wrapped = function()
			local start = debugprofilestop()
            local value = data.duration()
            local finish = debugprofilestop()

            if not table_for_duration_profiling[ data.key ] then
                table_for_duration_profiling[ data.key ] = { time = 0, count = 0 }
            end

            table_for_duration_profiling[ data.key ].time = table_for_duration_profiling[ data.key ].time + finish - start
            table_for_duration_profiling[ data.key ].count = table_for_duration_profiling[ data.key ].count + 1
            return value
        end
		data.duration = wrapped

Then you can make that table accessible by adding it to the addon's table or somewhere else accessible:

    Hekili.DurP = table_for_duration_profiling

Then access in-game with /dump Hekili.DurP or make a function that calculates the average cost per aura iteration (table_for_duration_profiling[ aura].time / max( 1, table_for_duration_profling[aura ].count )).

(This was dry-coded, so there are probably errors or things I didn't think about, but I think you'll get the idea.)

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