Significant performance regression in WebHandler

[wyhasany]

Hi! It looks like this PR makes big performance regression for my project. As there is a contention on LRU cache locks. We're creating thousands of links concurrently and threads have to wait for each other. @odrotbohm could you revert that PR or propose non blocking solution?

Here you can see part of profiling my app:

Before that change there is no wall-clock time on that locks.

The reason of that is the ConcurrentReferenceHashMap. It doesn't lock on get() method while ConcurrentLruCache$get uses ReentrantReadWriteLock.

More over (by @mprzeor):

just to show you the scale of the problem that @wyhasany described - we had to downgrade Spring Boot from 2.4.3 to 2.4.1 because average response time in our project increased 10 times, from 7ms to 70ms

[wyhasany]

Link to #1405

[odrotbohm]

It looks like the fundamental problem is that the cache key includes the expanded mapping of the method pointed to which means that we have virtually unlimited cache keys depending on the expanded links we create. I'll try to separate the keys and cached values from that dynamic value so that we still only lookup the static metadata once and combine that with the dynamically to produce URI.

[odrotbohm]

Would you mind to try the 1.3.0-SNAPSHOTs and see whether that improves things? If that's the case I'd backport the changes to 1.2 and 1.1 respectively so that they're picked up in the Boot 2.3 maintenance releases.

[wyhasany]

Definitely, we can make such a test 👍

[wyhasany]

Is it already published?

[odrotbohm]

It is, see here. You should be able to consume the artifacts by setting spring-hateoas.version in your POM to 1.3.0-SNAPSHOT.

[wyhasany]

It would be hard to test it as the newest changes breaks our unit tests like below:

Cannot invoke "org.springframework.core.convert.ConversionService.canConvert(org.springframework.core.convert.TypeDescriptor, org.springframework.core.convert.TypeDescriptor)" because "conversionService" is null
java.lang.NullPointerException: Cannot invoke "org.springframework.core.convert.ConversionService.canConvert(org.springframework.core.convert.TypeDescriptor, org.springframework.core.convert.TypeDescriptor)" because "conversionService" is null
	at org.springframework.hateoas.server.core.WebHandler$HandlerMethodParameter.getValueAsString(WebHandler.java:363)
	at org.springframework.hateoas.server.core.WebHandler.bindRequestParameters(WebHandler.java:227)
	at org.springframework.hateoas.server.core.WebHandler.lambda$linkTo$0(WebHandler.java:133)
	at org.springframework.hateoas.server.core.WebHandler.linkTo(WebHandler.java:86)
	at org.springframework.hateoas.server.mvc.WebMvcLinkBuilderFactory.linkTo(WebMvcLinkBuilderFactory.java:121)
	at org.springframework.hateoas.server.mvc.WebMvcLinkBuilder.linkTo(WebMvcLinkBuilder.java:167)
	at acme.LinkCreator$LinkInCreation.build(LinkCreator.java:74)

do you have any best practices how to pass fake FALLBACK_CONVERSION_SERVICE?

It looks like following code won't work at our tests:

private Supplier<ConversionService> getConversionService() {

	return () -> {

		RequestAttributes attributes = RequestContextHolder.getRequestAttributes();

		if (!ServletRequestAttributes.class.isInstance(attributes)) {
			return null;
		}

		ServletContext servletContext = ((ServletRequestAttributes) attributes).getRequest().getServletContext();
		WebApplicationContext context = WebApplicationContextUtils.getWebApplicationContext(servletContext);

		return context == null || !context.containsBean("mvcConversionService")
				? FALLBACK_CONVERSION_SERVICE
				: context.getBean("mvcConversionService", ConversionService.class);
	};
}

as there is no ThreadLocal RequestContextHolder.getRequestAttributes() in context.

This condition seems to be broken to me:

if (!ServletRequestAttributes.class.isInstance(attributes)) {
	return null;
}

when attribute is null then it returns null and we can't reach logic:

return context == null || !context.containsBean("mvcConversionService")
    ? FALLBACK_CONVERSION_SERVICE
    : context.getBean("mvcConversionService", ConversionService.class);

to use fallback service

[odrotbohm]

Yet another great catch, @wyhasany! It should affect unit tests only that do not register RequestAttributes. Ours do that, because we of course want to see MVC's ConversionService picked up. Missed that NPE through that. Just pushed a fix that's already published.

For reference for others: you nee to use the snapshot repository at https://repo.spring.io/snapshot to access snapshots. I.e the 1.3.0-SNAPSHOT binaries can be found here: https://repo.spring.io/snapshot/org/springframework/hateoas/spring-hateoas/1.3.0-SNAPSHOT/

[odrotbohm]

I've also ported the fix to 1.2.5-SNAPSHOT, if that's easier to try.

[wyhasany]

@odrotbohm that looks great, tommorow I'm going to test it

[wyhasany]

The another day to test spring-hateoas :) as far as my unit tests still fails with 1.3.0-SNAPSHOT I've made test with 1.2.5-SNAPSHOT. Should I open another ticket or should I change my test to use thread local request?

I've made test today it seems that there is no longer regression. Our app works with similiar latency as with spring-hateoas 1.2.2. That's a great job @odrotbohm! 🎊 I think you can close this issue.

[odrotbohm]

That's great news! I forgot to push the fix for the NPE on master. It should be up and ready for consumption now. I'll do an additional back port to 1.1.x for inclusion in Spring Boot 2.3 then, too. Releases to be shipped tomorrow, for inclusion in Boot releases later this week.

[odrotbohm]

Looks like 1.1 isn't even affected.

[wyhasany]

That was pretty fast! Thanks for awesome support 👍