-
-
Notifications
You must be signed in to change notification settings - Fork 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
Improved TestStep creation performance #2667
Conversation
Changed implementation to use variant 6 proposed in cucumber#2666 .
Added test cases for `TestAbortedExceptions` `Predicate` with one negative example and two positive examples.
Corrected reference to non-existing code.
Added info in Unreleased section and corrected typo in Cucumber 7.10.0 section.
@@ -26,20 +26,18 @@ final class TestAbortedExceptions { | |||
|
|||
static Predicate<Throwable> createIsTestAbortedExceptionPredicate() { | |||
ClassLoader defaultClassLoader = ClassLoaders.getDefaultClassLoader(); | |||
return Arrays.stream(TEST_ABORTED_EXCEPTIONS) | |||
.flatMap(s -> { | |||
return throwable -> Arrays.stream(TEST_ABORTED_EXCEPTIONS) |
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.
Would a for loop 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 benchmarked the following code:
static Predicate<Throwable> createIsTestAbortedExceptionPredicate7() {
ClassLoader defaultClassLoader = ClassLoaders.getDefaultClassLoader();
return throwable -> {
for (String s : TEST_ABORTED_EXCEPTIONS) {
try {
Class<?> aClass = defaultClassLoader.loadClass(s);
if (aClass.isInstance(throwable)) {
return true;
}
} catch (Throwable t) {
rethrowIfUnrecoverable(t);
log.debug(t,
() -> String.format(
"Failed to load class %s: will not be supported for aborted executions.", s));
}
}
return false;
};
}
The JMH benchmark says the variant 7 (the one with a loop) is slightly slower than the proposed version (variant 6), but not significantly :
Benchmark Mode Cnt Score Error Units
TestAbortedExceptionsBenchmark.createIsTestAbortedExceptionPredicate5 thrpt 25 264824252,996 ± 4775342,657 ops/s
TestAbortedExceptionsBenchmark.createIsTestAbortedExceptionPredicate6 thrpt 25 259886661,320 ± 1766902,768 ops/s
TestAbortedExceptionsBenchmark.createIsTestAbortedExceptionPredicate7 thrpt 25 254627500,665 ± 12269987,245 ops/s
I think the code readability is better with the Stream+anyMatch than with the loop, so I would prefer to keep the proposed version..
If you run |
Corrected format.
Corrected format.
removed unused import
Codecov Report
@@ Coverage Diff @@
## main #2667 +/- ##
============================================
- Coverage 84.82% 84.82% -0.01%
+ Complexity 2695 2694 -1
============================================
Files 322 322
Lines 9576 9574 -2
Branches 907 907
============================================
- Hits 8123 8121 -2
Misses 1121 1121
Partials 332 332
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hi @jkronegg, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
Cheers! |
🤔 What's changed?
Refactored
TestAbortedExceptions
Predicate
creation.⚡️ What's your motivation?
Performance gain (
Predicate
creation is about 50'000 times faster).Fixes #2666
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
Nothing.
📋 Checklist: