GROOVY-12333: findBalancedGroups: bound the cost of deeply nested input - #2872
Merged
Conversation
A few hundred KB of nested delimiters cost tens of seconds, then died - with a StackOverflowError if the stack ran out first, otherwise by exhausting the heap. Two independent quadratics, both in the nesting. Depth was assigned eagerly during assembly. find() builds bottom-up, so a node is attached to a new parent once per enclosing delimiter, and each attach re-walked the whole subtree beneath it to restamp depths - quadratic, and one stack frame per level. Depth is now resolved from the parent chain when first asked for and cached, along with every ancestor walked on the way, so resolving a whole tree costs O(n) and any node is O(1) once resolved. The walk is iterative, so nesting no longer consumes stack. A chain 20,000 deep goes from 3646ms to 51ms, and one 10,000 deep whose depths are never read from 853ms to 4ms. Attaching a node whose depth was already read caches it against a parent chain that is about to change, so the attach drops the cached depths beneath it. That walk is iterative too, prunes at any node whose depth was never read, and does not run during find(), which reads no depths while assembling. Each node also held its own copy of its matched text. Groups nest, so their spans overlap and those copies duplicate most of the document once per level: 64KB of nesting retained about 990MB. A node now keeps the source and slices on demand, which is what the start/end offsets it already carried were for, so retention is proportional to the input - the same 64KB now retains 2MB. Reading every group's text costs what it did; reading one group's text repeatedly now slices each time rather than returning a stored copy, about 2ns per call. The package-private six-argument constructor consequently takes the source rather than the already-sliced text, and its offsets index into it. Passing text detached from its offsets is no longer meaningful, so the range check that compared them is now a bounds check against the source. find() is the only production caller and always had the source to hand; it no longer allocates a substring per group. The source must be effectively immutable for as long as a node lives. find() already flattens its input to a String before matching, which guarantees that.
🚨 TestLens detected 3 failed tests 🚨Here is what you can do:
Test SummaryBuild and test / lts (17, windows-latest, 1) > :groovy-groovysh:test
🏷️ Commit: 3bcc275 Test FailuresGroovyPosixCommandsTest > headAndGrepAndWcStripControlCharactersFromNames() (:groovy-groovysh:test in Build and test / lts (17, windows-latest, 1))GroovyPosixCommandsTest > lsStripsControlCharactersFromFileNames() (:groovy-groovysh:test in Build and test / lts (17, windows-latest, 1))GroovyPosixCommandsTest > lsStripsControlCharactersFromSymlinkTargets() (:groovy-groovysh:test in Build and test / lts (17, windows-latest, 1))Rerun ControlsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2872 +/- ##
==================================================
+ Coverage 70.9621% 70.9736% +0.0115%
- Complexity 37181 37196 +15
==================================================
Files 1578 1578
Lines 135189 135215 +26
Branches 25038 25046 +8
==================================================
+ Hits 95933 95967 +34
+ Misses 30569 30562 -7
+ Partials 8687 8686 -1
🚀 New features to boost your workflow:
|
daniellansun
approved these changes
Sep 3, 2026
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
3 participants
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.
A few hundred KB of nested delimiters cost tens of seconds, then died - with a StackOverflowError if the stack ran out first, otherwise by exhausting the heap. Two independent quadratics, both in the nesting.
Depth was assigned eagerly during assembly. find() builds bottom-up, so a node is attached to a new parent once per enclosing delimiter, and each attach re-walked the whole subtree beneath it to restamp depths - quadratic, and one stack frame per level. Depth is now resolved from the parent chain when first asked for and cached, along with every ancestor walked on the way, so resolving a whole tree costs O(n) and any node is O(1) once resolved. The walk is iterative, so nesting no longer consumes stack. A chain 20,000 deep goes from 3646ms to 51ms, and one 10,000 deep whose depths are never read from 853ms to 4ms.
Attaching a node whose depth was already read caches it against a parent chain that is about to change, so the attach drops the cached depths beneath it. That walk is iterative too, prunes at any node whose depth was never read, and does not run during find(), which reads no depths while assembling.
Each node also held its own copy of its matched text. Groups nest, so their spans overlap and those copies duplicate most of the document once per level: 64KB of nesting retained about 990MB. A node now keeps the source and slices on demand, which is what the start/end offsets it already carried were for, so retention is proportional to the input - the same 64KB now retains 2MB. Reading every group's text costs what it did; reading one group's text repeatedly now slices each time rather than returning a stored copy, about 2ns per call.
The package-private six-argument constructor consequently takes the source rather than the already-sliced text, and its offsets index into it. Passing text detached from its offsets is no longer meaningful, so the range check that compared them is now a bounds check against the source. find() is the only production caller and always had the source to hand; it no longer allocates a substring per group.
The source must be effectively immutable for as long as a node lives. find() already flattens its input to a String before matching, which guarantees that.