[SwiftWarningControl] Fix exponential queue growth in addWarningGroupControls#3344
Open
inju2403 wants to merge 2 commits into
Open
[SwiftWarningControl] Fix exponential queue growth in addWarningGroupControls#3344inju2403 wants to merge 2 commits into
inju2403 wants to merge 2 commits into
Conversation
…Controls The BFS over the inheritance tree used `Array.removeFirst()` (O(N) per call) and only checked `processedGroups` at enqueue time, so a sub-group reachable from multiple already-queued parents would be enqueued (and re-processed) multiple times. On diamond-shaped inheritance DAGs the queue grew as 2^depth, so even modest depths (>= 10) effectively hung. Switch to an index-based queue and de-dupe at dequeue time, making the loop O(N) in the number of distinct sub-groups. Adds a performance test exercising chain, fan, and diamond-DAG workloads.
Exercises the path that previously could re-process a sub-group reachable from multiple parents, complementing the performance test (which proves the queue no longer explodes).
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.
Problem
addWarningGroupControlswalks the diagnostic-group inheritance tree via BFS to propagate a control to every transitive sub-group. The implementation has two issues:Array.removeFirst()shifts the rest of the queue on every dequeue.processedGroupsis only checked when enqueuing sub-groups. A group reachable from more than one already-queued parent is enqueued multiple times, has its controlre-applied each time, and (more importantly) re-expands its own sub-groups each time it's processed.
For a diamond-shaped inheritance DAG (each level has 2 nodes, both pointing to both nodes of the next level), the queue grows as 2^depth, so even modest depths (>= 10) effectively hang.
Fix
Array.removeFirst().processedGroups.insert(_).inserted.Enqueue-time filtering is kept as an optimization, but it isn't sufficient on its own: between the moment a sub-group is filtered and the moment it would be dequeued, another path through the inheritance tree can still enqueue the same sub-group. The
processedGroups.insert(_).insertedcheck at dequeue is what actually guarantees each group is processed at most once.The loop now processes each distinct sub-group exactly once (the enqueue-time filter still admits duplicates that the dequeue-time check then skips, so the bound is O(N + E) where E is the number of edges in the inheritance graph). This matches the
insert(_).insertedpattern already used elsewhere in this module (DiagnosticGroupInheritanceTree.hasCycle) and elsewhere in the repo (SwiftOperators/PrecedenceGraph,SwiftSyntax/Raw/RawSyntaxArena), and does not introduce any new dependencies.Note: I left the
enabledGroups.insert(diagnosticGroupIdentifier)line referencing the outer loop variable rather thangroupIdentifier. Whether sub-groups that inherit a non-ignored control should also appear inenabledGroupslooks like a separate semantic question that I'd rather not bundle into this perf fix -- happy to address it in a follow-up if reviewers prefer that semantic.Measurements
Tests/PerformanceTest/WarningControlRegionsPerformanceTests.swiftmeasures three workloads using_InstructionCounter:A regression test (
testDiamondSubGroupInheritance) is added to the default test suite to pin down the expected behavior on a diamond-shaped inheritance graph (perf tests are gated behindSWIFTSYNTAX_ENABLE_LONG_TESTS).All 27 existing
SwiftWarningControlTestcases pass.