fix(#6263): one budget for the working set of an algo call, not just its walk buffers - #6285
Conversation
…its walk buffers #6065 capped the embedding-dimension knobs at 4096 and #6216 priced the random-walk buffers against a heap budget. Between them the largest allocation of these procedures stayed outside every budget: the matrices themselves, sized nodeCount x dimension. A dimension cap bounds one embedding ROW at 32 KB and says nothing about the matrix - at node2vec's default dimension of 128 the two Skip-gram matrices cost ~2080 bytes per node, the same order as the walk matrix beside them that was already refused up front. fastRP made the gap plainest: no walk buffer at all, so no budget of any kind applied to it. arcadedb.cypher.algoMaxWalkMemory is renamed arcadedb.cypher.algoMaxWorkingMemory. The key was introduced in this same unreleased version, so no deprecated alias is carried: the concept - estimate in saturating long arithmetic, reject as a client error before allocating, auto-scale the default with the JVM heap - was never walk-specific, only its name was. Reservations now accumulate over the call instead of being checked one allocation at a time, because a single call routinely holds several of these at once and pricing each separately would let it exceed the budget by however many components it happens to have. The error names the component, the counts that sized it and what the call had already reserved. Priced, all before anything is allocated: node2vec (walks + 2 embedding matrices), fastRP, hashGNN (its boolean feature matrices are the larger pair, four times as wide as the embedding), graphSAGE (per-layer peak, including the projection), and the square matrices that no knob sizes at all - apsp, simRank, maxFlow, kShortestPaths, where the graph alone decides and simRank allocates two full nodeCount x nodeCount matrices to answer a question about two nodes.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
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.
Review: #6285 - one budget for the working set of an algo callSolid, well-scoped generalization of the #6216 walk-memory budget into a per-call, accumulating Verification performedI wasn't able to get
I'd still want CI to actually execute Design
Minor observations (non-blocking)
Docs
Nice work, this closes a real gap ( |
…is which Two points from the first review round. The reservation now happens before graph.adjacency(): in fastRP, node2vec, hashGNN, graphSAGE and simRank the estimate needs only the node count and the knobs, so a call that is going to be refused no longer first pays the O(edges) adjacency build. This was the pre-existing #6216 ordering rather than a regression, but every one of these call sites is already being touched here. algo.kShortestPaths sums a double weight matrix and a boolean mask, which are the same shape but not the same size; the message called them "2 matrices of n x n nodes", which the arithmetic behind it never assumed. It now names the element types. algo.maxFlow's two matrices really are both double, so its wording stands. The third observation - that the budget bounds one call rather than aggregate heap across concurrent calls - is left as-is: that is the existing design and a cross-query pool is a different mechanism, not a wording change.
|
Round 1 addressed in 1a0d871. 1. Adjacency built before the budget check - valid, applied. The reservation now precedes
2. 3. Per-call budget, not a global pool - correct, and deliberately left alone. Bounding aggregate heap across concurrently running algo calls is a different mechanism (a shared pool with acquire/release semantics and a policy for what a saturated pool does to a waiting query), not a change to this setting's wording or scope. Worth its own issue if concurrent-load OOMs show up; not something to bolt onto a per-call estimate. On the verification you couldn't run locally: |
ReviewSolid piece of work overall: the accounting is careful (I hand-verified the byte arithmetic in every priced procedure against the test expectations - node2vec, fastrp, hashgnn, graphsage, apsp, simRank, maxFlow, kShortestPaths all check out exactly), the reservations are placed before the O(edges) adjacency build as claimed, and the "accumulate rather than check-per-component" design is correct and is exactly what the One gap:
|
…ger wraps int Found by the second review round, and it is the same defect in both of its forms. terminalNodes is the only knob in the package a caller supplies as data rather than as a number, so nothing validates its length - not even the node count, since repeating the same vertex is accepted. It sizes a terminals x nodeCount pair of Dijkstra tables and then t * (t - 1) / 2 terminal pairs across four parallel arrays, none of which was priced. That expression was evaluated in int. The division by 2 happens after the product, so the result fitting an int never saved it: at 46342 terminals it wrapped and new int[pairCount] died as NegativeArraySizeException: -1073716337, naming nothing. It is now computed in long, reserved against the budget - 1073767311 pairs, about 43 GB, at that length - and refused outright past the entries a Java array can hold, whatever the heap setting says. The pair arrays include an Integer[] index array sorted through a Comparator; new BOXED_INTEGER_BYTES prices it at the 24 bytes an entry it really costs rather than at the 4 of the int it carries. Replacing that boxed sort with a primitive one is a real win but a different change. A sweep of every two-dimensional allocation in the package confirms this was the last unpriced one.
|
Round 2 addressed in 590963c. The Reproduced before fixing. Reverting at 46342 terminals - exactly the shape you predicted. Worth noting why 46342 rather than 65536: the division by 2 happens after the product, so What landed:
Sweep, so round 3 doesn't find another. Every two-dimensional allocation in Verification: 328 tests across On the two remaining notes: the inner-class one is fair - |
ReviewWent through the diff in detail, including cross-checking the new memory estimates against the actual allocation patterns in each touched algorithm (not just the reservation call sites). Summary: this is a well-scoped, carefully verified change. Findings below are minor. Correctness checks performed
Minor: doc/config description omits
|
The description enumerates what the budget bounds, and it is what a user reads through the server config endpoint rather than something only a maintainer sees, so it falling a commit behind the code it describes is a defect in its own small way.
|
Round 3 addressed in 0607a5e. One point, valid, applied.
On the two design observations, no change and no disagreement:
One correction to the numbers in your test-coverage section: Full run after this commit: 48 tests across |
ReviewWent through the full diff (all touched Correctness — looks solid. I manually recomputed Ordering. In each touched procedure ( Rename. Grepped for GraphSAGE peak accounting. Worth double-checking during review: the per-layer reservation prices Minor/non-blocking:
Test coverage. The 14 new tests are well-targeted: one per procedure for the "over budget" case, over-reach guards (fits budget / disabled budget), the accumulation test for node2vec (proving a per-component check would have let 22688 bytes through a 20000-byte budget), and the int-wrap regression for steinerTree. The No blocking issues found. This is a careful, well-scoped fix with unusually strong self-consistency between docs, code, and tests. |
… javadocs Each priced procedure was restating "estimated in long arithmetic and reserved before anything is allocated" nearly verbatim - WHAT the shared helper already documents, in six places, which is the kind of comment CLAUDE.md asks not to write. Each class javadoc now names only what is specifically priced there and links MemoryBudget for the rest. Also settles on `x` for multiplication in the prose this PR added, which had drifted between three characters.
|
Round 4 addressed in eb80d4f. Two of the three applied, one declined as a separate concern. Javadoc repetition - valid, and the sharpest of the three. Six class javadocs were each restating "estimated in long arithmetic and reserved before anything is allocated", which is WHAT Mixed
On the GraphSAGE over-pricing you flagged: correct, and deliberate - later layers project from Test counts, since the description is now two commits behind: 17 tests in |
ReviewWent through the full diff ( Correctness
Test coverageThe new Nits (non-blocking)
Verification noteI was not able to run Overall: well-scoped, well-tested, and the "beyond what the issue asked for" extension (pricing the four graph-only-sized procedures plus |
…is per spur node The test class javadoc had lost half a clause in an edit pass. It also predated algo.steinerTree being priced, so it now mentions the one working set sized by neither a knob nor the graph. AlgoKShortestPaths' comment said only that one mask is ever live, and a reader took that to mean the mask is allocated once and reused. It is allocated per spur node; what is priced once is the PEAK. Said explicitly, along with why the reallocation is a separate question a budget does not answer.
|
Round 5 addressed in f23799c. One nit applied, and one thing in the review itself worth correcting. Garbled sentence in the test javadoc - real, mine, fixed. It had lost half a clause in an edit pass, and it also predated Correction:
Sensitivity, since two rounds of CI could not run the suite: the numbers in the description are from a local run, but the claim that matters is the negative one, and it is reproducible - revert the nine procedures to Latest local run after this commit: 22 tests across |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
🟢 Coverage 100.00% diff coverage · -6.49% coverage variation
Metric Results Coverage variation ✅ -6.49% coverage variation Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (6b20fb1) 159115 125535 78.90% Head commit (4b7e686) 191480 (+32365) 138647 (+13112) 72.41% (-6.49%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#6285) 54 54 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
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.
ReviewI traced the full diff plus the actual algorithm bodies (not just the new reservation call-sites) to check that every byte estimate matches what's really allocated, and worked through the arithmetic in each new Verified correct
Minor observations (non-blocking)
Test coverageThe 14 new tests plus the reused No changes requested from me. |
|
Round 6 is clean - no changes requested - and rounds 4, 5 and 6 all closed without a blocking finding, so I'm treating this as done and leaving the branch at f23799c rather than opening a seventh cycle over cosmetics. The two things this round raised that I did not change, and why:
Deferred work from rounds 4-6 is now #6289: the per-spur-node Final state: 6 review rounds, 5 commits. Round 1 moved the reservations ahead of the O(edges) adjacency build; round 2 found the one procedure I had missed, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6285 +/- ##
==========================================
+ Coverage 70.14% 70.23% +0.08%
==========================================
Files 1840 1840
Lines 159115 159127 +12
Branches 33556 33559 +3
==========================================
+ Hits 111619 111757 +138
+ Misses 33619 33492 -127
- Partials 13877 13878 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Heads-up: #6288 (issue #6264) merged as
One thing worth knowing rather than merging: if this PR renames Also filed #6295 from the same review: |
#6288 (issue #6264) landed first and touched the same three places. Resolved toward one API rather than two. AbstractAlgoProcedure: #6288 generalised `checkWalkBudget` into `checkBufferBudget(db, bytes, what, detail)` so `algo.slpa` could price its label memory. That is the same idea this branch implements as `MemoryBudget`, which additionally accumulates over the call, so keeping both would leave two budget APIs on the same setting. `checkBufferBudget` and `checkWalkBudget` are gone; `algo.slpa`'s reservation goes through `MemoryBudget.reserve` and reads the same to a caller, since the message shape was already identical. Its estimate is now `matrixBytes(n, iterations + 1, INT_BYTES)` - the same figure its hand-rolled expression computed - and moved ahead of the adjacency build like the others. Both branches added `saturatingSum`; the merge kept both copies and the duplicate would not compile. #6288's version is kept, javadoc included: it documents why a footprint that mixes a saturated product with a plain addition must not wrap, which is the better explanation. Its `@link` now points at `MemoryBudget.reserve`, where the hazard it describes would land. GlobalConfiguration: one description covering both, so `algo.slpa`'s label memory is named alongside the matrices. #6288's own release-notes section referred to the setting by its old name, which this branch renames; it now says so and points forward. AlgoGraphSAGE: #6288 gave `layers` a minimum of 1, so the `if (layers > 0)` guard around the layer-matrix reservation is now unreachable-false. Reserved unconditionally. AlgoNode2Vec: this branch moved the reservation ahead of the adjacency build and #6288 made `bytesPerWalk` saturating in place. Both kept - the block moved, with the saturating arithmetic in it.
|
Rebased onto main (4b7e686) after #6288 landed. The overlap was real rather than textual: #6288 needed a heap bound for
Verified after the merge, not assumed: 395 tests green across |
ReviewSolid, well-scoped follow-up to #6216/#6065. The core idea (a per-call I checked the arithmetic in every new Things worth a look before merge:
No correctness bugs found in the priced estimates or the reservation accumulation logic itself. No security concerns (error messages only leak size/count numbers, not data). Nice attention to the GC/performance mantra — everything added is cheap |
Closes #6263.
The gap
#6065 capped every embedding-dimension knob at
MAX_EMBEDDING_DIMENSION(4096); #6216 priced the random-walk buffers against a heap budget. Between them, the largest allocation of these procedures stayed outside every budget: the matrices themselves.A dimension cap bounds one embedding row at 32 KB and says nothing about a
nodeCount x dimensionmatrix. Atalgo.node2vec's defaultembeddingDimension: 128the two Skip-gram matrices cost ~2080 bytes per node (208 MB at 100k nodes, 2.1 GB at 1M, 21 GB at 10M) - the same order as the walk matrix sitting beside them in the same method, at ~3560 bytes per node, which #6216 already refused up front. One budgeted, one not.algo.fastrpmade it plainest: it has no walk buffer at all, so no budget of any kind applied to it.The failure mode was an
OutOfMemoryErrorinstead of the client error naming a parameter that #6065 and #6216 both exist to produce - and an OOM on a shared server takes down work unrelated to the query that caused it.What this does
Option 1 from the issue, generalised further.
Rename, no alias:
arcadedb.cypher.algoMaxWalkMemory->arcadedb.cypher.algoMaxWorkingMemory. The key was introduced in this same unreleased version (26.9.1-SNAPSHOT, #6222 merged today), so there is no deprecated alias to carry - the concept it implements was never walk-specific, only its name was. Defaultmax(64MB, maxHeap/8), negative = no limit,IllegalArgumentException-> HTTP 400: all unchanged.Reservations accumulate over the call rather than being checked one allocation at a time. That is what "how much heap may this call take?" actually asks, and a single call routinely holds several of these at once - node2vec keeps its walk matrix alive while training over two embedding matrices - so a per-component check would let a call exceed the budget by however many components it happens to have.
checkWalkBudgetis replaced by a per-callMemoryBudgetwithreserve(bytes, component, detail), plusmatrixBytes(rows, cols, elementBytes)andsaturatingSumnext to the existingsaturatingProduct.Priced, all before anything is allocated:
algo.node2vecnodeCount x embeddingDimensionalgo.fastrpnodeCount x dimensionsalgo.hashgnnnodeCount x 4*embeddingDimensionfeature matrices + 1 xnodeCount x embeddingDimensionalgo.graphsagenodeCount x embeddingDimension+ the layer'sembeddingDimension x 2*initDimprojectionalgo.apspnodeCount x nodeCountdistance matrixalgo.simRanknodeCount x nodeCountsimilarity matricesalgo.maxFlownodeCount x nodeCountcapacity + residualalgo.kShortestPathsnodeCount x nodeCountweight matrix + removed-edge maskalgo.randomWalkBeyond what the issue asked for, and why
The issue scoped this to the four embedding procedures. The last four rows above are the reason the rename is worth doing at all rather than adding a second walk-shaped key: their matrices are sized by the graph alone, with no knob involved anywhere.
algo.simRank(a, b)answers a question about exactly two nodes and allocates two fullnodeCount x nodeCountmatrices to do it (1.6 GB at 10k nodes), because the similarity of one pair is defined recursively over every pair.algo.apspdocumented itself as suitable "up to a few thousand vertices", which was advice, not a bound. Naming a keyalgoMaxWorkingMemoryand then leaving the largest working sets in the package unpriced would over-promise exactly the wayalgoMaxWalkMemoryunder-promised.These algorithms are cubic or worse in time, so the memory bound bites at roughly the scale where the runtime is already impractical; what changes is that the refusal names the node count and the setting instead of arriving as an OOM several minutes in.
Two findings from the same pass:
algo.hashgnn's feature matrices, not its embedding matrix, are the larger pair. They are four times as wide, so even asbooleanthey cost half a byte per dimension per node against the embedding's eight. Priced by name rather than folded into an "embedding matrices" figure that would understate the call by ~2x.algo.graphsage's projection matrix dominates on a small graph (outDim x 2*initDim= 67584 bytes at the defaults). Its peak is per-layer, not per-run - each layer drops the matrix it read - solayersdoes not multiply the reservation.AlgoGraphSAGE.java'snew double[outDim][concatDim], flagged in the issue as "worth pricing in the same pass", is included.Verification
New
Issue6263AlgoWorkingMemoryBudgetTest, 14 tests. Sensitivity checked the way #6216 was: the eight touched procedures reverted toHEAD(and node2vec's embedding reservation removed by hand, since it cannot compile against the old helper) -> 11 of 14 red. The three that stay green are the over-reach guards, which must pass both ways: the budget must not refuse a call it used to serve, at an ample budget, at a disabled budget, and -everyPricedProcedureStillRunsUnderTheDefaultBudget- at the untouched default across all nine procedures.Algo*Test+Issue6216*+Issue6263*: 325 tests green*Cypher*Test+*Configuration*Test: 3191 tests greenmvn install -DskipTests: greenRelease notes appended to
docs/release-26.9.1.md, and the #6216 section's mention of the old key updated so the notes never name a key that no release contains.