Skip to content

refactor: introduce incremental constraint collectors for improved performance#2270

Open
triceo wants to merge 14 commits intoTimefoldAI:mainfrom
triceo:collector-update
Open

refactor: introduce incremental constraint collectors for improved performance#2270
triceo wants to merge 14 commits intoTimefoldAI:mainfrom
triceo:collector-update

Conversation

@triceo
Copy link
Copy Markdown
Collaborator

@triceo triceo commented Apr 29, 2026

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 triceo force-pushed the collector-update branch from f67e6ae to 2e21a87 Compare May 6, 2026 18:12
@triceo triceo marked this pull request as ready for review May 6, 2026 18:12
Copilot AI review requested due to automatic review settings May 6, 2026 18:12
@triceo
Copy link
Copy Markdown
Collaborator Author

triceo commented May 6, 2026

@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 triceo added this to the v2.1.0 milestone May 6, 2026
@triceo
Copy link
Copy Markdown
Collaborator Author

triceo commented May 6, 2026

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.

@triceo triceo self-assigned this May 6, 2026
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

@Christopher-Chianelli
Copy link
Copy Markdown
Contributor

Christopher-Chianelli commented May 6, 2026

@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
Copy link
Copy Markdown
Contributor

Christopher-Chianelli commented May 6, 2026

  • 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
Copy link
Copy Markdown
Collaborator Author

triceo commented May 6, 2026

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
Copy link
Copy Markdown
Contributor

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);


public abstract class AbstractLongSumSlot {

public static final class State {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MutableLong?


public abstract class AbstractLongAverageSlot {

public static final class State {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why seperate state into its own class? It a private field, so it not like subclasses can access the state field directly.

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.

2 participants