perf: eliminate duplicate full-DAG evaluation per server mutation#156
Merged
Conversation
…uation Part of #128. The two-line idiom "effects = active_effects(...); resolved = Evaluator.evaluate!(...)" was repeated verbatim at four sites in characters.ex (prep_context, concept_roll!, resolve_all, compute_pending_choice_slots). Each repetition is a chance for the pipeline to drift (e.g. one site forgetting inventory effects if the effects composition ever changes), and having no single owner makes future memoization of the full-DAG evaluation impossible. resolved_state/2 now owns the pipeline and returns {effects, resolved} as a tuple rather than just the resolved map because compute_pending_choice_slots needs the effects list afterward (it re-evaluates per level with XP substituted). The other three sites discard the effects element. The function is public rather than private because the CLI server duplicates the same idiom (resolve_character/2 in server.ex) and will be switched to this helper in the next commit.
Closes #128. Every mutation request (award, resolve_choice, build_finish) evaluated the full DAG twice on the same character state: the server's resolve_character/2 ran active_effects + Evaluator.evaluate! to compute pending choices, then Serializer.serialize_character independently ran the identical two lines again. For read paths the duplication was latent rather than active, but the idiom itself was copy-pasted in three places across the two apps. serialize_character/5 now accepts an optional precomputed resolved map and falls back to Characters.resolved_state/2 (added in the previous commit) when given none. Handlers that already hold a resolved map (award x2, resolve_choice x2, build_finish) pass it through, so those requests evaluate the DAG exactly once. Handlers that never resolved separately (show, build_start, random_resolve) keep relying on the serializer's internal fallback — they already evaluated only once, so threading state through them would add plumbing for no saved work. resolve_character/2 is deleted; both remaining evaluation entry points in the CLI now go through the library helper, which is the single place a future memoization would live. The optional parameter is a plain resolved map rather than the full {effects, resolved} tuple because the serializer never uses the effects list; passing the tuple would couple its signature to state it ignores.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
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.
Closes #128.
What
Two commits:
refactor(characters): extract resolved_state/2— theactive_effects+Evaluator.evaluate!idiom appeared verbatim at four sites incharacters.ex(six at the time the issue was filed; refactor(characters): unify auto-resolve and tracked resolution appliers #127 already collapsed two of them intoresolve_all).Characters.resolved_state/2now owns the pipeline and returns{effects, resolved}— a tuple becausecompute_pending_choice_slotsneeds the effects list afterward for its per-level re-evaluation; the other sites discard it.perf(cli): evaluate the DAG once per server mutation request— every mutation request (award, resolve_choice, build_finish) evaluated the full DAG twice: the server'sresolve_character/2once for pending choices, thenSerializer.serialize_characteragain internally.serialize_character/5now takes an optional precomputed resolved map (falling back toresolved_state/2when absent), the five handler sites that already hold one pass it through, andresolve_character/2is deleted.Notes
show,build_start,random_resolve) keep using the serializer's internal fallback — they already evaluated once, so threading state through them would be plumbing for no saved work.{effects, resolved}tuple, since it never uses the effects list.resolve_allloops is inherent (effects change between iterations); centralizing inresolved_state/2is what makes future memoization possible.No behavior change; all 389 tests pass unchanged, credo and dialyzer clean.
Summary by Bito