-
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 all commits
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", "10", "100", "1000"}) | ||
private int reactorLeadingLines; | ||
|
||
@Param({"0", "10", "100", "1000"}) | ||
private int trailingLines; | ||
|
||
private String stackTrace; | ||
|
||
@Setup(Level.Iteration) | ||
public void setup() { | ||
stackTrace = createLargeStackTrace(reactorLeadingLines, trailingLines); | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
@Benchmark | ||
public String measureThroughput() { | ||
return Traces.extractOperatorAssemblyInformation(stackTrace); | ||
} | ||
|
||
private static String createLargeStackTrace(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."; | ||
|
||
/** | ||
* 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.Mono.onAssembly") | ||
|| stackTraceRow.equals("reactor.core.publisher.Flux.onAssembly") | ||
Comment on lines
59
to
60
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 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 |
||
|| 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("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. |
||
} | ||
|
||
/** | ||
|
@@ -129,48 +128,92 @@ static boolean isUserCode(String line) { | |
* from the assembly stack trace. | ||
*/ | ||
static String[] extractOperatorAssemblyInformationParts(String source) { | ||
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); | ||
Comment on lines
130
to
+131
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}; | ||
} | ||
Comment on lines
+140
to
143
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>() { | ||
Comment on lines
+172
to
+178
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() { | ||
if (index >= source.length()) { | ||
return null; | ||
} | ||
|
||
while (index < source.length()) { | ||
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.