Skip to content

TracerouteEngineImpl: limit number of traces materialized from TraceDag.#8939

Merged
dhalperi merged 3 commits intomasterfrom
iter-traces
Feb 1, 2024
Merged

TracerouteEngineImpl: limit number of traces materialized from TraceDag.#8939
dhalperi merged 3 commits intomasterfrom
iter-traces

Conversation

@anothermattbrown
Copy link
Copy Markdown
Contributor

TraceDags have been observed to contain huge numbers of traces, too many to materialize into a list. Limiting to 10k for now. Rewrote TraceDagImpl to use iterators internally, as limit() wasn't working on the previous implementation based on recursive streams flattened with flatMap. Something about the stream implementation materializes many more traces than the limit, which wastes compute time and risks OOM. Rewriting using the same style of code but based on iterators solved the problem.

TraceDags have been observed to contain huge numbers of traces, too many to materialize into a list.
Limiting to 10k for now. Rewrote TraceDagImpl to use iterators internally, as limit() wasn't working
on the previous implementation based on recursive streams flattened with flatMap. Something about the
stream implementation materializes many more traces than the limit, which wastes compute time and risks
OOM. Rewriting using the same style of code but based on iterators solved the problem.
@batfish-bot
Copy link
Copy Markdown

This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 30, 2024

Codecov Report

Merging #8939 (12b010f) into master (ec8fb08) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 86.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8939      +/-   ##
==========================================
- Coverage   72.53%   72.52%   -0.01%     
==========================================
  Files        3321     3323       +2     
  Lines      169579   169605      +26     
  Branches    19912    19914       +2     
==========================================
+ Hits       123003   123009       +6     
- Misses      37415    37432      +17     
- Partials     9161     9164       +3     
Files Coverage Δ
...n/java/org/batfish/common/traceroute/TraceDag.java 100.00% <100.00%> (ø)
...va/org/batfish/common/traceroute/TraceDagImpl.java 87.30% <100.00%> (+0.86%) ⬆️
...va/org/batfish/dataplane/TracerouteEngineImpl.java 96.87% <100.00%> (+0.32%) ⬆️
.../java/org/batfish/common/util/FlatMapIterator.java 77.77% <77.77%> (ø)

... and 4 files with indirect coverage changes

Copy link
Copy Markdown
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown)


projects/batfish/src/main/java/org/batfish/dataplane/TracerouteEngineImpl.java line 69 at r1 (raw file):

                        .getValue()
                        .getTraces()
                        .limit(10000)

would probably 1/ extract to a constant in the interface 2/ add a limited variant in the interface where this const is the default 3/ migrate interface default impl to be the limited version of the constant.

Code quote:

10000

Copy link
Copy Markdown
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown)

@dhalperi dhalperi merged commit 6f73594 into master Feb 1, 2024
@dhalperi dhalperi deleted the iter-traces branch February 1, 2024 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants