Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

07jasjeet
Copy link

@07jasjeet 07jasjeet commented Mar 13, 2024

Targets #130

Changelog

  1. Whenever a parent of a child class is directly or indirectly annotated with @State annotation, our child class will also be considered when generating benchmarks.
  2. Supplimentary annotations like @Measurement @Warmup @OutputTime @BenchmarkMode are considered in this order: child -> top most annotated parent

Note: This is a phase-1 implementation, with just the basic idea laid out.

Now, if a parent class is abstract, that child class will also be benchmarked, with annotations derived in order: child ?: parent.
1. Refactored code (cleanup)
2. All classes except final are considered when considering parent annotated class.
@qurbonzoda qurbonzoda self-requested a review March 15, 2024 13:09

val inheritableClassDescriptors = classDescriptors.filter { it.modality != Modality.FINAL }

// Abstract classes which are annotated directly.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that directlyAnnotatedInheritableClassDescriptors includes more than just abstract classes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops yes old comment. Will fix!

.filterIsInstance<ClassDescriptor>()

val inheritableClassDescriptors = classDescriptors.filter { it.modality != Modality.FINAL }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about using "non-final" instead of "inheritable"?
The term "non-final" just sounds more familiar to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

}) {
add(AnnotatedClassDescriptor(it))
} else {
val parent = it.getParentAnnotated(annotatedInheritableClassDescriptors)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could directlyAnnotatedInheritableClassDescriptors be used here?
I am struggling to understand the need for annotatedInheritableClassDescriptors. Cloud you please clarify?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh after another thought, I think directlyAnnotatedInheritableClassDescriptors should be used here. Will do!

private fun ClassDescriptor.getParentAnnotated(
annotatedInheritableClassDescriptors: List<ClassDescriptor>
): ClassDescriptor? {
annotatedInheritableClassDescriptors.forEach { annotatedAbstractClass ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can find be used to simplify the code?

Copy link
Collaborator

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?

annotatedInheritableClassDescriptors: List<ClassDescriptor>
): ClassDescriptor? {
annotatedInheritableClassDescriptors.forEach { annotatedAbstractClass ->
if (this.isSubclassOf(annotatedAbstractClass)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that there are multiple superclasses for this class descriptor?

Copy link
Author

Choose a reason for hiding this comment

The 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:

  1. Simple straight forward: Only one annotated superclass.
  2. Multiple superclasses but only one is annotated.
  3. Multiple superclasses and intermediaries (super class of this but sub class of the top most annotated parent class) are annotated as well.
  4. Multiple superclasses and intermediaries with annotations E.x., @Warmup, @Measurement etc without @State.

What should be the route to follow here?
For 4th point, my suggestion is that independent annotations should be ignored. For the 3rd point, I would suggest nearest annotated parent. 1st and 2nd are easily dealt with.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be the route to follow here?

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

@Fork(1)
@Warmup(iterations = 0)
@Measurement(iterations = 1, time = 1, timeUnit = TimeUnit.SECONDS)
abstract class BaseBenchmark {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the base class declare benchmark methods as well?

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of the following use case:

import kotlinx.benchmark.*

@State(Scope.Benchmark)
@Measurement(iterations = 7, time = 1, timeUnit = BenchmarkTimeUnit.SECONDS)
@Warmup(iterations = 7, time = 1, timeUnit = BenchmarkTimeUnit.SECONDS)
@OutputTimeUnit(BenchmarkTimeUnit.MILLISECONDS)
abstract class IterationBenchmarkBase {
    abstract val list: List<String>

    @Param("10", "100")
    var size: Int = 0

    @Benchmark
    fun iterationBenchmark(bh: Blackhole) {
        for (element in list) {
            bh.consume(element)
        }
    }
}

class ArrayListIterationBenchmark : IterationBenchmarkBase() {
    override val list: List<String>
        get() = ArrayList<String>(size).apply { fill(size) }
}

class ArrayDequeIterationBenchmark : IterationBenchmarkBase() {
    override val list: List<String>
        get() = ArrayDeque<String>(size).apply { fill(size) }
}

private fun MutableList<String>.fill(size: Int) {
    addAll((1..size).map { it.toString() })
}

}
}

class TestInheritedBenchmark: BaseBenchmark() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a TestInheritedBenchmark subclass declare benchmark methods as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand a bit on your question?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

@qurbonzoda
Copy link
Collaborator

Note: This is a phase-1 implementation, with just the basic idea laid out.

I would appreciate it if you could clarify what exactly you think should be included in phase 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants