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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize observability and debugging experience #3901
Optimize observability and debugging experience #3901
Changes from 4 commits
518d059
a67e27c
c3e519a
6f7d079
7c8ce7b
02fa17e
36b3845
9cef75f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the private constant below the package-private ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The largest improvements in this PR come from these changes.
Key contributors to performance of the old implementation:
String#split
accepts a regular expression. We're no longer performing the comparatively expensive operation of compiling regular expressions.String#split
allocates an array and substrings proportional to the provided input, covering a potentially large part of the input that does not at all influence the result of this method.Stream
operation likewise processes irrelevant lines, and allocates a potentially large list.The new implementation instead lazily iterates over the input, processing only relevant lines, and tracking only the two most-recently-seen lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some logic was moved around, but existing comments were relocated with it. This should aid review. It's nice that there was already good test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This manually-crafted iterator feels a bit like "coding like it's 1999", but I didn't find a less-verbose alternative that doesn't impact over-all code readability. (Had Guava been on the classpath, then I'd have opted to extend
AbstractIterator
.) Open to suggestions!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an outline of an idea to avoid using String and get the next line:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The match for the reactor package name can also be done in the same linear scanning manner. I wonder if it'd be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at this, and went down the rabbit hole. The TL;DR is that I added three commits that further improve performance:
String#join
, unrelated to your suggestion here.String#trim
, such that the original string's underlyingchar[]
is always reused, thanks to the implementation ofString#substring
. This is IIUC an alternative to your suggestion to useCharBuffer
; I couldn't use the latter, as it lacks operations such as.startsWith
andindexOf
.Substring
class and is "somehow" even more performant.Benchmark of the code on `main`
Benchmark of the already-reviewed code
Benchmark after the first improvement
Benchmark after the second improvement
Benchmark after the third improvement
So for the common (1, 1) case, we see the following timing and memory usage differences:
I can see how "Speedup 3" is controversial from a maintainability point of view. I guess the only way to justify it, is by realizing that we're dealing with a very hot codepath here (at least for Reactor Debug Agent users). Up to you :)