Optimize matchList Selection in find_which_reaction.cpp
Summary
find_which_reaction.cpp currently builds a temporary matchList to collect all matching reaction rates, then performs a second pass to select the match with the greatest number of ancillary interfaces.
This temporary container can be removed and replaced with a streaming best-match selection that preserves the current behavior exactly for the ordinary forward and symmetric reaction paths.
Proposed Change
Replace the temporary matchList construction and subsequent selection pass with:
int bestRateIndex = -1;
std::size_t mostAncillaryIfaces = 0;
for (std::size_t i = 0; i < oneRxn.rateList.size(); ++i) {
const auto& rate = oneRxn.rateList[i];
if (!hasIntangibles(..., rate)) {
continue;
}
const std::size_t ancillaryCount
= rate.otherIfaceLists[0].size()
+ rate.otherIfaceLists[1].size();
if (bestRateIndex == -1
|| ancillaryCount > mostAncillaryIfaces) {
bestRateIndex = static_cast<int>(i);
mostAncillaryIfaces = ancillaryCount;
}
}
The strict > comparison is intentional. It preserves the current first-match-on-tie behavior.
Benefits
This removes:
- heap allocations and reallocations caused by
push_back();
- the temporary match arrays;
- the second scan over all matches;
- pointer arithmetic used to recover rate indices.
Calling reserve() on matchList would only reduce the number of allocations. Eliminating the temporary vector entirely is simpler and more efficient.
The same pattern also appears in:
find_reaction_rate_state.cpp:9
That location should be updated using the same streaming-selection approach.
Reverse Bimolecular State-Change Branch Requires a Separate Fix
The reverse bimolecular state-change branch near:
find_which_reaction.cpp:52
has several correctness defects that should not be mixed into this result-preserving optimization.
Current Defects
-
Back-reaction index zero is incorrectly rejected
The current condition uses:
However, zero is a valid index. The sentinel value is -1, so the condition should distinguish against -1, not require an index greater than zero.
-
Undefined pointer subtraction
Near line 69, the code subtracts a pointer into a back-reaction rate vector from a pointer into the forward-reaction rate vector.
Pointer subtraction is only defined when both pointers refer to elements of the same array or container storage. Subtracting pointers from unrelated vectors is undefined behavior.
-
Single-match case does not assign rateIndex
One branch returns after finding a match without assigning the selected rate index. Callers subsequently treat the reaction as invalid.
-
Multiple-match case does not select the best match
When more than one rate matches, the branch returns without applying the ancillary-interface selection rule.
-
Zero-match case dereferences an empty matchList
The current implementation can access the first element of an empty match list.
Recommendation
The ordinary forward and symmetric paths can be optimized with exact behavioral equivalence.
The reverse bimolecular state-change path should first have its intended semantics established, corrected, and covered by tests. Undefined behavior and incomplete index assignment cannot be meaningfully preserved as part of a strict-consistency optimization.
This should therefore be handled in a separate correctness issue or pull request.
Additional Optimization: hasIntangibles.cpp
A related hotspot exists near:
The current implementation scans every molecular interface for each ancillary-interface requirement.
Because relIfaceIndex is explicitly an index into:
the implementation can use a bounds-checked direct lookup instead of repeatedly scanning the entire interface list.
Current Complexity
Approximately:
O(number of ancillary requirements × number of molecular interfaces)
Proposed Complexity
For valid models using direct indexed lookup:
O(number of ancillary requirements)
The direct lookup should retain bounds validation so malformed indices are handled safely.
Optimize
matchListSelection infind_which_reaction.cppSummary
find_which_reaction.cppcurrently builds a temporarymatchListto collect all matching reaction rates, then performs a second pass to select the match with the greatest number of ancillary interfaces.This temporary container can be removed and replaced with a streaming best-match selection that preserves the current behavior exactly for the ordinary forward and symmetric reaction paths.
Proposed Change
Replace the temporary
matchListconstruction and subsequent selection pass with:The strict
>comparison is intentional. It preserves the current first-match-on-tie behavior.Benefits
This removes:
push_back();Calling
reserve()onmatchListwould only reduce the number of allocations. Eliminating the temporary vector entirely is simpler and more efficient.The same pattern also appears in:
That location should be updated using the same streaming-selection approach.
Reverse Bimolecular State-Change Branch Requires a Separate Fix
The reverse bimolecular state-change branch near:
has several correctness defects that should not be mixed into this result-preserving optimization.
Current Defects
Back-reaction index zero is incorrectly rejected
The current condition uses:
conjBackRxnIndex > 0However, zero is a valid index. The sentinel value is
-1, so the condition should distinguish against-1, not require an index greater than zero.Undefined pointer subtraction
Near line 69, the code subtracts a pointer into a back-reaction rate vector from a pointer into the forward-reaction rate vector.
Pointer subtraction is only defined when both pointers refer to elements of the same array or container storage. Subtracting pointers from unrelated vectors is undefined behavior.
Single-match case does not assign
rateIndexOne branch returns after finding a match without assigning the selected rate index. Callers subsequently treat the reaction as invalid.
Multiple-match case does not select the best match
When more than one rate matches, the branch returns without applying the ancillary-interface selection rule.
Zero-match case dereferences an empty
matchListThe current implementation can access the first element of an empty match list.
Recommendation
The ordinary forward and symmetric paths can be optimized with exact behavioral equivalence.
The reverse bimolecular state-change path should first have its intended semantics established, corrected, and covered by tests. Undefined behavior and incomplete index assignment cannot be meaningfully preserved as part of a strict-consistency optimization.
This should therefore be handled in a separate correctness issue or pull request.
Additional Optimization:
hasIntangibles.cppA related hotspot exists near:
The current implementation scans every molecular interface for each ancillary-interface requirement.
Because
relIfaceIndexis explicitly an index into:the implementation can use a bounds-checked direct lookup instead of repeatedly scanning the entire interface list.
Current Complexity
Approximately:
Proposed Complexity
For valid models using direct indexed lookup:
The direct lookup should retain bounds validation so malformed indices are handled safely.