-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Scannable#name()
and related logic
#3901
base: main
Are you sure you want to change the base?
Changes from 4 commits
518d059
a67e27c
c3e519a
6f7d079
7c8ce7b
02fa17e
36b3845
9cef75f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
After
Note 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(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright (c) 2018-2023 VMware Inc. or its affiliates, All Rights Reserved. | ||
* 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.List; | ||
import java.util.Iterator; | ||
import java.util.NoSuchElementException; | ||
import java.util.function.Supplier; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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,9 +57,8 @@ 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.Mono.onAssembly") | ||
|| stackTraceRow.equals("reactor.core.publisher.Flux.onAssembly") | ||
|| stackTraceRow.equals("reactor.core.publisher.ParallelFlux.onAssembly") | ||
|| stackTraceRow.startsWith("reactor.core.publisher.Flux.onAssembly") | ||
|| stackTraceRow.startsWith("reactor.core.publisher.ParallelFlux.onAssembly") | ||
|| stackTraceRow.startsWith("reactor.core.publisher.SignalLogger") | ||
|| stackTraceRow.startsWith("reactor.core.publisher.FluxOnAssembly") | ||
|| stackTraceRow.startsWith("reactor.core.publisher.MonoOnAssembly.") | ||
|
@@ -103,7 +102,7 @@ static String extractOperatorAssemblyInformation(String source) { | |
} | ||
|
||
static boolean isUserCode(String line) { | ||
return !line.startsWith("reactor.core.publisher") || line.contains("Test"); | ||
return !line.startsWith(PUBLISHER_PACKAGE_PREFIX) || line.contains("Test"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that's ok, let's leave it as is. |
||
} | ||
|
||
/** | ||
|
@@ -129,48 +128,88 @@ static boolean isUserCode(String line) { | |
* from the assembly stack trace. | ||
*/ | ||
static String[] extractOperatorAssemblyInformationParts(String source) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check I hadn't reviewed it because the change should affect behavior, but pushed a proposal 👍 |
||
String[] uncleanTraces = source.split("\n"); | ||
final List<String> traces = Stream.of(uncleanTraces) | ||
.map(String::trim) | ||
.filter(s -> !s.isEmpty()) | ||
.collect(Collectors.toList()); | ||
Iterator<String> traces = trimmedNonemptyLines(source); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
The new implementation instead lazily iterates over the input, processing only relevant lines, and tracking only the two most-recently-seen lines. |
||
|
||
if (traces.isEmpty()) { | ||
if (!traces.hasNext()) { | ||
return new String[0]; | ||
} | ||
|
||
int i = 0; | ||
while (i < traces.size() && !isUserCode(traces.get(i))) { | ||
i++; | ||
} | ||
String prevLine = null; | ||
String currentLine = traces.next(); | ||
|
||
String apiLine; | ||
String userCodeLine; | ||
if (i == 0) { | ||
//no line was a reactor API line | ||
apiLine = ""; | ||
userCodeLine = traces.get(0); | ||
} | ||
else if (i == traces.size()) { | ||
//we skipped ALL lines, meaning they're all reactor API lines. We'll fully display the last one | ||
apiLine = ""; | ||
userCodeLine = traces.get(i-1).replaceFirst("reactor.core.publisher.", ""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and below: |
||
} | ||
else { | ||
//currently on user code line, previous one is API | ||
apiLine = traces.get(i - 1); | ||
userCodeLine = traces.get(i); | ||
if (isUserCode(currentLine)) { | ||
// No line is a Reactor API line. | ||
return new String[]{currentLine}; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
//now we want something in the form "Flux.map ⇢ user.code.Class.method(Class.java:123)" | ||
if (apiLine.isEmpty()) return new String[] { userCodeLine }; | ||
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; | ||
|
||
int linePartIndex = apiLine.indexOf('('); | ||
if (linePartIndex > 0) { | ||
apiLine = apiLine.substring(0, linePartIndex); | ||
return new String[]{dropPublisherPackagePrefix(apiLine), "at " + currentLine}; | ||
} | ||
} | ||
apiLine = apiLine.replaceFirst("reactor.core.publisher.", ""); | ||
|
||
return new String[] { apiLine, "at " + userCodeLine }; | ||
// 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. Choose a reason for hiding this commentThe 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 |
||
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. Choose a reason for hiding this commentThe 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: // 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. Choose a reason for hiding this commentThe 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 commentThe 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:
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 :) |
||
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; | ||
} | ||
}; | ||
} | ||
} |
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.