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.

Sign up for GitHub

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

Jump to bottom

Cache types in ConstructorMapper #2650

Closed
wants to merge 3 commits into from
Closed

Cache types in ConstructorMapper #2650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eb042b
Use ConstructorBenchmark in main method
elonazoulay Mar 1, 2024
344bb93
Cache types in ConstructorMapper
elonazoulay Feb 22, 2024
a067a4c
Cache types in ConstructorInstanceFactory
elonazoulay Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/src/main/java/org/jdbi/v3/benchmark/ConstructorBenchmark.java
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ public class ConstructorBenchmark {

public static void main(final String[] args) throws RunnerException {
final Options options = new OptionsBuilder()
.include(BeanBindingBenchmark.class.getSimpleName())
.include(ConstructorBenchmark.class.getSimpleName())
.forks(0)
.build();
new Runner(options).run();
Expand Down
34 changes: 24 additions & 10 deletions core/src/main/java/org/jdbi/v3/core/mapper/reflect/ConstructorInstanceFactory.java
View file
Open in desktop
Expand Up @@ -13,38 +13,44 @@
*/
package org.jdbi.v3.core.mapper.reflect;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.lang.reflect.Parameter;
import java.lang.reflect.Type;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;
import java.util.stream.Stream;

import org.jdbi.v3.core.internal.exceptions.Unchecked;

import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toUnmodifiableList;

class ConstructorInstanceFactory<T> extends InstanceFactory<T> {
private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lmk if this shouldn't be cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you want the weak hashmap added back vs caching in the constructor instance factory (saved the previous version, so I can quickly update).

Copy link
Member

Choose a reason for hiding this comment

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

So I think there is a bit of a misunderstanding here. I didn't mean to cache the lookup factory instance itself, but the constructorHandle instance itself.

Something like SynchronizedMap<WeakHashMap<ConstructorHandleAndTypes, MethodHandle>>> (referring to the key type from the other PR)

Sorry I wasn't clear, and sorry for taking so long to respond! Been a busy couple weeks at work.

private final Constructor<T> constructor;

private final Supplier<Type[]> typeSupplier;
private final List<Type> types;
private final MethodHandle constructorHandle;

ConstructorInstanceFactory(Constructor<T> constructor) {
super(constructor);
this.constructor = requireNonNull(constructor, "constructor is null");
this.typeSupplier = getTypeSupplier(constructor, super::getTypes);
this.types = extractTypes(constructor, super::getTypes);
this.constructorHandle = getConstructorMethodHandle(constructor);
}

@Override
Type[] getTypes() {
return typeSupplier.get();
List<Type> getTypes() {
return types;
}

@Override
T newInstance(Object... params) {
return Unchecked.<Object[], T>function(constructor::newInstance).apply(params);
return (T) Unchecked.<Object[], Object>function(constructorHandle::invokeWithArguments).apply(params);
}

@Override
Expand Down Expand Up @@ -76,13 +82,21 @@ private static <T> boolean isGenericInformationLost(Constructor<T> factory) {
return lossDetected;
}

private static <T> Supplier<Type[]> getTypeSupplier(Constructor<T> constructor, Supplier<Type[]> defaultSupplier) {
private static <T> List<Type> extractTypes(Constructor<T> constructor, Supplier<List<Type>> defaultSupplier) {
if (isGenericInformationLost(constructor)) {
return () -> getFields(constructor)
return getFields(constructor)
.map(Field::getGenericType)
.toArray(Type[]::new);
.collect(toUnmodifiableList());
}
return defaultSupplier.get();
}

private static <T> MethodHandle getConstructorMethodHandle(Constructor<T> constructor) {
try {
return LOOKUP.unreflectConstructor(constructor).asFixedArity();
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
return defaultSupplier;
}

private static <T> Stream<Field> getFields(Constructor<T> constructor) {
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/jdbi/v3/core/mapper/reflect/ConstructorMapper.java
View file
Open in desktop
Expand Up @@ -207,14 +207,14 @@ private Optional<RowMapper<T>> createSpecializedRowMapper(StatementContext ctx,
List<String> unmatchedColumns) {
final int count = factory.getParameterCount();
final Parameter[] parameters = factory.getParameters();
final Type[] types = factory.getTypes();
final List<Type> types = factory.getTypes();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to convert to list due to error about exposing an array. Thinking to change the api to store the array internally and change the signature to getType(index), wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds ok

boolean matchedColumns = false;
final List<String> unmatchedParameters = new ArrayList<>();
final List<ParameterData> paramData = new ArrayList<>();

for (int i = 0; i < count; i++) {
final Parameter parameter = parameters[i];
final Type parameterType = types[i];
final Type parameterType = types.get(i);
boolean nullable = isNullable(parameter);
Nested nested = parameter.getAnnotation(Nested.class);
if (nested == null) {
Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/org/jdbi/v3/core/mapper/reflect/InstanceFactory.java
View file
Open in desktop
Expand Up @@ -18,12 +18,14 @@
import java.lang.reflect.Parameter;
import java.lang.reflect.Type;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.stream.Stream;

import jakarta.annotation.Nullable;

import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toUnmodifiableList;

abstract class InstanceFactory<T> {
private final Executable executable;
Expand All @@ -44,10 +46,10 @@ Parameter[] getParameters() {
return executable.getParameters();
}

Type[] getTypes() {
List<Type> getTypes() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here - should it be Type getType(int index)?

Copy link
Member

Choose a reason for hiding this comment

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

OK by me

return Arrays.stream(getParameters())
.map(Parameter::getParameterizedType)
.toArray(Type[]::new);
.collect(toUnmodifiableList());
}

@Nullable
Expand Down