Skip to content

Fix operator precedence in ProvisionalEvaluationCache::on_failure - #80

Draft
xmakro wants to merge 1 commit into
perf/base-0713from
fix/provisional-cache-on-failure
Draft

Fix operator precedence in ProvisionalEvaluationCache::on_failure#80
xmakro wants to merge 1 commit into
perf/base-0713from
fix/provisional-cache-on-failure

Conversation

@xmakro

@xmakro xmakro commented Jul 31, 2026

Copy link
Copy Markdown
Owner

on_failure is meant to discard the provisional cache entries created since the given depth-first number. on_completion documents the rule with concrete numbers: with dfn of 2 for the C node, the entry for D at DFN 3 is removed while A and B at DFNs 0 and 1 are kept. next_dfn is a plain increasing counter, so entries created while a node is on the stack have the higher numbers, and insert_provisional uses the same >= from_dfn test to find the entries it created. on_failure should therefore remove from_dfn >= dfn.

The condition is currently written !eval.from_dfn >= dfn, where ! is a bitwise negation of a usize rather than a logical one, so it evaluates as usize::MAX - from_dfn >= dfn and holds for any realistic value. Every entry is dropped whenever an obligation fails.

The history is worth spelling out, because the obvious reading of the typo gives the wrong fix. Before 59f5045 ("add more debug logs") this was a retain predicate, retain(|_key, eval| eval.from_dfn >= dfn), which kept from_dfn >= dfn and dropped everything older. Expanding a retain predicate into an if/else that returns false to remove requires negating it, so the author wanted !(eval.from_dfn >= dfn) and the parenthesis is what went missing. But restoring that would restore a predicate that was already inverted with respect to both doc comments: it keeps precisely the entries that may depend on the failing node and discards the independent ones. The stray ! turned an inverted predicate into an unconditional clear, which is wasteful but conservative, and that is presumably why nothing ever misbehaved.

So this is not a restoration of intended behaviour. The documented behaviour appears never to have been implemented. That also explains why correcting it is not a performance win: I measured no significant change on syn, serde, hyper and ripgrep, so nothing has been tuned around the cache working. Taking it as a correctness and clarity fix, it retains more entries than the current code does and wants the usual test coverage on that basis.

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.

1 participant