-
Notifications
You must be signed in to change notification settings - Fork 38.3k
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
Add fast-path for no-args constructor in BeanUtils.instantiateClass #24104
Conversation
@sdeleuze Could you have a look into this? |
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 would like to have data points for Kotlin and Java via JMH to be able to decide if we integrate such change or not, could you please measure that and share the results?
@@ -182,11 +183,18 @@ | |||
try { | |||
ReflectionUtils.makeAccessible(ctor); | |||
if (KotlinDetector.isKotlinReflectPresent() && KotlinDetector.isKotlinType(ctor.getDeclaringClass())) { | |||
if (ObjectUtils.isEmpty(args)) { | |||
return KotlinDelegate.instantiateClass(ctor); |
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.
Given the fact that JetBrains/kotlin#2855 has been closed because in practice it brings no benefit, is that part still relevant from your POV and if yes, could you elaborate on why and provide a JMH benchmark to quantify the gains?
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.
Hi, here's the benchmark:
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
open class MyBenchmark {
private val noArgConstructor = TestClass1::class.java.getDeclaredConstructor()
private val constructor = TestClass2::class.java.getDeclaredConstructor(Int::class.java, String::class.java)
@Benchmark
fun emptyConstructor(): Any {
return BeanUtils.instantiateClass(noArgConstructor)
}
@Benchmark
fun nonEmptyConstructor(): Any {
return BeanUtils.instantiateClass(constructor, 1, "str")
}
class TestClass1()
class TestClass2(int: Int, string: String)
}
Results:
before
Benchmark Mode Cnt Score Error Units
MyBenchmark.emptyConstructor avgt 100 52,279 ? 1,089 ns/op
MyBenchmark.emptyConstructor:?gc.alloc.rate.norm avgt 100 140,815 ? 2,499 B/op
MyBenchmark.nonEmptyConstructor avgt 100 272,787 ? 3,971 ns/op
MyBenchmark.nonEmptyConstructor:?gc.alloc.rate.norm avgt 100 985,684 ? 4,364 B/op
after
Benchmark Mode Cnt Score Error Units
MyBenchmark.emptyConstructor avgt 100 50,635 ? 0,870 ns/op
MyBenchmark.emptyConstructor:?gc.alloc.rate.norm avgt 100 91,853 ? 2,329 B/op
MyBenchmark.nonEmptyConstructor avgt 100 265,335 ? 5,555 ns/op
MyBenchmark.nonEmptyConstructor:?gc.alloc.rate.norm avgt 100 988,885 ? 3,272 B/op
int constructorParamCount = ctor.getParameterCount(); | ||
Assert.isTrue(args.length <= constructorParamCount, "Can't specify more arguments than constructor parameters"); | ||
if (constructorParamCount == 0) { | ||
return ctor.newInstance(); |
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.
Could you please provide a JMH benchmark of the gains?
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.
Yeah, I'll try to. The immediate gain here is that we don't allocate a new array calling ctor.getParameterTypes()
for zero-arg constructor case
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 am still interested by concrete data points with JMH benchmarks before and after the proposed change, could you please provide that?
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 the benchmark:
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class MyBenchmark {
private Constructor<TestClass1> noArgConstructor;
private Constructor<TestClass2> constructor;
@Setup
public void setUp() throws NoSuchMethodException {
noArgConstructor = TestClass1.class.getDeclaredConstructor();
constructor = TestClass2.class.getDeclaredConstructor(int.class, String.class);
}
@Benchmark
public Object emptyConstructor() {
return BeanUtils.instantiateClass(noArgConstructor);
}
@Benchmark
public Object nonEmptyConstructor() {
return BeanUtils.instantiateClass(constructor, 1, "str");
}
static class TestClass1{
};
static class TestClass2 {
private final int value1;
private final String value2;
TestClass2(int value1, String value2) {
this.value1 = value1;
this.value2 = value2;
}
}
}
And results:
baseline
Benchmark Mode Cnt Score Error Units
MyBenchmark.emptyConstructor avgt 40 9,880 ± 0,630 ns/op
MyBenchmark.emptyConstructor:·gc.alloc.rate.norm avgt 40 32,003 ± 0,001 B/op
MyBenchmark.nonEmptyConstructor avgt 40 11,937 ± 0,131 ns/op
MyBenchmark.nonEmptyConstructor:·gc.alloc.rate.norm avgt 40 48,004 ± 0,001 B/op
patched
Benchmark Mode Cnt Score Error Units
MyBenchmark.emptyConstructor avgt 40 5,968 ± 0,112 ns/op
MyBenchmark.emptyConstructor:·gc.alloc.rate.norm avgt 40 16,002 ± 0,001 B/op
MyBenchmark.nonEmptyConstructor avgt 40 11,922 ± 0,255 ns/op
MyBenchmark.nonEmptyConstructor:·gc.alloc.rate.norm avgt 40 48,004 ± 0,001 B/op
@sdeleuze is there anything I could do about this PR? |
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.
@stsypanov Thanks for your patience. I am ok with the Java changes, but the Kotlin ones break some tests and introduce too much duplication. I have refined the implementation with sdeleuze@9d8a035, which also makes it slightly faster (the branch is available here). Could you please provide me your feedback on those changes?
For no-args constructor. See spring-projectsgh-24104
This refinement ensures the constructor is properly accessible, avoid duplicating current logic and provide a slightly faster implementation of the Kotlin codepath. See spring-projectsgh-24104
Commit 9a180d1 introduced Kotlin-based JMH benchmarks which prevent spring-beans from being imported into the Eclipse IDE. This commit ensures that src/jmh/kotlin is imported in Eclipse with the "test" attribute set. See spring-projectsgh-24104
For the cases when constructor has no args we could have fast-paths which is likely to improve start-up time as BeanUtils.instantiateClass is called at least once for each bean instantiation in application context.