Performance improvements: save flows for NestedTree - #231
Conversation
Replaces the per-descendant save() loop in moveTo() with a single bulk depth update, and skips the depth recompute on saves that did not move the node. Moving a subtree of 15 nodes drops from 188 SQL statements to 7 in testing.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I tested this against the real NestedTree trait in a stock Winter app on MariaDB. Moving a subtree of 200 nodes runs 608 SQL statements and takes 116 ms on develop. The old code saves every descendant one by one. Each save opens a transaction, reloads, counts ancestors, and writes an update. The bulk depth update in this PR brings the same move down to 8 statements and 2.9 ms, about 37 times faster. The statement count also stays flat as the subtree grows. At 15, 50, and 200 descendants it is always 8, where develop ran 53, 158, and 608. A plain rename drops from 4 statements to 1, because the depth recompute now skips a node that did not move. There is one honest caveat about the numbers. The 188 to 7 figure in the description came from an app that clears caches in afterSave for each descendant. That alone roughly tripled the count. Stock storm sits at 53 to 8. The linear to constant shape holds either way. 
|
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
2 participants
Summary
Two changes to
NestedTreethat remove most of the query overhead from save and move operations, with no change to the resulting tree state.1.
moveTo()no longer saves every descendant individually.A move previously ended with:
Each of those saves exists only to realign
nest_depth, but pays for a full model lifecycle:setDepth()opens a transaction, runs areload(), an ancestorcount(), and anUPDATE, and the save fires every bound model event. Since a subtree moves intact, every descendant shifts by the same depth difference as the moved node, so a single bulkUPDATE nest_depth = nest_depth + (delta)over the subtree range replaces the loop. If the moved node had no stored depth (corrupt or legacy data), the code falls back to realigning each descendant from its own ancestry viasetDepth(true).2.
setDepth()skips recomputing when the save did not move the node.setDepth()runs on everymodel.afterSave. For an ordinary save (e.g. renaming a node) it still paid the transaction + reload + ancestor count + UPDATE. It now returns early when the save cycle did not move the node (moveToNewParentId === false) and a depth is already stored.moveToNewParent()also marks the pending move as processed after handling it, which removes a second, redundant depth recompute that previously followed every move inside the sameafterSave.Measurements
Against a real application table (MySQL/MariaDB), moving a node with 15 descendants via
makeChildOf():The statement count is now constant regardless of subtree size (previously ~12 statements per descendant, plus whatever the host application binds to its save events; our application clears caches in
afterSave, so a single drag in a reorder UI also triggered one cache clear per descendant).A plain
save()with no parent change drops the trait's 3-statement depth recompute (and its transaction) entirely.Behavior notes
nest_depthagainst the ancestry, so a manually corrupted depth value is no longer silently self-healed by unrelated saves. Moves still realign depth from the ancestry.Tests
Four new tests in
NestedTreeTest: subtree moves realign descendant depths (deeper and shallower, viamakeChildOf()/makeRoot()and viaparent_idreassignment + save), non-move saves skip the depth recompute, and a query-count regression test pinning the constant-statement move. Full suite passes: 715 tests, 3145 assertions (13 pre-existing PHPUnit deprecations, 9 pre-existing skips).