-
Notifications
You must be signed in to change notification settings - Fork 41
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
#130: Support benchmarks having @State annotations in parent classes #193
base: master
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,34 @@ | ||
package test | ||
|
||
import org.openjdk.jmh.annotations.* | ||
import java.util.concurrent.TimeUnit | ||
import kotlin.math.cos | ||
import kotlin.math.sqrt | ||
|
||
@State(Scope.Benchmark) | ||
@Fork(1) | ||
@Warmup(iterations = 0) | ||
@Measurement(iterations = 1, time = 1, timeUnit = TimeUnit.SECONDS) | ||
abstract class BaseBenchmark { | ||
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. Can the base class declare benchmark methods as well? 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. As to why? -> It doesn't make sense to run the same benchmark again if two or more classes are inheriting from same non-final class. 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 can think of the following use case:
|
||
|
||
protected lateinit var data: TestData | ||
|
||
@Setup | ||
fun setUp() { | ||
data = TestData(50.0) | ||
} | ||
} | ||
|
||
class TestInheritedBenchmark: BaseBenchmark() { | ||
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. Can a 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. Could you expand a bit on your question? 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 am referring to cases of multi-hierarchy benchmark classes, within which multiple classes define benchmark methods. |
||
@Benchmark | ||
fun sqrtBenchmark(): Double { | ||
return sqrt(data.value) | ||
} | ||
|
||
@Benchmark | ||
fun cosBenchmark(): Double { | ||
return cos(data.value) | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package kotlinx.benchmark.gradle | |
import com.squareup.kotlinpoet.* | ||
import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy | ||
import org.jetbrains.kotlin.descriptors.* | ||
import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor | ||
import org.jetbrains.kotlin.name.* | ||
import org.jetbrains.kotlin.resolve.* | ||
import org.jetbrains.kotlin.resolve.annotations.* | ||
|
@@ -99,21 +100,99 @@ class SuiteSourceGenerator(val title: String, val module: ModuleDescriptor, val | |
|
||
private fun processPackage(module: ModuleDescriptor, packageView: PackageViewDescriptor) { | ||
for (packageFragment in packageView.fragments.filter { it.module == module }) { | ||
DescriptorUtils.getAllDescriptors(packageFragment.getMemberScope()) | ||
val classDescriptors = DescriptorUtils.getAllDescriptors(packageFragment.getMemberScope()) | ||
.filterIsInstance<ClassDescriptor>() | ||
|
||
val inheritableClassDescriptors = classDescriptors.filter { it.modality != Modality.FINAL } | ||
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. WDYT about using "non-final" instead of "inheritable"? 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. Sounds good. |
||
|
||
// Abstract classes which are annotated directly. | ||
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. It seems to me that 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. Oops yes old comment. Will fix! |
||
val directlyAnnotatedInheritableClassDescriptors = inheritableClassDescriptors | ||
.filter { it.annotations.any { it.fqName.toString() == stateAnnotationFQN } } | ||
.filter { it.modality != Modality.ABSTRACT } | ||
.forEach { | ||
generateBenchmark(it) | ||
|
||
// Contains both directly and indirectly annotated ClassDescriptors. | ||
val annotatedInheritableClassDescriptors = inheritableClassDescriptors | ||
.filter { inheritableClass -> | ||
directlyAnnotatedInheritableClassDescriptors.forEach { annotatedInheritableClass -> | ||
if (inheritableClass.isSubclassOf(annotatedInheritableClass)) { | ||
// If any benchmark class is not annotated but extended with an abstract class | ||
// annotated with @State, it should be included when generating benchmarks. | ||
return@filter true | ||
} | ||
} | ||
return@filter false | ||
} | ||
|
||
val annotatedClassDescriptors = mutableListOf<AnnotatedClassDescriptor>() | ||
.apply { | ||
classDescriptors | ||
.filter { it.modality != Modality.ABSTRACT } | ||
.forEach { | ||
if (it.annotations.any { | ||
annotationDescriptor -> annotationDescriptor.fqName.toString() == stateAnnotationFQN | ||
}) { | ||
add(AnnotatedClassDescriptor(it)) | ||
} else { | ||
val parent = it.getParentAnnotated(annotatedInheritableClassDescriptors) | ||
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. Could 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. Huh after another thought, I think |
||
if (parent != null) { | ||
add(AnnotatedClassDescriptor(it, parent)) | ||
} | ||
} | ||
} | ||
} | ||
|
||
annotatedClassDescriptors.forEach { | ||
generateBenchmark(it) | ||
} | ||
} | ||
|
||
for (subpackageName in module.getSubPackagesOf(packageView.fqName, MemberScope.ALL_NAME_FILTER)) { | ||
processPackage(module, module.getPackage(subpackageName)) | ||
} | ||
} | ||
|
||
private fun generateBenchmark(original: ClassDescriptor) { | ||
/** @param annotatedInheritableClassDescriptors descriptors of classes which are inheritable and directly annotated. | ||
* with [stateAnnotationFQN]. | ||
* @return Top most parent class descriptor which was annotated with [stateAnnotationFQN] and inherited by `this` class | ||
* descriptor or null if none of its parent classes are not annotated.*/ | ||
private fun ClassDescriptor.getParentAnnotated( | ||
annotatedInheritableClassDescriptors: List<ClassDescriptor> | ||
): ClassDescriptor? { | ||
annotatedInheritableClassDescriptors.forEach { annotatedAbstractClass -> | ||
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. Can find be used to simplify the code? |
||
if (this.isSubclassOf(annotatedAbstractClass)) { | ||
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. Could it be that there are multiple superclasses for 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 was expecting this! So as for phase-1 I was just looking out for bare bones implementation which means this here just a base setup. Currently this function picks the first superclass that is directly annotated. Now there are some cases:
What should be the route to follow here? 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.
It depends on how JMH handles this aspect. Being consistent with that library is important, since it runs the benchmarks on the JVM. Achieving consistent behavior across other platforms would be ideal. It is worth mentioning that we do not aim to replicate all of JMH's sophistication, but when something is not supported in non-JVM targets, we would like to issue a warning (or throw an exception) about it. 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 would greatly appreciate it if you could check how JMH handles this case. It would greatly help us decide which route we should take. |
||
return annotatedAbstractClass | ||
} | ||
} | ||
return null | ||
} | ||
|
||
/** @param parentAnnotatedClassDescriptor if null, [original] is directly annotated with [stateAnnotationFQN].*/ | ||
private data class AnnotatedClassDescriptor( | ||
val original: ClassDescriptor, | ||
val parentAnnotatedClassDescriptor: ClassDescriptor? = null | ||
) | ||
|
||
private fun ClassDescriptor.getParameterProperties(): List<PropertyDescriptor> = | ||
DescriptorUtils.getAllDescriptors(this.unsubstitutedMemberScope) | ||
.filterIsInstance<PropertyDescriptor>() | ||
.filter { it.annotations.any { it.fqName.toString() == paramAnnotationFQN } } | ||
|
||
private fun ClassDescriptor.getMeasureAnnotation(): AnnotationDescriptor? = | ||
this.annotations.singleOrNull { it.fqName.toString() == measureAnnotationFQN } | ||
|
||
private fun ClassDescriptor.getWarmupAnnotation(): AnnotationDescriptor? = | ||
this.annotations.singleOrNull { it.fqName.toString() == warmupAnnotationFQN } | ||
|
||
private fun ClassDescriptor.getOutputTimeAnnotation(): AnnotationDescriptor? = | ||
this.annotations.singleOrNull { it.fqName.toString() == outputTimeAnnotationFQN } | ||
|
||
private fun ClassDescriptor.getModeAnnotation(): AnnotationDescriptor? = | ||
this.annotations.singleOrNull { it.fqName.toString() == modeAnnotationFQN } | ||
|
||
|
||
private fun generateBenchmark(classDescriptor: AnnotatedClassDescriptor) { | ||
val original = classDescriptor.original | ||
val parent = classDescriptor.parentAnnotatedClassDescriptor | ||
|
||
val originalPackage = original.fqNameSafe.parent() | ||
val originalName = original.fqNameSafe.shortName() | ||
val originalClass = ClassName(originalPackage.toString(), originalName.toString()) | ||
|
@@ -125,14 +204,13 @@ class SuiteSourceGenerator(val title: String, val module: ModuleDescriptor, val | |
val functions = DescriptorUtils.getAllDescriptors(original.unsubstitutedMemberScope) | ||
.filterIsInstance<FunctionDescriptor>() | ||
|
||
val parameterProperties = DescriptorUtils.getAllDescriptors(original.unsubstitutedMemberScope) | ||
.filterIsInstance<PropertyDescriptor>() | ||
.filter { it.annotations.any { it.fqName.toString() == paramAnnotationFQN } } | ||
val parameterProperties = original.getParameterProperties() | ||
|
||
val measureAnnotation = original.annotations.singleOrNull { it.fqName.toString() == measureAnnotationFQN } | ||
val warmupAnnotation = original.annotations.singleOrNull { it.fqName.toString() == warmupAnnotationFQN } | ||
val outputTimeAnnotation = original.annotations.singleOrNull { it.fqName.toString() == outputTimeAnnotationFQN } | ||
val modeAnnotation = original.annotations.singleOrNull { it.fqName.toString() == modeAnnotationFQN } | ||
// original's annotations are given higher priority than parent's annotation. | ||
val measureAnnotation = original.getMeasureAnnotation() ?: parent?.getMeasureAnnotation() | ||
val warmupAnnotation = original.getWarmupAnnotation() ?: parent?.getWarmupAnnotation() | ||
val outputTimeAnnotation = original.getOutputTimeAnnotation() ?: parent?.getOutputTimeAnnotation() | ||
val modeAnnotation = original.getModeAnnotation() ?: parent?.getModeAnnotation() | ||
|
||
val outputTimeUnitValue = outputTimeAnnotation?.argumentValue("value") as EnumValue? | ||
val outputTimeUnit = outputTimeUnitValue?.enumEntryName?.toString() | ||
|
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 add some integration tests instead?