SLING-12217 : Reduce resource lookups in Sling Model resolution

[bkkothari2255]

Issue : SLING-12217

While SLING-12244 fixed the database side of these duplicate lookups, AdapterImplementations.lookup() was still hammering the CPU by re-evaluating search paths and querying the large ConcurrentHashMap hundreds of times per page render for the same exact resource type

changes:

• Request scoped Cache: attach a cache directly to ResourceResolver.getPropertyMap() so it lives and dies with the HTTP request
• Thread Safe Identity Caching: The outer cache uses Collections.synchronizedMap(new IdentityHashMap<>()) to safely bypass expensive O(N) deep equality checks without crashing multi-threaded integration tests
• Thread Safe Inner Cache: Uses a standard ConcurrentHashMap for concurrent string lookups
• Remembering Not Found Results: We use a placeholder (Object.class) to remember when a resource type doesn't have a model. This lets us instantly abort future checks instead of endlessly re-walking the JCR tree looking for something that isn't there.

Let me know what you think?

[joerghoh]

A ResourceResolver is not thread-safe, so this cache does not need to be thread-safe as well.

[joerghoh]

Also, why is an IdentityHashMap used? This would be only effective, if the same map reference is used multiple times when calling this method. Is this really the case?

[joerghoh]

And a third aspect: We probably should avoid using an unbounded map, as it could lead to massive memory usage or even a memory leak.

[bkkothari2255]

A ResourceResolver is not thread-safe, so this cache does not need to be thread-safe as well.

You are absolutely right!

in a production environment, the ResourceResolver is tied to a single request and single thread, so it shouldn't need thread-safety

However, during the maven build, I observed that during the integration tests (like AdapterFactoryTest), a single mocked ResourceResolver is actually shared across multiple threads. This contract violation in the test suite was corrupting the standard map buckets and causing the JVM to hang

I added synchronizedMap purely as a workaround to keep the test suite passing

If we prefer to keep the production code "pure" we can remove the synchronization and look into fixing the multi-threading in the test class instead

What do you prefer?

[bkkothari2255]

Also, why is an IdentityHashMap used? This would be only effective, if the same map reference is used multiple times when calling this method. Is this really the case?

I double-checked this, and the map reference is indeed constant

the map argument always comes from the class-level variables (e.g., resourceTypeMappingsForResources), which are declared as final, Because the reference never changes and IdentityHashMap works perfectly here to skip the deep equality check

Could you verify if my logic holds up there?

[bkkothari2255]

And a third aspect: We probably should avoid using an unbounded map, as it could lead to massive memory usage or even a memory leak.

For the memory leak, I actually used ResourceResolver.getPropertyMap() exactly as suggested in the Jira ticket,

Since the cache dies with the HTTP request, it can't cause a long-term leak. And based on your WKND stats (~36 distinct paths), it shouldn't use much memory during the request either

Let me know if you'd still prefer we use a bounded map just to be extra safe or if you think the request-scoped map is good to go

[joerghoh]

That's a very complex type definition and hard to reason about. Would it make sense to encapsulate some parts of it in an inner class, just to simplify this expression?

[bkkothari2255]

Agreed! I'll encapsulate that structure into an inner class to make it much easier to reason about, and push the change shortly

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud