fix(#6296, #6289): the biggest allocation of an algo call is the one nobody was looking at - #6298
Conversation
…nobody was looking at #6263 bounded the working set of an `algo.*` call. Both of these were left out of it deliberately: #6296 is the *result* set of the same call, and #6289 is churn rather than a peak, so neither is something that budget could have caught. ## #6296 - algo.apsp materialised n² rows before returning any of them The distance matrix is reserved against `arcadedb.cypher.algoMaxWorkingMemory`; the rows beside it were not. At the 64 MB floor of that setting the matrix check admits n ≈ 2890, and a connected graph of 2890 nodes then built ~8.3M `ResultInternal` objects - each with a three-entry property map, all alive at once, well over a gigabyte - against the 64 MB the budget had just finished enforcing next to it. The budget did its job and the call still died, of an allocation its message never named. Floyd-Warshall completes the matrix before the first row is emitted, so the rows are a pure projection of it and nothing required them to exist together. They now stream: the row-side footprint is O(1), and `CallStep` keeps the iterator rather than collecting it, so a `LIMIT` upstream costs what it says it costs. Measured on the 200-node cycle in the regression test: building the stream went from 13.6 MB to 83 KB, and the rows are now paid for as they are read. ## #6289 - allocation churn on the dense paths 1. `algo.kShortestPaths` allocated a `nodeCount x nodeCount` removed-edge mask per spur node - ~200 MB through the young generation for one k=10 call at 1000 nodes. The issue proposed hoisting it and clearing it, and asked for a measurement first. Neither was needed: the mask never had to be square. Yen's keeps the edge (p[i], p[i+1]) only for a path sharing the root p[0..i] with prevPath, and prevPath[i] IS the spur node - so every entry it ever set had the same source, and the other n-1 rows were allocated and zeroed only to prove they were empty. One `boolean[nodeCount]` indexed by target says exactly as much. It is allocated once for the call and cleared by naming the few entries it set, so there is no `Arrays.fill` over the node count either. 43.3 MB -> 4.4 MB on the test graph, and the budget reservation now prices two node-sized masks rather than a square one. 2. `algo.steinerTree` and `algo.mst` sorted their Kruskal indices through an `Integer[]` - 24 bytes per entry against the 4 the `int` occupies, over a count that is quadratic in a caller-supplied terminal list. Replaced by a shared primitive index sort. A bottom-up merge sort rather than the index quicksort the issue suggested: the keys are user data, so a quicksort degrades to O(n²) on an input a caller chooses, and merging is stable, which is what keeps the tie order identical to the TimSort behind `Arrays.sort(Integer[], Comparator)`. Measured at 8.4 bytes per index against 24. `BOXED_INTEGER_BYTES`, which existed only to price the boxing honestly, is gone with it. 3. `MemoryBudget.reserve()` added to the running total before checking it, so a refused reservation was recorded as granted and the next message quoted it back as "the N bytes this call already reserved". Committed only once granted now. While in `AlgoKShortestPaths`: an unused `PriorityQueue` and a dead incremental cost expression that computed the path weight four ways - two `Arrays.copyOf` and four `pathWeight` walks per spur node - and then threw the answer away in favour of the single `pathWeight(totalPath, ...)` beside it.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewWent through the diff carefully (algorithm changes, memory-budget fix, and both new test classes). Overall this is a well-scoped, well-tested fix — the two allocation issues are real, the fixes are minimal and match the root cause, and the regression tests actually pin the old-vs-new behavior rather than just re-asserting the new one. A few notes below; nothing blocking. Correctness
Memory-budget bookkeeping
Style / minor nits
Test coverageGood — the new tests pin correctness (k distinct ascending-weight paths, MST/Steiner tree unchanged from both directions) alongside the allocation-shape assertions, and the PR description states each allocation assertion was checked against the pre-fix code (reverting the fix turns the corresponding test red). That's exactly the discipline you want for this kind of change, where "faster" and "just wrong" can look identical without a correctness counterweight. Nice writeup in the PR description too — the "why not X" call-outs (not touching |
Review of #6298 raised one substantive gap and two nits. The gap: making the rows lazy moves exactly one thing - `graph.getRID()` now runs as a row is read rather than inside `execute()`. On the OLTP-backed `GraphData` that is an in-memory field read that cannot care when it happens; on the CSR-backed one it goes through the analytical view's node mapping, which takes a database reference. Both new fixtures were plain OLTP, so that path was unpinned. `theRowsAreTheSameWhenTheGraphIsCSRBacked` builds a `GraphAnalyticalView` over the cycle, asserts `CSR_ACCELERATED_VAR` so the test cannot silently pin the OLTP path a second time, and checks the same n² - n pairs at the same hop distances. Codacy's one minor finding, on the same line: `!(distances[j] >= INF)` was written as a literal negation of the eager loop's guard so the row set would provably match for every double. It is equivalent to `< INF` here anyway - no NaN can reach the matrix, since every write to it is guarded by `<`, which a NaN fails - so the straight comparison goes back in and the argument moves to the comment. Also: `arefused` -> `aRefused` in a test name, and a paragraph in both test classes on why the allocation assertions are tagged `performance` and not `benchmark` - `benchmark` is one of the three lanes CI excludes, and a regression guard that never runs guards nothing. These read a per-thread allocation counter rather than a clock, so the default lane is where they belong. The review's third note - that `algo.mst` reserves no budget for its edge arrays - is real and pre-existing, and stays out of scope here.
ReviewWent through the full diff ( Verified correct
Minor observations (not blocking)
Test coverageThe new regression tests are well-targeted: No functional or security issues found. Nice work isolating churn-vs-peak as a separate concern from the #6263 budget, and for actually proving the O(n²) mask reduction rather than just asserting it. |
|
Follow-up for the review's one actionable note: the missing The other two notes I'm leaving as they are: the |
1 participant
Closes #6296, closes #6289.
Both were surfaced while reviewing PR #6285 (issue #6263) and deliberately left out of it: #6296 is the result set of the same call rather than its working set, and #6289 is churn rather than a peak. Neither is something that budget could have caught.
#6296 -
algo.apspmaterialised n² rows before returning any of themThe distance matrix is reserved against
arcadedb.cypher.algoMaxWorkingMemory; the rows beside it were not. At the 64 MB floor of that setting the matrix check admits n ≈ 2890, and a connected graph of 2890 nodes then built ~8.3MResultInternalobjects - each with a three-entry property map, all alive at once, well over a gigabyte - against the 64 MB the budget had just finished enforcing next to it. The budget did its job and the call still died, of an allocation its message never named.Floyd-Warshall completes the matrix before the first row is emitted, so the rows are a pure projection of it and nothing required them to exist together. They now stream. The row-side footprint is O(1), and
CallStepkeeps the iterator rather than collecting it, so aLIMITupstream costs what it says it costs.Measured on the 200-node cycle in the regression test (
ThreadMXBean.getThreadAllocatedBytes, per-thread so no GC can move it):The issue also raises whether the row count should be bounded. Streaming answers it in the right place: the consumer decides how many it holds, and a
LIMITis now what bounds the call. A top-k form of the procedure would be a different interface, not a fix, so it is left alone.The secondary finding in the issue -
toEmbeddingListboxing every element - is deliberately not taken: a primitive-backedList<Double>view boxes onget()instead of up front, so on a consumer that reads each row once it moves the allocation rather than removing it. The issue itself says it is only worth doing if a profile says the boxing shows up, and this change is not that profile.#6289 - allocation churn on the dense paths
1. The per-spur-node
n x nmask inalgo.kShortestPaths. ~200 MB through the young generation for one k=10 call at 1000 nodes. The issue proposed hoisting the mask and clearing it, and asked for a measurement before choosing between that and reallocation.Neither was needed - the mask never had to be square, which is the better alternative the measurement would not have found. Yen's keeps the edge
(p[i], p[i+1])only for a path sharing the rootp[0..i]withprevPath, andprevPath[i]is the spur node, so every entry it ever set had the same source. The other n-1 rows were allocated and zeroed only to prove they were empty. A singleboolean[nodeCount]indexed by target says exactly as much, in O(nodeCount) rather than O(nodeCount²).That also removes the
Arrays.fill-vs-reallocate question the issue framed: the mask is allocated once for the whole call and cleared by naming the few entries it set - at most one per previously-found path - so clearing is O(paths), not O(nodeCount). The budget reservation follows the allocation and now prices two node-sized masks instead of a square one.Measured on a 501-node graph driving ~150 spur nodes: 43 259 968 -> 4 411 528 bytes for the call, against a weight matrix of 2 008 008 bytes allocated once.
2. The boxed
Integer[]Kruskal sort inalgo.steinerTreeandalgo.mst. Replaced by one shared primitive index sort,AbstractAlgoProcedure.sortedIndexesByWeight.A bottom-up merge sort rather than the index quicksort the issue suggested, for two reasons that both matter here: the keys are user data, so a quicksort degrades to O(n²) on an input a caller chooses; and merging is stable, which is what keeps ties in exactly the order the TimSort behind
Arrays.sort(Integer[], Comparator)produced.Double.compareis the comparison, soNaNand-0.0order identically too - the test asserts the new order against the old comparator sort on both awkward and randomised inputs.Measured: 8.4 bytes per index against 24.
BOXED_INTEGER_BYTES, which existed only to price that boxing honestly, is gone with it, andalgo.steinerTree's reservation now prices five primitive arrays per terminal pair.3.
MemoryBudget.reserve()mutating before it throws. Committed only once granted. The invariant is now observable: a refused reservation no longer appears in the "on top of the N bytes this call already reserved" of the next message.Also, while in
AlgoKShortestPathsAn unused
PriorityQueue, and a dead incremental cost expression that computed the path weight four ways - twoArrays.copyOfand fourpathWeightwalks per spur node - and then threw the answer away in favour of the singlepathWeight(totalPath, ...)on the line below it. Same file, same theme.Verification
Every allocation assertion was checked against the old code before being trusted: reverting each fix in turn makes the corresponding test red (13.6 MB build for #6296, 43.3 MB for the mask, a lying "already reserved" figure for the budget), so none of them is vacuous.
Issue6296AlgoAPSPLazyRowsTest(4 tests) - the full n² - n row set is unchanged and each pair appears once; distances around the cycle are unchanged; a partially read stream still yields its tail; and the allocation split between building and draining.Issue6289AlgoAllocationChurnTest(7 tests) - k distinct paths in ascending weight (the correctness counterweight to collapsing the mask); the reservation message; the call's allocation against the weight matrix; the index sort against the boxed comparator sort it replaced, including NaN, ±0.0, the infinities and tie stability; MST and Steiner trees unchanged, pinned from both sides (the MST takes the two cheap bypass edges of every hop, the Steiner path takes the single direct hop instead); and the budget invariant.Issue6263AlgoWorkingMemoryBudgetTestupdated for the smallerkShortestPathsreservation (400 -> 328 bytes on its 4-node graph).com.arcadedb.query.opencyphersweep: 4520 tests, 0 failures.The two
ThreadMXBeanmeasurements are tagged@Tag("performance"), matchingRidScoreMinHeapTest, and skip themselves on a JVM without per-thread allocation counters.Files changed
engine/.../algo/AbstractAlgoProcedure.java-sortedIndexesByWeight, budget invariant,BOXED_INTEGER_BYTESremovedengine/.../algo/AlgoAPSP.java- lazy rowsengine/.../algo/AlgoKShortestPaths.java- node-sized spur masks, reservation, dead codeengine/.../algo/AlgoMST.java,engine/.../algo/AlgoSteinerTree.java- primitive index sortengine/src/test/.../algo/Issue6296AlgoAPSPLazyRowsTest.java(new),Issue6289AlgoAllocationChurnTest.java(new),Issue6263AlgoWorkingMemoryBudgetTest.java