Repository navigation
fix(profiler): scan the object graph breadth-first - #25
Merged
Merged
Conversation
Discovery used a depth-first walk against a global visit budget, so the first object-valued own property of stateObj could consume the whole budget and stop the scan for every sibling after it. Extensions that hold cached data in the first sibling -- rss-feed keeps a Map of feed sources, each with an array of parsed items -- ended up with only that subtree instrumented; the objects carrying the extension's actual behaviour were never reached, and the call tree collapsed to depth 0. The array guard that shielded the previous implementation was removed to fix collection discovery (issue #23) without anything replacing it. Walk in level order instead. Methods sit near the root while bulk data sits deeper, so breadth-first spends the budget on shallow objects first and truncates the deep data. Objects are claimed when queued so shared references are not scheduled twice, and exhausting maxVisitedObjects now only ends the scan and marks it truncated rather than tripping the hard discoveryStopped flag, which stays reserved for maxPatchedFunctions. Add a regression test covering a data-heavy first sibling; it fails against the depth-first implementation. Co-Authored-By: Claude Opus 5 <[email protected]>
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.
Follow-up to #24. That PR fixed collection discovery for issue #23, but introduced a regression that showed up on the first live capture.
The problem
Discovery walked the object graph depth-first against a global visit budget (
maxVisitedObjects: 2000). Hitting the budget set#discoveryStopped, which ended the entire scan — not just that branch. So the first object-valued own property ofstateObjcould consume the whole budget and starve every sibling enumerated after it.#24removed the guard that had been shielding the old implementation from this:The pre-#24 walk never entered arrays, so bulk data never cost anything. Once arrays and
Map/Setvalues became traversable — which is exactly what issue #23 needed — nothing replaced that protection.Observed on rss-feed (
_storeholds aMapof 27 feed sources, each with an array of parsed items): only_store.*got instrumented._indicator.*,_poller.*and_repository.*were never reached, and because the calling functions were missing, every event collapsed todepth: 0— the call tree, and with it the Self-vs-Total distinction, was gone.Reproduced synthetically at realistic proportions (27 sources × 40 items, default limits):
_store_indicator_poller_repositoryThe fix
Walk in level order. Methods sit near the root while bulk data (cached items, parsed entries) sits deeper, so breadth-first spends the budget on shallow objects first and truncates the deep data instead.
truncatedis stilltruein both rows above — the difference is what gets cut.#patchTreeis replaced by#patchGraph(level-order queue) +#visitObject(single-node work). The four walk methods no longer recurse; they receive anenqueuecallback.maxVisitedObjectsnow only ends the loop and marks the scan truncated. The hard#discoveryStoppedflag stays reserved formaxPatchedFunctions, where a global stop is the intended behaviour.No changes to
#patchObjector the restore path — wrapper installation and descriptor restoration are untouched.Notes for the reviewer
testDataHeavySiblingDoesNotStarvePeersmirrors the rss-feed shape withmaxVisitedObjects: 30so the budget genuinely runs out. I verified it fails against the depth-first implementation — otherwise it would not be catching anything._store._sources.values. The old code shadow-patchedMap.prototype.valuesonto the instance, which measures the GJS runtime rather than the extension._indicator.*,_poller.*,_repository.*and, importantly, non-zerodepthvalues.🤖 Generated with Claude Code