Misc improvements#2520
Open
triceo wants to merge 2 commits into
Open
Conversation
…ch node ForEachFilteredUniNode.update() re-entered insert()/retract() on filter transitions, corrupting tupleCountWithoutFiltering: a fact flipping fail->pass was counted twice, and a fact flipping pass->fail was decremented while still inserted. The activation protocol (ActivitySupport) reads that counter exactly once, at the first settle() — and shadow variable initialization delivers updates between fact insertion and that first settle. A pass->fail transition in that window could therefore zero the counter and permanently remove the node and its downstream constraint from propagation, silently producing wrong scores for the rest of the session. Counter mutations are now exclusive to the external insert()/retract() entry points; update()'s filter transitions route through non-counting helpers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntaining-index removal put() already holds the downstream indexer reference when it builds the per-child bookkeeping, but remove() looked it up again from the indexer map, hashing a user key per child per removal. The child entry now carries the indexer alongside the key (Pair becomes Triple inside CompositeListEntry), and remove() uses it directly. In conference_scheduling this lookup was among the top HashMap.getNode callers.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers two small internal correctness/robustness improvements in the Bavet implementation: it tightens ForEachFilteredUniNode activity tracking to be based on “facts inserted” rather than “tuples propagated after filtering”, and it makes CONTAINING/CONTAINING_ANY_OF indexers more resilient during removal by carrying the resolved downstream indexer along with each child entry.
Changes:
- Adjust
ForEachFilteredUniNodeto track activity based on the number of inserted facts regardless of filter outcome, and avoid corrupting that count during filter transitions onupdate(). - Make
ContainingIndexerandContainingAnyOfIndexerstore the downstreamIndexerin eachCompositeListEntrychild to avoid re-lookup onremove()and enable safe key reuse after last tuple removal. - Add regression tests covering filtered-node activity behavior and key reuse after last removal for both indexer types.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/ai/timefold/solver/core/impl/bavet/uni/ForEachFilteredUniNode.java | Fixes activity bookkeeping so filtered-out facts still keep the node active until retracted. |
| core/src/test/java/ai/timefold/solver/core/impl/bavet/uni/ForEachFilteredUniNodeActivityTest.java | Adds regression coverage for activity behavior across filter transitions and activation timing. |
| core/src/main/java/ai/timefold/solver/core/impl/bavet/common/index/ContainingIndexer.java | Stores downstream indexer per child entry to make removal robust and allow downstream map cleanup without breaking later operations. |
| core/src/main/java/ai/timefold/solver/core/impl/bavet/common/index/ContainingAnyOfIndexer.java | Same as above for CONTAINING_ANY_OF behavior. |
| core/src/main/java/ai/timefold/solver/core/impl/util/CompositeListEntry.java | Updates child payload from Pair to Triple to carry downstream indexer alongside key and list entry. |
| core/src/test/java/ai/timefold/solver/core/impl/bavet/common/index/ContainingIndexerTest.java | Adds regression test for key reuse after last tuple removal. |
| core/src/test/java/ai/timefold/solver/core/impl/bavet/common/index/ContainingAnyOfIndexerTest.java | Adds regression test for key reuse after last tuple removal. |
Comment on lines
+5
to
+21
| import ai.timefold.solver.core.impl.bavet.common.index.Indexer; | ||
| import ai.timefold.solver.core.impl.bavet.common.joiner.JoinerType; | ||
|
|
||
| import org.jspecify.annotations.NullMarked; | ||
|
|
||
| /** | ||
| * Allows to store the same tuple in multiple downstream {@link ElementAwareLinkedList}s. | ||
| * Needed by {@link JoinerType#CONTAINING}, {@link JoinerType#CONTAINING_ANY_OF}, ... | ||
| * Each child carries the downstream {@link Indexer} resolved at put() time, | ||
| * so that remove() does not need to look it up again. | ||
| * | ||
| * @param <T> the tuple type | ||
| */ | ||
| @NullMarked | ||
| public record CompositeListEntry<Key_, T>(T element, List<Pair<Key_, ListEntry<T>>> children) implements ListEntry<T> { | ||
| public record CompositeListEntry<Key_, T>(T element, List<Triple<Key_, Indexer<T>, ListEntry<T>>> children) | ||
| implements | ||
| ListEntry<T> { |
|
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.
Two unrelated things I discovered while working something else.
Will be merged as individual commits.