Repository navigation
Power Report Performance - Skip nodes with 0 value. - #10383
Open
AdamZ-8113 wants to merge 5 commits into
Open
AdamZ-8113 wants to merge 5 commits into
AdamZ-8113 wants to merge 5 commits into
Conversation
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
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.
Description of the problem being solved:
This PR makes it so that power reports skip nodes that cannot affect the calculation. It is built on top of several other PRs I've submitted. #10313 and #10373 which I hope will be accepted/merged, and more important #10381 because without it, the results of power reports are wrong. I discovered the issue in 10381 while working on this PR, because I was getting correctness errors.
Transparently I understand a lot less of the code here than I do on the other PRs, this is exclusively Codex driven. The total lines of code changed for just the work to skip nodes that can't affect the calculation are 623 additions, 216 deletion. The other edits are from the other PRs. If there's a more preferable way to submit this, let me know.
The check to skip nodes records which modifier lookups run during a calculation, then checks whether a candidate node could supply a matching modifier. When the checks establish that adding the node cannot change the output, Power Report reuses the unchanged result.
How the new check works
Run one calculation without adding or removing nodes. During this pass, the PR enables recording inside the existing modifier lookup functions in
ModStore.lua, includingSum,More,Flag,Override,List,Tabulate, andHasMod.Record the arguments passed to those functions. For example,
CalcOffence.luacallsskillModList:Sum("INC", cfg, "Speed")when calculating increased/reduced speed. The recording capturesSpeed, theINCmodifier type, and the flags used to filter applicable modifiers. These are recorded before the lookup runs, so a lookup returning zero is still included: a newly added node could provide a modifier that the build currently lacks. Recording also covers checks for passive IDs that are currently unallocated.Construct each candidate node's contribution using existing PoB code. The new
probeNodeContributionfunction callsbuildModListForNodeon a temporary copy of the node. This produces its actual calculation modifiers rather than relying on its displayed text. The probe also includes allocation-count effects, such as increasing the number of allocated notables or masteries. It doesn't change the live passive tree.Compare the candidate's modifiers against the recorded lookups. The new
CalcRelevance.luamodule checks modifier names, types, and flags. Any possible match keeps the normal calculation, even if that calculation might ultimately return zero. A recorded allocation check for that node also prevents skipping it. Only an eligible candidate with no possible match can reuse the unchanged output.This does not introduce a predefined list of modifiers for Hit DPS, Life, or another report metric. The record comes from the lookup functions executed for the current build. It includes lookups performed by nested minion and Full DPS calculations.
Where it keeps the normal calculation
Only pure node additions can be skipped. Removing nodes or combining additions with other changes still uses the calculator. For a path containing several nodes, every node must pass the relevance checks.
The check also falls back to normal calculation for radius-jewel interactions, nodes that grant skills or produce explosion effects, and candidates it cannot safely inspect. If the recording pass encounters a reused skill calculation whose lookups weren't recorded, pruning is disabled for that report; the Manaforged and Reflection cache paths have explicit guards for this.
The recording and node classifications are rebuilt for each report run. A node is never skipped simply because it produced a zero result previously.
Steps taken to verify a working solution:
Once the code from this improvement was tested in isolation, I ran a comparison of all performance PRs together against a clean upstream/dev (true baseline). Across 50 build entries and 100 report configurations, the complete bundle reduced warm Power Report time by 49.5% (1.98x speedup) and cold report time by 48.3%. It was faster in 295/300 paired measurements.
This was all synthetic testing, so I took a side by side video to demonstrate the total speedup https://www.youtube.com/watch?v=XV2QHYDgrFY . I'm still clicking around in the video so timing isn't perfectly synced, but the right side shows the dev branch which is visibly much faster across several builds and tests.
Link to a build that showcases this PR: