Performance improvements for gem dropdown sort#2184
Open
ltogniolli wants to merge 3 commits into
Open
Conversation
The dropdown hover tooltip was cleared and rebuilt every frame, and each rebuild runs a full build calculation - hovering one gem cost ~60 full calcs per second. It now rebuilds only when the hovered gem, the build's outputRevision, or the relevant default settings change (tooltip:CheckForUpdate), turning the per-frame cost into a one-time cost per hovered gem. This also implements the fastCalcOptions path that the DPS sort loop was already calling into: UpdateSortCache defines the options and CalcOutputWithThisGem forwards them to the misc calculator, which now accepts them as an optional third argument: - Accelerated environment reuse: per-gem calcs carry over the cached player/enemy/minion DBs and a persistent environment with the existing accelerate flags (nodeAlloc, requirementsItems, requirementsGems), instead of rebuilding unchanged state from scratch for every gem. - skipEHP: defence estimations (EHP/max hit) are skipped during sorting unless sorting by Effective Hit Pool. - fullDPSOnly: when sorting by Full DPS, only the calcFullDPS roll-up is computed; the main-skill pass whose output was discarded is skipped. All other GetMiscCalculator callers pass two arguments and are unchanged. Verified by comparing outputs of the fast and unaccelerated paths for every supportable gem on test builds: identical results across all sort fields.
When sorting the gem dropdown by Full DPS, every candidate gem triggered a full calcFullDPS pass: one perform per Full-DPS-included skill, even though a support socketed into one group cannot affect most other skills. On builds with many included skills (e.g. several companions), this multiplied the cost of every dropdown open by the skill count. calcFullDPS now supports an optional per-skill result cache (specEnv.fullDPSCache), driven by diffing each skill's calculation inputs rather than by guessing what the candidate gem does: - During the base pass, each skill's harvested outputs are captured into a plain snapshot, together with its input references: the skill's own modifier list and the environment's "coupling surface" - the buffs, auras and curses every skill provides (buffList) and the exposure it can inflict, which are the channels through which one skill's gems can influence another skill's results. - On later calls, a skill whose own modifier list and the coupling surface are both unchanged merges its cached snapshot instead of recalculating. Anything that touches the surface (auras, exposure, buff-granting supports) conservatively recalculates every skill. - Modifier lists are compared by table identity, with a depth-limited structural fallback for the few modifiers that are reconstructed on every initEnv, which would otherwise defeat the diff. The harvest section of calcFullDPS is restructured into capture-then- merge to make snapshots replayable; merge order and semantics (sum vs maximum fields, Absolution count fix, entry naming) are preserved exactly. The cache is only used by the gem sort's fullDPSOnly path and is rebuilt with the calculator on every build change, so all other calcFullDPS callers and any stale-data concerns are unaffected. Adds TestFullDPSCache covering: cached results matching fresh calculations, local supports reusing other skills' results, exposure and aura supports forcing recalculation, the reconstructed-modifier corner case, candidate gem levels, and cache invalidation on build changes.
…tries - Replace the repeated per-stat, per-actor merge blocks with a single merge spec and fold; pass snapshots hold uniform per-actor entries, with the per-actor differences (entry naming, pass counts, dot scaling and gating) resolved as plain data at capture time. - Unify the captured output fields into one harvestFields list. Stats previously not folded for minions never occur on test builds, and the mirage path is currently disabled, so results are unchanged. - Compare modifiers with tableDeepEquals, called in both directions since it only inspects the first table's keys; a one-sided match could let a modifier that gained a field produce a stale cache hit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opening the gem dropdown can take a long time. This is specially noticeable when sorting by Full DPS when you have several sources of full dps enabled. Two sets of improvements:
The first commit adds a guard to avoid recalculating the support skill tooltip multiple times per second on hover. It also implements the fastCalc path partially introduced on #1504 that avoids recalculating unnecessary things when in Full DPS mode. This gives a modest 10% improvement on my testing.
The second commit adds a more aggressive cache that avoids getting into the calculation at all by storing inputs and outputs of the previous calculation only recalculating Full DPS targets when the change input impact that skill. It provides a much better 60-70% improvement on my testing.
This cache only applies to the fastCalc/FullDPS path so existing callers are unnaffected decreasing the risk.
It doesn't include my changes from #2147 . if that lands first the caching needs to be updated, and vice versa.
Steps taken to verify a working solution:
I automated runs in a few of my builds making sure the cached/uncached values match, created an automated test spec with the main assertions and edge cases encountered, and manual testing.
Before screenshot:
After screenshot: