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

HV 6 Memory and performance improvements #814

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
2 participants
@gsmet
Copy link
Member

commented Jul 19, 2017

Note 2: the order of the commits displayed below is not exactly the one I have in my branch - the order of the HV-1439 commits is more consistent in my branch.

It's a big one. I started working on the different subjects separately but as I touched the same code and had the same issues, I had to cherry-pick some of the commits in other branches and finally ended up joining the efforts in one branch. The commits are well isolated and should be easily understandable by reading the commit comment and the code.

HV-1438

There are several things here:

  • I removed completely the ObjectValueExtractor. It's not a good idea to rely on the extractor infrastructure to validate the annotated object itself. Funnily enough, removing it revealed a few issues (these are the commits before the "Remove the ObjectValueExtractor" commit);
  • Narrowing down the value extractors considered is a necessity as cascading is real slow now, mostly due to the calls to TypeVariableBindings. What I did work, but I think I found an issue in the TCK: beanvalidation/beanvalidation-tck#149 .

HV-1437

These are memory footprint improvements. The idea was mostly to reduce the size of the metadata. The example reported by Stuart is kinda extreme: see https://github.com/keycloak/keycloak/blob/master/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RealmEntity.java .

6.0 was already better than 5.x as I introduce the CollectionHelper.toImmutable* utilities: they trim down the empty and one element collections to the minimum but there was still some room for improvements.

All in all, we reduced the BeanMetaDataImpl of RealmEntity from ~177 kB in the original snapshot of Stuart (using 5.x) to ~60 kB (using this branch).

I'll post something on the list about some other things I'd like to discuss.

HV-1439

Benchmark improvements

The benchmarks were a bit painful to use when you wanted to compare closely 2 versions and switch from one to the other.

I pushed some clean ups to them.

I also removed the iterative benchmarks which were useless.

Some runtime performance improvements

Nothing big but a few things I noticed while benchmarking and it definitely improved the performances.

Quite a lot of parsing/metadata building improvements

The situation was quite bad here so I made quite a lot of improvements to reduce the startup cost of HV.

HV-1443

A small fix required by the other changes.

@gsmet gsmet force-pushed the gsmet:HV-1437-2 branch 6 times, most recently from c19f1a1 to ca5c4a7 Jul 19, 2017

@gsmet gsmet force-pushed the gsmet:HV-1437-2 branch 3 times, most recently from a049968 to 704e37b Jul 31, 2017

@gunnarmorling
Copy link
Member

left a comment

@gsmet Looks great to me overall. Added some comments/questions inline, nothing special really.


