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

Optimizes IonValue.getType() to avoid vtable/itable lookups when invoked on IonStruct values. #928

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

tgregg
Copy link
Contributor

@tgregg tgregg commented Aug 29, 2024

Description of changes:
Also adds a 'jmh' target that allows us to add microbenchmarks for targeted performance testing. These benchmarks are run using ./gradlew jmh. Example output:

Benchmark                                  Mode  Cnt    Score   Error  Units
IonValueGetTypeBenchmark.getTypeContainer  avgt    5  469.625 ± 5.799  ns/op
IonValueGetTypeBenchmark.ionValueGetType   avgt    5    7.863 ± 0.139  ns/op

This is optional and can be removed from this PR if reviewers have objections. I think it's nice because it reduces the effort required to add new JMH benchmarks that are more targeted than the ion-java-benchmark-cli. Note that we followed this same pattern in ion-java-path-extraction. New benchmarks need not even be committed, but having the JMH framework committed allows for quick local experimentation. I'm proposing to commit the new IonValueGetTypeBenchmark mostly as an example.

As for the proposed change, the existing implementation of IonValue.getType() is an interface method with 14 implementations, requiring an itable lookup to invoke. This has been shown to be much slower than invoking a final method or an abstract method with two or fewer implementations. The proposed change adds a special case for IonStructLite.getType that avoids itable/vtable lookups. All other types fall back to the existing implementations, now called getTypeSlow().

IonStructLite is chosen for the fast path because it is the most commonly used container type as well as one of the most common of all types. This is important because internal usages of IonValue.getType() occur on container values much more often than on scalars. Although the added microbenchmarks show this change is performance-neutral, performance testing within a large application that heavily uses Ion structs demonstrates significant benefit.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines +834 to +841
Class<?> clazz = this.getClass();
// IonStructLite is chosen for the fast path because it is the most commonly used container type as well as
// one of the most common of all types. This is important because internal usages of IonValue.getType() occur
// on container values much more often than on scalars. Do not remove or add special case branches here without
// a thorough study of the performance implications.
if (clazz == IonStructLite.class) {
return IonType.STRUCT;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you included the most important part of the optimization 🎉 .

@tgregg tgregg merged commit fa12d30 into master Aug 29, 2024
13 checks passed
@tgregg tgregg deleted the ionvalue-gettype-optimization branch August 29, 2024 21:13
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.

3 participants