refactor: introduce incremental constraint collectors for improved performance

[triceo]

Still a lot of work to do, but early PoCs look promising.

This benchmark run proves there are no regressions from adding the incremental support to the GroupNode:
https://github.com/TimefoldAI/timefold-solver-benchmarks/actions/runs/25095715799/job/73533079940

This benchmark proves no regressions after the new collector implementations were integrated:
https://github.com/TimefoldAI/timefold-solver-benchmarks/actions/runs/25427319378

There are small performance improvements, but the most expensive collectors are not actually part of those benchmarks, so the biggest benefit of this PR does not show up there.

[triceo]

@Christopher-Chianelli Ready for review now.

I wasn't able to keep the uni changes separate from the bi/tri/quad, so this PR is large. But if you review the uni parts, you can just skim the rest; they are mechanical copies with updates for the higher arities.

[triceo]

It would be great if we had update() variants for consecutive sequences and connected intervals; those collectors are expensive - if we can save some work there, it will immediately show.

[Christopher-Chianelli]

@triceo GitHub still not allowing me to create any review comments, so I putting my review comments directly on the the PR.

UniConstraintCollector - Does a user need to implement both accumulator and incrementalAccumulator? I notice some of our builtin ConstraintCollectors (such as AndThenUniCollector) implement both?

[Christopher-Chianelli]

• Per the Javadoc of ...AccumulatedValue:

/**
 * <li>{@link #add(Object)} will be called externally exactly once, when the value enters the group.
 * An instance of {@link UniConstraintCollectorAccumulatedValue} will only be created if there is a value to add.</li>
 * /

• But in ConditionalUniCollector:

        private final UniConstraintCollectorAccumulatedValue<A> innerValue;
        private boolean active = false;

        AccumulatedValue(ResultContainer_ container) {
            this.innerValue = innerIncremental.intoGroup(container);
        }

        @Override
        public void add(A a) {
            if (!predicate.test(a)) {
                return;
            }
            active = true;
            innerValue.add(a);
        }

        @Override
        public void update(A a) {
            boolean nowActive = predicate.test(a);
            if (active && nowActive) {
                innerValue.update(a);
            } else if (active) {
                active = false;
                innerValue.remove();
            } else if (nowActive) {
                active = true;
                innerValue.add(a);
            }
        }

i.e. add and remove can both be called multiple times.

[triceo]

i.e. add and remove can both be called multiple times.

They can, but they will not be. That is a contract that we guarantee.
(The alternative was to create more garbage, essentially add() would create a new container - and that was negatively affecting benchmarks.)

[Christopher-Chianelli]

i.e. add and remove can both be called multiple times.

They can, but they will not be. That is a contract that we guarantee. (The alternative was to create more garbage, essentially add() would create a new container - and that was negatively affecting benchmarks.)

Something like this would cause add/remove to be called multiple times currently:

constraintFactory.forEach(Employee.class)
    .join(Shift.class, equal(id(), Shift::getEmployee)
    // Employee has an InverseCollectionShadowVariable to get shift count
    .groupBy((employee, shift) -> employee, conditionally((employee, shift) -> employee.getShiftCount() > 5, sum((employee, shift) -> shift.getDuration())))
    .filter((employee, hours) -> hours != null && hours > 24)
    .penalize(HardSoftScore.ONE_HARD, (employee, hours) -> hours - 24)
    .asConstraint("Overworked employee")

In particular, the conditionally can and will call the sum's add multiple times. It seems our actually contract is:

• add can only be called when the value is not "added" (and once called, the value is "added")
• update can only be called when the value is "added" (and does not affect the state of "added")
• remove can only be called when the value is "added" (and once called, the value is not "added")

i.e. this is explictly allowed:

var a = new Data();
value.add(a);
a.change();
value.update(a);
value.remove(a);
value.add(a);

[triceo]

Something like this would cause add/remove to be called multiple times currently:

Good point. For collectors which contain other collectors (and only for them), the contract as specified doesn't actually hold.

Two ways out of this:

• These nesting collectors would correctly recreate the inner instances on every remove+add, so that the contract still holds.
• Or we update the contract to what you outlined in your previous message.

Since we want to prevent unnecessary garbage from being collected, the second option is probably the way to go.

[triceo]

UniConstraintCollector - Does a user need to implement both accumulator and incrementalAccumulator? I notice some of our builtin ConstraintCollectors (such as AndThenUniCollector) implement both?

User typically only implements one, and uses the isIncremental() method to say which they implemented. Our own default collectors indeed implement both, and only for backwards compatibility reasons - if for whatever reason the user decided to call the old non-incremental collector manually, they'd get an exception if they were not implemented, breaking backwards compatibility.

[triceo]

@Christopher-Chianelli I have resolved your comments so far, but I feel as if the review wasn't yet complete.

In addition to whatever you will to point out, I'm looking for a discussion of the following points specifically:

• Is "incremental collector" a good name for the concept? Aren't all collectors incremental? "updatable collector" ain't much better, all collectors are updatable, only right now it's done through remove+add.
• The switching between incremental and non-incremental collectors; the isIncremental() method. There was no way to map the old collector API to be incremental backwards compatibly, which is why I had to introduce this. But I don't like it - it forces the user to make a choice, and based on that choice, some methods of the API become useless; it's ugly. Can you think of a different way of doing this?
• UniConstraintCollectorAccumulatedValue is IMO a horrible name. Doesn't say much. Any suggestions for improvement?

[Christopher-Chianelli]

@Christopher-Chianelli I have resolved your comments so far, but I feel as if the review wasn't yet complete.

I am still going through the review; keeping context and making notes for a 8000+ line changes is not trivial, even if most of the changes are mechanical.

* Is "incremental collector" a good name for the concept? Aren't all collectors incremental? "updatable collector" ain't much better, all collectors are updatable, only right now it's done through remove+add.

Slot-based? The old collector API use a global state, whereas this uses a per-tuple state.

* The switching between incremental and non-incremental collectors; the isIncremental() method. There was no way to map the old collector API to be incremental backwards compatibly, which is why I had to introduce this. But I don't like it - it forces the user to make a choice, and based on that choice, some methods of the API become useless; it's ugly. Can you think of a different way of doing this?

Create a sealed interface AbstractConstraintCollector, make the old and new constraint collector interfaces two seperate implementations of it, and change the CS groupBy API to take AbstractConstraintCollector instead.

* `UniConstraintCollectorAccumulatedValue` is IMO a horrible name. Doesn't say much. Any suggestions for improvement?

UniConstraintCollectorValueHandle? https://en.wikipedia.org/wiki/Handle_(computing)

[Christopher-Chianelli]

Does cachedInnerMap only have one key at a time? removedMapped() is always called before this. It seems you are using a Map strictly for its entry, so you can have a (value, count) tuple.

[sonarqubecloud]

Quality Gate failed

Failed conditions
21.9% Duplication on New Code (required ≤ 5%)

See analysis details on SonarQube Cloud