Refine WebJarVersionLocator caching

[sdeleuze]

As pointed out by @vpavic here, WebJarCache could probably be removed. I suggest we replace it by java.util.concurrent.ConcurrentMap interface and update the logic to use ConcurrentMap#computeIfAbsent instead of WebJarCache#get and WebJarCache#put.

Also Spring has already its own caching mechanism enabled via ResourceChainRegistration (enabled by default with Spring Boot), so Spring likely do not want a 2nd caching mechanism in WebJarVersionLocator (in order to avoid wasting memory).

My proposal is to refine WebJarVersionLocator constructors with:

• WebJarVersionLocator() default constructor that does not use any caching
• WebJarVersionLocator(ConcurrentMap concurrentMap) which allows to use caching with new WebJarVersionLocator(new ConcurrentHashMap())

Alternatively, you could have:

• WebJarVersionLocator() default constructor that uses a cache based on ConcurrentHashMap
• WebJarVersionLocator(boolean cacheEnabled) that can allow Spring to disable caching
• WebJarVersionLocator(ConcurrentMap concurrentMap) that allows to pass a custom ConcurrentMap instance.

Or:

• WebJarVersionLocator() default constructor that uses a cache based on ConcurrentHashMap
• WebJarVersionLocator(@Nullable ConcurrentMap concurrentMap) which allows to disable caching with a null parameter.

Maybe the last proposal is better?

[jamesward]

I'm not totally clear on what ResourceChainRegistration is caching and how that relates to the WebJars locator. Can you provide more details? As some background, the recommended way to use the locator is from a templating engine so that dynamic rendered pages contain versioned URLs. This means that every rendering of a template will hit the locator, which then (without caching) loads stuff from the classpath, parses that stuff, and then returns a version or path. The reason for the caching is to avoid the unnecessary repeated classpath load & parse steps.

Also worth noting that ConcurrentHashMap#computeIfAbsent does synchronize on the cache key which is desired over the current get and put approach.

[jamesward]

I've implemented the computeIfAbsent change:
12e6fe7

And improved the caching to only cache the version which is the part that is loaded from the classpath.

[jamesward]

Also added caching for missing lookups. Full change:
https://github.com/webjars/webjars-locator-lite/compare/cache-improvements

[sdeleuze]

I'm not totally clear on what ResourceChainRegistration is caching and how that relates to the WebJars locator. Can you provide more details?

It is an HTTP request to resource location cache, but with additional features, see CachingResourceResolver. Given the fact this is not the most common use case without Spring, maybe you could just add support for a @Nullable constructor parameter (WebJarVersionLocator(@Nullable WebJarCache cache)) and in that case do not use any cache (my third proposal)?

[jamesward]

I've read through that and I'm still not sure how it gets used in relation to the locator. I think in Spring you have something that makes it so users can just make a request to /webjars/bootstrap/js/bootstrap.js but I'm not sure if that would somehow cache the path in the classpath that serves that. Or something else.

FWIW, I don't recommend people use those versionless URLs as they don't support good client side caching.

[sdeleuze]

I have checked on my repro and I can confirm that LiteWebJarsResourceResolver which is using WebJarVersionLocator is only invoked the first time the request is performed otherwise the cached version is return from CachingResourceResolver by default.

I understand your recommandation, but the WebJars integration in Spring is part of an extensive static web resource handling where Spring users can manage caching, version resolution and other features, so that kind of concern will be managed outside WebJarVersionLocator at Spring level, hence my ask for being able to disable the caching at WebJarVersionLocator level without having to craft a custom WebJarCache implementation, see https://spring.io/blog/2014/07/24/spring-framework-4-1-handling-static-web-resources for more context.

[sdeleuze]

Also see LiteWebJarsResourceResolver Javadoc which I think follow the guidelines you shared:

A ResourceResolver that delegates to the chain to locate a resource and then attempts to find a matching versioned resource contained in a WebJar JAR file.
This allows WebJars.org users to write version agnostic paths in their templates, like <script src="/webjars/jquery/jquery.min.js"/>. This path will be resolved to the unique version <script src="/webjars/jquery/1.2.0/jquery.min.js"/>, which is a better fit for HTTP caching and version management in applications.
This also resolves resources for version agnostic HTTP requests GET /jquery/jquery.min.js.
This resolver requires the org.webjars:webjars-locator-lite library on the classpath and is automatically registered if that library is present.

[jamesward]

Thanks for the details on that and for confirming that LiteWebJarsResourceResolver maybe doesn't need the cache. Did you confirm that this is true for multiple files in the same WebJar? The latest cache impl only has one cache miss for all of the files in a WebJar.

One other path I'm concerned about is usage from templating like Thymeleaf. I'm not sure if it'd be typical to inject something from webjars-locator-lite where the cache would be disabled and in that usecase the lack of a cache would be problematic.

[sdeleuze]

Good point, indeed with this new impl WebJarVersionLocator has added value and is complementary to the Spring cache, so let's keep it enabled.

Do you want to release a last 0.0.5 version with what we have in main to allow a last round of testing before 1.0.0?

[jamesward]

Cool. Releasing 0.0.5 now.

[sdeleuze]

Works as expected in Spring Framework tests and my demo app.