chore: improve sorting operation

[zepfred]

This PR enables sorting the value ranges at the ValueRangeManager level.

The sorting feature is likely useful during the CH phase, and the new approach can address the sorting requirement for the existing CH configurations. However, the existing selector sorting nodes remain available, as they are required for range filtering or nearby selectors. Also, these corner cases may only make sense for the LS phases.

[zepfred]

@triceo The PR is ready for review and will rerun all previous model validations, just like we did when adding the sorting feature.

[zepfred]

There is an issue with the quickstart compilation job, and I will take a look.

[triceo]

I am not excited about how sorting creates yet another completely separate path for some selectors, and another part for others.

ValueRangeManager needs to be the ultimate source of truth. Think of the future, where we are inevitably going to be asked to enable mutable value ranges on entities. (When variables change, so do value ranges.) This complexity simply cannot be managed, if value ranges are all over the place. This needs to be centralized.
This future is closer than you think; early next year I expect.

IMO we should start cutting off the paths that lead to these discrepancies. Deprecating, failing fast. Especially when we introduce new functionality, like here - with new functionality, there is no backwards compatibility. We can simply decide to make some things impossible. We can start to reduce the impossible cross-combinations of move selector configurations.

Also, you are far too attached to the ValueRange type. In the future, this type will not exist. Every ValueRange will just be a Collection. Make your future you's life easier, and stop treating the ValueRange interface as a building block for everything.

We only have a limited amount of time to spend on this, and there are good things in here. Let's discuss where we go from here.

[triceo]

There is an issue with the quickstart compilation job, and I will take a look.

The Vert.x exception is a red herring. Your real problem is here:

025-11-04 07:29:04,008 ERROR [org.acm.pro.res.ProjectJobSchedulingResource] (pool-8-thread-1) Failed solving jobId (52a73dc5-6458-4eda-bb33-61a1b2a59fd4).: java.lang.UnsupportedOperationException
at ai.timefold.solver.core.impl.domain.valuerange.AbstractCountableValueRange.sort(AbstractCountableValueRange.java:44)

[triceo]

I'm thinking we're approaching this all wrong.

We're building this hierarchy of selectors, decorating with additional behaviors. But value ranges shouldn't work like that. Value ranges are meant to be the root selector. When sorting is necessary, it should be applied to the root selector directly, not via some decorator later. The root iteration should already happen in the sorted order.

How do values come into the value range selector? Who puts them there? That is where we need to start - and that source of data needs to be the value range manager. Which will already have everything sorted. And then we can start building all the other functionality on top of that - the filter decorator etc., all of that can be layered on top of what's already sorted.

This is a large refactor, and maybe we don't want to do it right now. But this is the way to make sure that value range manager is the only source of truth for data. And consequently, it is the only way how we can safely produce value ranges which change with their entity variables.

[zepfred]

I'm thinking we're approaching this all wrong.

We're building this hierarchy of selectors, decorating with additional behaviors. But value ranges shouldn't work like that. Value ranges are meant to be the root selector. When sorting is necessary, it should be applied to the root selector directly, not via some decorator later. The root iteration should already happen in the sorted order.

That's the current approach. The base method buildBaseValueSelector creates the root node, which uses the sorter and fetches the sorted value range when needed. I'm currently working to remove the step applySorting when filtering range nodes or nearby selectors are used.

How do values come into the value range selector? Who puts them there? That is where we need to start - and that source of data needs to be the value range manager. Which will already have everything sorted. And then we can start building all the other functionality on top of that - the filter decorator etc., all of that can be layered on top of what's already sorted.

Yes, that's correct. However, the range filtering and the nearby nodes ignore the root selector sort order because they use different structures to fetch the values: reachable values and the distance matrix.

This is a large refactor, and maybe we don't want to do it right now. But this is the way to make sure that value range manager is the only source of truth for data. And consequently, it is the only way how we can safely produce value ranges which change with their entity variables.

I have a solution to maintain the ValueRangeManager as the single source of truth for the reachable values structure and have not yet looked into the nearby selector.

[zepfred]

@triceo I'll begin reviewing the necessary changes to the enterprise repository, and ValueSelectorFactory will also be modified to pass the sorter to the inner nodes.

This change is needed because both FilteringValueRangeSelector and the nearby nodes use a different structure to compute the values list, and they might not follow the sort order from the root nodes.

[triceo]

We already confirmed that the code works, so I'm only focusing on quality of the code itself.

