Optimize Scannable#name() and related logic
#3901
+220
−68
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* | ||
| * Copyright (c) 2024 VMware Inc. or its affiliates, All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package reactor.core.publisher; | ||
|
|
||
| import java.util.concurrent.TimeUnit; | ||
| import org.openjdk.jmh.annotations.Benchmark; | ||
| import org.openjdk.jmh.annotations.BenchmarkMode; | ||
| import org.openjdk.jmh.annotations.Fork; | ||
| import org.openjdk.jmh.annotations.Level; | ||
| import org.openjdk.jmh.annotations.Measurement; | ||
| import org.openjdk.jmh.annotations.Mode; | ||
| import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
| import org.openjdk.jmh.annotations.Param; | ||
| import org.openjdk.jmh.annotations.Scope; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
| import org.openjdk.jmh.annotations.State; | ||
| import org.openjdk.jmh.annotations.Warmup; | ||
|
|
||
| @BenchmarkMode({Mode.AverageTime}) | ||
| @Warmup(iterations = 5, time = 5, timeUnit = TimeUnit.SECONDS) | ||
| @Measurement(iterations = 5, time = 5, timeUnit = TimeUnit.SECONDS) | ||
| @Fork(value = 1) | ||
| @OutputTimeUnit(TimeUnit.NANOSECONDS) | ||
| @State(Scope.Benchmark) | ||
| public class TracesBenchmark { | ||
|
Comment on lines
+33
to
+39
There was a problem hiding this comment. The configuration and setup for this benchmark match those of the existing benchmarks, including explicit specification of default parameters. Can be cleaned up a bit more if desired. As for the impact of this PR, the following shows the before- and after output of Before:AfterNote how performance of the new implementation is almost independent of the input, while the old implementation scales fairly poorly. |
||
| @Param({"0", "1", "2"}) | ||
| private int reactorLeadingLines; | ||
|
|
||
| @Param({"0", "1", "2"}) | ||
| private int trailingLines; | ||
|
|
||
| private String stackTrace; | ||
|
|
||
| @Setup(Level.Iteration) | ||
| public void setup() { | ||
| stackTrace = createStackTrace(reactorLeadingLines, trailingLines); | ||
| } | ||
|
|
||
| @SuppressWarnings("unused") | ||
| @Benchmark | ||
| public String measureThroughput() { | ||
| return Traces.extractOperatorAssemblyInformation(stackTrace); | ||
| } | ||
|
|
||
| private static String createStackTrace(int reactorLeadingLines, int trailingLines) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (int i = 0; i < reactorLeadingLines; i++) { | ||
| sb.append("\tat reactor.core.publisher.Flux.someOperation(Flux.java:42)\n"); | ||
| } | ||
| sb.append("\tat some.user.package.SomeUserClass.someOperation(SomeUserClass.java:1234)\n"); | ||
| for (int i = 0; i < trailingLines; i++) { | ||
| sb.append("\tat any.package.AnyClass.anyOperation(AnyClass.java:1)\n"); | ||
| } | ||
| return sb.toString(); | ||
| } | ||
| } | ||
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2018-2024 VMware Inc. or its affiliates, All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
|
|
@@ -16,11 +16,10 @@ | |
|
|
||
| package reactor.core.publisher; | ||
|
|
||
| import java.util.Iterator; | ||
| import java.util.NoSuchElementException; | ||
| import java.util.function.Supplier; | ||
| import reactor.util.annotation.Nullable; | ||
|
|
||
| /** | ||
| * Utilities around manipulating stack traces and displaying assembly traces. | ||
|
|
@@ -29,6 +28,7 @@ | |
| * @author Sergei Egorov | ||
| */ | ||
| final class Traces { | ||
| private static final String PUBLISHER_PACKAGE_PREFIX = "reactor.core.publisher."; | ||
|
There was a problem hiding this comment. Let's move the private constant below the package-private ones. |
||
|
|
||
| /** | ||
| * If set to true, the creation of FluxOnAssembly will capture the raw stacktrace | ||
|
|
@@ -57,7 +57,6 @@ final class Traces { | |
| static boolean shouldSanitize(String stackTraceRow) { | ||
| return stackTraceRow.startsWith("java.util.function") | ||
| || stackTraceRow.startsWith("reactor.core.publisher.Mono.onAssembly") | ||
| || stackTraceRow.equals("reactor.core.publisher.Flux.onAssembly") | ||
|
There was a problem hiding this comment. This line removal is unrelated to the rest of the PR. The dropped line is redundant, as it is preceded by a predicate that matches in strictly more contexts. Question: should the There was a problem hiding this comment. I think they can all use There was a problem hiding this comment. Alright, will do (and update the PR description to match this additional change). |
||
| || stackTraceRow.equals("reactor.core.publisher.ParallelFlux.onAssembly") | ||
| || stackTraceRow.startsWith("reactor.core.publisher.SignalLogger") | ||
|
|
@@ -103,7 +102,7 @@ static String extractOperatorAssemblyInformation(String source) { | |
| } | ||
|
|
||
| static boolean isUserCode(String line) { | ||
| return !line.startsWith(PUBLISHER_PACKAGE_PREFIX) || line.contains("Test"); | ||
|
There was a problem hiding this comment. The constant has a trailing dot. Afaik that doesn't matter, as each line should include a class name, too. There was a problem hiding this comment. In case at any point future maintainers decide to add a package that begins with reactor.core.publisher but does not end with a dot I think we can remove the dot from the prefix. Unless it's needed? There was a problem hiding this comment. Good question. So if I drop the dot from the constant, then the I can of course also just revert the change on this line, though given that currently there's no There was a problem hiding this comment. No, that's ok, let's leave it as is. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -129,48 +128,92 @@ static boolean isUserCode(String line) { | |
| * from the assembly stack trace. | ||
| */ | ||
| static String[] extractOperatorAssemblyInformationParts(String source) { | ||
|
There was a problem hiding this comment. Since you're changing this mechanism a lot, can you also review the javadoc and update? E.g. There was a problem hiding this comment. Check I hadn't reviewed it because the change should affect behavior, but pushed a proposal 👍 |
||
| Iterator<String> traces = trimmedNonemptyLines(source); | ||
|
There was a problem hiding this comment. The largest improvements in this PR come from these changes. Key contributors to performance of the old implementation:
The new implementation instead lazily iterates over the input, processing only relevant lines, and tracking only the two most-recently-seen lines. |
||
|
|
||
| if (!traces.hasNext()) { | ||
| return new String[0]; | ||
| } | ||
|
|
||
| String prevLine = null; | ||
| String currentLine = traces.next(); | ||
|
|
||
|
There was a problem hiding this comment. Here and below: |
||
| if (isUserCode(currentLine)) { | ||
| // No line is a Reactor API line. | ||
| return new String[]{currentLine}; | ||
| } | ||
|
There was a problem hiding this comment. 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. |
||
|
|
||
| while (traces.hasNext()) { | ||
| prevLine = currentLine; | ||
| currentLine = traces.next(); | ||
|
|
||
| if (isUserCode(currentLine)) { | ||
| // Currently on user code line, previous one is API. Attempt to create something in the form | ||
| // "Flux.map ⇢ user.code.Class.method(Class.java:123)". | ||
| int linePartIndex = prevLine.indexOf('('); | ||
| String apiLine = linePartIndex > 0 ? | ||
| prevLine.substring(0, linePartIndex) : | ||
| prevLine; | ||
|
|
||
| return new String[]{dropPublisherPackagePrefix(apiLine), "at " + currentLine}; | ||
| } | ||
| } | ||
|
|
||
| // We skipped ALL lines, meaning they're all Reactor API lines. We'll fully display the last | ||
| // one. | ||
| return new String[]{dropPublisherPackagePrefix(currentLine)}; | ||
| } | ||
|
|
||
| private static String dropPublisherPackagePrefix(String line) { | ||
| return line.startsWith(PUBLISHER_PACKAGE_PREFIX) | ||
| ? line.substring(PUBLISHER_PACKAGE_PREFIX.length()) | ||
| : line; | ||
| } | ||
|
|
||
| /** | ||
| * Returns an iterator over all trimmed non-empty lines in the given source string. | ||
| * | ||
| * @implNote This implementation attempts to minimize allocations. | ||
| */ | ||
| private static Iterator<String> trimmedNonemptyLines(String source) { | ||
| return new Iterator<String>() { | ||
|
There was a problem hiding this comment. 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 |
||
| private int index = 0; | ||
| @Nullable | ||
| private String next = getNextLine(); | ||
|
|
||
| @Override | ||
| public boolean hasNext() { | ||
| return next != null; | ||
| } | ||
|
|
||
| @Override | ||
| public String next() { | ||
| String current = next; | ||
| if (current == null) { | ||
| throw new NoSuchElementException(); | ||
| } | ||
| next = getNextLine(); | ||
| return current; | ||
| } | ||
|
|
||
| @Nullable | ||
| private String getNextLine() { | ||
|
There was a problem hiding this comment. Here's an outline of an idea to avoid using String and get the next line: // Assume the entire input (String source) is wrapped in CharBuffer
CharBuffer cb = CharBuffer.wrap(source);
private CharBuffer getNextLine() {
int i = 0;
while (i < cb.length()) {
if (Character.isWhitespace(cb.charAt(i))) continue;
int end = i + 1;
while (end < cb.length() && cb.charAt(end) != '\n') {
end++;
}
CharBuffer line = cb.subSequence(i, end);
i = end + 1;
return line;
}
}There was a problem hiding this comment. 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. 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:
Benchmark of the code on `main`Benchmark of the already-reviewed codeBenchmark after the first improvementBenchmark after the second improvementBenchmark after the third improvementSo 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 :) |
||
| if (index >= source.length()) { | ||
| return null; | ||
| } | ||
|
There was a problem hiding this comment. Just realized that we can drop this case 👁️ There was a problem hiding this comment. Ah, yes. I didn't finish reviewing yesterday and it seems we noticed the same thing :) |
||
|
|
||
| while (index < source.length()) { | ||
Stephan202 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| int end = source.indexOf('\n', index); | ||
| if (end == -1) { | ||
| end = source.length(); | ||
| } | ||
| String line = source.substring(index, end).trim(); | ||
| index = end + 1; | ||
| if (!line.isEmpty()) { | ||
| return line; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| }; | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
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 and in
MonoCallableBenchmark: as stated, these are unrelated improvements. Can be pulled into a separate PR if desired.