@SuppressWarnings("unchecked")
public <T> T getMandatoryParameter(String parameterName, Class<T> type) {
if ( !parameters.containsKey( parameterName ) ) {

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

We could avoid the containsKey() + get() cascade by just checking the result of the latter on being null.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

Done.

private Map<String, Object> getAnnotationValues(Annotation annotation) {
if ( Proxy.isProxyClass( annotation.getClass() ) ) {
InvocationHandler invocationHandler = Proxy.getInvocationHandler( annotation );
if ( invocationHandler instanceof AnnotationProxy ) {

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

The commit message is a bit mis-leading IMHO, as it's only about proxies created by ourselves. I first thought you'd access the parameter map of the actual sun.reflect.annotation.AnnotationInvocationHandler.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

Not sure what is unclear. I replaced is a proxy by is an AnnotationProxy in the commit message.

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 3, 2017

Member

Ok, that was what I meant.


public static class AnnotationParameters implements Serializable {

private final Map<String, Object> parameters;

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

Would be nice to have the @Immutable tag here.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

Done.

private final Map<Class<? extends Annotation>, List<? extends ConstraintValidatorDescriptor<?>>> builtinConstraints;

private final ConcurrentHashMap<Class<? extends Annotation>, Boolean> externalConstraints = new ConcurrentHashMap<>();

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

Minor nitpick -- could be declared as ConcurrentMap.

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

More generally, though, it seems to go a bit against the goal of reducing memory usage.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

Yes, I know. But I think it's sufficiently beneficial and we improved a lot the memory footprint so we can afford it.

@@ -69,7 +69,7 @@ else if ( genericSuperType instanceof ParameterizedType ) {

boolean typeParameterFoundInSubType = false;
for ( Entry<TypeVariable<?>, TypeVariable<?>> subTypeParameter : bindings.entrySet() ) {
if ( typeArgument.equals( subTypeParameter.getValue() ) ) {
if ( typeArgument == subTypeParameter.getValue() ) {

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

Just to confirm: are we totally sure we get the same instance when asking for the same argument multiple times? Admittedly, it'd feel safer to stick to equals.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

I think so. You compare the type arguments of the same class so they should be identical.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

OK, considering this part is not a hot spot anymore, I think we can go back to using equals. It made a big difference when TypeVariableBindings was called at runtime but it's probably negligible now that it's only a startup cost.

I don't see a case where == would fail but as it's deep into Java, let's be on the safe side.

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 3, 2017

Member

Ok, cool. My concern is that it's relying on an implementation detail of the class library whether "==" is fine or not.

Set<ValueExtractorDescriptor> typeCompatibleExtractors = valueExtractors.values()
.stream()
.filter( e -> TypeHelper.isAssignable( erasedDeclaredType, e.getContainerType() ) )
.collect( Collectors.toSet() );

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

Could we collect into our immutable helper right away?

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

This is a transient set, it's not reused after that so I don't think it makes sense to make it immutable here.

We usually only make collections immutable when we store them in a field.

@@ -786,4 +786,11 @@ ValueExtractorDefinitionException getOnlyUnboundWildcardTypeArgumentsSupportedFo

@Message(id = 226, value = "Container element constraints and cascading validation are not supported on arrays: %1$s")
ValidationException getContainerElementConstraintsAndCascadedValidationNotSupportedOnArraysException(@FormatWith(TypeFormatter.class) Type type);

@Message(id = 227, value = "The value extractor candidate set may not be empty for type: %1$s.")
IllegalArgumentException getValueExtractorCandidateSetMayNotBeEmptyException( @FormatWith(ClassObjectFormatter.class) Class<?> valueType );

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

Not a specific BV exception?

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

IMHO it's a very low level issue people shouldn't have. It's more to be on the safe side. I even thought it might just be an assertion.

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

Changed it to use Contracts.

Type type = ReflectionHelper.typeOf( executable );
boolean isCascaded = returnValueType.getValid() != null;
Map<Class<?>, Class<?>> groupConversions = groupConversionBuilder.buildGroupConversionMap(
returnValueType.getConvertGroup(),
defaultPackage
);

return isArray
? CascadingTypeParameter.arrayElement( type, isCascaded, containerElementTypesCascadingMetaData, groupConversions )

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 1, 2017

Member

Is that method array element obsolete wit that? I.e. can it be removed?

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 1, 2017

Author Member

It's not removed because we still use it in the code I kept if we want to revive array support. It makes sense for the array elements but here we are talking about the whole array so it should be an annotated object.

@gsmet gsmet force-pushed the gsmet:HV-1437-2 branch 2 times, most recently from 3d75edc to 4f4418d Aug 1, 2017

@gsmet

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

@gunnarmorling force pushed an update fixing your remarks.

I answered to the others, let me know if there's something else you want me to fix.

@gsmet

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

@gunnarmorling added 2 more commits to remove the metadata of the unconstrained methods (one is purely cosmetic).

public ExecutableMetaData getMetaDataFor(Executable executable) {
return executableMetaDataMap.get( ExecutableHelper.getSignature( executable ) );
public Optional<ExecutableMetaData> getMetaDataFor(Executable executable) {
String signature = ExecutableHelper.getSignature( executable );

This comment has been minimized.

Copy link
@gsmet

gsmet Aug 2, 2017

Author Member

First I thought about having 2 separate methods but the call to ExecutableHelper.getSignature( executable ) has a higher cost than expected. Thus, it's better to have only one method.

I think the Optional makes it clear that the behavior is a bit different. We might even rename the method to differentiate it better? Maybe getConstraintMetaDataFor()?

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Aug 3, 2017

Member

No strong opinion on the rename.

@gsmet gsmet closed this Aug 3, 2017

@gsmet gsmet force-pushed the gsmet:HV-1437-2 branch from 5530c8a to 51ca465 Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.