This is a good proof of concept, but you cannot stop there - after you've proven that your idea works, you have to clean up the code. Turn it into something that will be easy to maintain going forward, and perhaps even by people who aren't you.

ValueRangeManager and related classes, as it stands now, are spaghetti code. It is hard to read, it is hard to understand, and it will be hard to maintain. No page-long methods, no 1000-line classes. I give some advice in comments.

[triceo]

We are suddenly adding this to all the selectors, and I have to ask... why now? Why was it not a problem ever before, in the 10+ years that the selector code has existed?

This smells. We are introducing new behavior, IMO without fully understanding why it worked perfectly fine without it.

[zepfred]

ListRuinRecreateMoveSelector isn't that old. The purpose of these changes is to release allocated resources and prevent memory leaks. I reviewed all nodes to identify non-final fields whose references we haven't cleared by the end of the phase and the solver.

For example, the CH repository PlacerBasedMoveRepository was not clearing the placementIterator reference, causing related resources such as value range caches and reachable value structures to never be reclaimed after the CH phase.

Also, these are private instances that are not used once they are cleared.

[triceo]

See, this is the strange part to me.

Why would we clear the inside of something, when the outside should just get garbage collected? We should remove a reference to the thing, and everything inside the thing will get collected automatically.

Specifically in the case of PlacerBasedMoveRepository - why does that even survive the CH phase? Something is keeping it alive, and that is what we should be solving. What you're doing here is curing the symptoms, not the disease.

[zepfred]

Let's revisit PlacerBasedMoveRepository. The CH phase instance remains with the AbstractSolver until the process completes. This means that DefaultConstructionHeuristicPhase contains a final field moveRepository that is not cleared. Since the final field references an iterator (placementIterator) that may also hold references to ReachableValues, the range cache and all internal reachable values structures will not be released, even if the ValueRangeManager is reset.

[zepfred]

You can confirm what I'm telling you at AbstractSolver#runPhases.

[triceo]

I believe you. :-) But my point is - by removing the phase instance once it's no longer needed, you solve the root cause.

Whether we want to do it is another thing. But it is cleaner than resetting things and hoping that everyone always remembers to do it.

[zepfred]

Removing the phases after the process finishes is already done. The point is to keep data from finished phases while another is still running.

[triceo]

This method will be very expensive. It happens for every single value range, it iterates over it (possibly 20k items) and for each item it will do a hash lookup. If there is one place that would benefit from micro-optimization, it is this one. And then when the lookup is done, there will be a conversion from Integer to int.

My bet is that this is going to jump out in profiling, and by far.

[zepfred]

I'm unsure how we can improve this. Remember that the index map, which includes all planning values and the ordinal IDs, is not available when the descriptors are created or when we extract the data.

The class CompositeCountableValueRange could benefit from access to the map of values and ordinal IDs, but the current logic doesn't support passing this information, and the solution to enable that seems worse to me. On the other hand, other range caches like ListValueRange need to read all the data to compute the BitSet.

Please let me know if you have any suggestions for improving it.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[triceo]

We're getting there.
I am making suggestions for how to break this class down into manageable subcomponents.

If you make each of them small, each will be easy to understand.
And ValueRangeManager will become just a conductor of an orchestra - and will become easy to understand too.

[triceo]

Notice: This is code duplication. It is also not important at this point in time for me to know that this happens.

If you hide this in the ValueRangeItem constructor, this method immediately becomes easier to read. Hide complexity, separate concerns.

[triceo]

This is slightly different than above, so we cannot simply share the code. But what we can do is to create two static constructors on ValueRangeItem which will hide this away.

[triceo]

This field is now being used from exactly one method. You know what that tells me?

The field and the method can be extracted to a separate type. Hide the complexity, expose a well-named type with a well-named method. Nobody needs to know that there is an array, and hashmaps etc. etc.

[triceo]

All code dealing with this field is also hidden in just a few methods. They do not deal with much of the other state. This tells me they can be extracted into another class. Hide complexity, separate concerns, avoid massive classes.

[triceo]

All these arrays are based on value range descriptors.
Which means this class is effectively N instances within 1 class; 1 for each value range descriptor.

Arguably, if you refactor the logic so that each VRM works with exactly one VRD, the code will also get much simpler. (You will just have to have one "router" class to delegate requests to these instances based on the specific VRD used.) Something like valueRangeManager.getFor(valueRangeDescriptor).