PreferredConstructor.isConstructorParameter(…) on hot code paths creates a lot of allocations

[lucaspouzac]

Hello,

I think it would be interesting to also cache in the case where the persistentproperty is not used by the constructor, in order to avoid the creation of an iterator, in particular rather expensive during large list.

spring-data-commons/src/main/java/org/springframework/data/mapping/PreferredConstructor.java

Line 122 in cd82d50

public boolean isConstructorParameter(PersistentProperty<?> property) {

	public boolean isConstructorParameter(PersistentProperty<?> property) {
...
		for (Parameter<?, P> parameter : parameters) {
			if (parameter.maps(property)) {
				isPropertyParameterCache.put(property, true);
				return true;
			}
		}

		isPropertyParameterCache.put(property, false);
		return false;
	}

[mp911de]

That's in place for almost 8 years, see:

spring-data-commons/src/main/java/org/springframework/data/mapping/PreferredConstructor.java

Lines 122 to 130 in cd82d50

	public boolean isConstructorParameter(PersistentProperty<?> property) {
	Assert.notNull(property, "Property must not be null!");
	Boolean cached = isPropertyParameterCache.get(property);
	if (cached != null) {
	return cached;
	}

[lucaspouzac]

Yes, but only true values are cached, not false values, see below :

spring-data-commons/src/main/java/org/springframework/data/mapping/PreferredConstructor.java

Lines 122 to 142 in cd82d50

	public boolean isConstructorParameter(PersistentProperty<?> property) {
	Assert.notNull(property, "Property must not be null!");
	Boolean cached = isPropertyParameterCache.get(property);
	if (cached != null) {
	return cached;
	}
	boolean result = false;
	for (Parameter<?, P> parameter : parameters) {
	if (parameter.maps(property)) {
	isPropertyParameterCache.put(property, true);
	result = true;
	break;
	}
	}
	return result;
	}

[mp911de]

Caching the negative outcome doesn't break anything so we should be good to reuse the cached result. However, the API can be used in a way, where the PersistentProperty was not yet added to PersistentEntity so these state changes may lead to an invalid cached result when the state gets changed.

[mp911de]

Benchmark after applying the change:

Before:
Benchmark                                                               Mode  Cnt        Score        Error  Units
TypicalEntityReaderBenchmark.simpleEntityGeneratedConstructorAndField  thrpt   10  6848447,474 ± 554354,377  ops/s

After
Benchmark                                                               Mode  Cnt        Score         Error  Units
TypicalEntityReaderBenchmark.simpleEntityGeneratedConstructorAndField  thrpt   10  9071099,879 ± 1423166,087  ops/s

Roughly a gain of 32% in throughput. Good catch.