Repository navigation
state: json, traverse and reset without recursing once per level - #668
Merged
Merged
Conversation
The walks #664 left recursive: ptree()/json()/save() and their loader, traverse(), and reset(). - ptree()/json()/save() write one level: an entry per node keyed by its full path, in pre-order, holding its value ("" for a childless empty node below the start). Those are the leaves of the nested document they used to write, in the same order. boost's ptree, write_json and read_json recurse once per nesting level, so the nested form could not be made deep-safe without a codec of our own. The loader walks every nesting level with a heap stack, so nested documents still load. - traverse() visits each node once. It traverse()d every descendant children() listed, so a node d levels down a chain was visited 2^(d-1) times (state_cluster_shard::snapshot() listed entries that many times). Pre-order with a heap stack, traverseEnter/Exit nested, the regex as before; it walks held nodes instead of re-finding them by name from the root, which recreated nodes removed meanwhile. - reset(true) clears pre-order and touches post-order as before, with a heap stack and no lock held while signaling. The recursion held every ancestor's lock during each touch, so a slot that looked its node up from the root (as the sync adapter's do) deadlocked. reset(false) drops the child map after releasing the lock, so destroyed() slots don't run under it. New state_deep_tree_test cases run on a 64 KiB stack: json/save/restore round trip 1000 deep, traverse 3000 deep, reset 100000 deep (3000 with signals), plus the visit order, reset order, flat format, legacy nested load and the lock-free signaling. state_snapshot_protocol_test checks a snapshot lists each node once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #664.
Why
#664 removed the per-level recursion from most of
cvc::state, but listed three walks it did not cover:ptree()/json()/save()and their loader (ptree(pt),json(j),restore());traverse();reset(true).Each still used a native frame per tree level, so a deep tree overflows a small stack (wasm's 64 KiB). Two of them were also wrong in other ways:
traverse()visited a node once per ancestor in the walk. It calledtraverse()on every descendantchildren()listed, and each of those did the same. A node d levels down a chain was visited 2^(d-1) times: a 20-deep chain made 1,048,576 visits for 21 nodes (12.6 s).state_cluster_shard::snapshot()walks withtraverse(), so its snapshots listed (and encoded) entries that many times.reset(true)deadlocked against ordinary slots. It held every ancestor's lock while each descendant's touch signaled. A slot that looks its node up from the root takes those locks. The sync adapter'svalueChangedslots do exactly that, soroot("a").reset()with an adapter attached and children underahangs on a non-recursive mutex.What
ptree(),json(),save(): one level. The document has an entry per node, keyed by its full path, in pre-order (children in name order). Each entry holds the node's value, or""for a childless empty node below the start.Those are exactly the leaves of the nested document these used to write, in the same order. The keys were already full paths, so nesting added nothing.
boost's ptree,
write_jsonandread_jsonall recurse once per nesting level. Only making the walk iterative would still have leftjson()overflowing; the nested form could not be made deep-safe without a JSON codec of our own.No consumer in the repo reads the nested shape:
state_json, ariadnesave_state/restore_stateandstate_listfeed it back to the loader;state jsonprints it, and xmlrpccvcstate_get_jsonreturns it.That printed or returned output is the visible change: it is now one flat object.
Each node's value and children are read once under its own lock. Before, the walk held every ancestor's lock while listing the subtree below it.
traverse(): each node once.re(all of them when it is empty).traverseEnterfires beforefuncsees it, and itstraverseExitfires after the nodes under it. With a pattern, the walk still goes under nodes that do not match, as before.state_ptrs instead of finding each node again by name from the root. That lookup recreated nodes removed meanwhile.funchas seen it, aschildren()used to be.reset(true): same order, no locks held while signaling.ResetClearsTopDownAndTouchesBottomUppins that order and passes on state: walk the tree and its links without recursing once per level #664's code too.reset(false)swaps the child map out under the lock and destroys it after the lock is released, so the subtree'sdestroyed()slots no longer run under the node's lock.Measurements (MSVC 14.38 Release, Windows)
Deepest operation that passes, before (#664's code) → after. A 64 KiB or 256 KiB stack is a thread with that stack; 1 MiB is the main thread.
json()+save(), thenjson(j)+restore()into a fresh app (test capped at 1000)reset(true, false)of a chainreset()with a listener on the root (capped at 3000)traverse()of a chain (capped at 3000)traverse()before never got deep enough to overflow, because its cost was exponential. Visits for a chain d deep, before: 1024 at d = 10 (19 ms), 65,536 at 16 (0.86 s), 1,048,576 at 20 (12.6 s). After: d + 1 visits, and 3000 deep takes 0.27 s on a 64 KiB stack.ResetSignalsWithoutTheAncestorsLocksdeadlocks on state: walk the tree and its links without recursing once per level #664's code. Its watchdog fires after 60 s. On this branch it passes in under 1 ms.Tests
state_deep_tree_testcases (8; the file now has 18). The deep ones run on a 64 KiB thread:json()/save()→json(j)/restore()round trip;traverse()order, enter/exit nesting and the regex filter;traverse()3000 deep, with visit and enter/exit counts;reset(true, false)100000 deep, andreset()3000 deep with a root listener (leaf first, then upward);reset(false)'sdestroyed()slots taking the ancestors' locks.StateSnapshotProtocolTest.SnapshotListsEachNodeOnce: before,a.b.cwas listed twice.CVC_STATE_DEEP_DEPTH/CVC_STATE_DEEP_STACK_KIBwork as in state: walk the tree and its links without recursing once per level #664.image_testReadsCmykInputAsRgbandcvc_cli_testVol2Img/Img2Vol: ImageMagick has no decode delegate here, as on master.StateClusterMembership.FailureDetectionDeadand.HeartbeatRevivesSuspectPeerfailed once while the pycvc scripts ran alongside. They passed in the run before the rebase and 5 of 5 times alone. These are heartbeat timing tests, and the membership code uses none of these walks.test_pycvc_state.py'sstate_jsonround trip. The other two:test_pycvc.pyfails on the.rawivfile lock that volume: close the file after a successful rawiv write #663 fixes.test_pycvc_lod.pypasses every check, then crashes at interpreter exit. state: walk the tree and its links without recursing once per level #664's build does the same (3 of 3 runs on each).check_test_targets.pyis OK (174 executables).Found, not fixed
state_list::erase()/insert()copy an element's subtree withdst.ptree(src.ptree()).ptree()'s keys are full paths from the root, butptree(pt)applies them relative todst. So the copy lands underdst.<src's full path>, anddst's own children are not updated. This is the same before and after this PR. Only scalar list elements are tested today. The same asymmetry applies to xmlrpccvcstate_set_json(path, …)and ariadnerestore_state()on a node other than the root.