joerghoh commented on code in PR #86:
URL:
https://github.com/apache/sling-org-apache-sling-models-impl/pull/86#discussion_r3035743013
##########
src/main/java/org/apache/sling/models/impl/AdapterImplementations.java:
##########
@@ -314,39 +316,71 @@ public Class<?> getModelClassForResource(final Resource
resource) {
return getModelClassForResource(resource,
resourceTypeMappingsForResources);
}
+ @SuppressWarnings("unchecked")
+ private static Map<Map<String, Class<?>>, Map<String, Object>>
getModelClassCache(final ResourceResolver resolver) {
+ return (Map<Map<String, Class<?>>, Map<String, Object>>)
+ resolver.getPropertyMap().get(CACHE_KEY);
+ }
+
protected static Class<?> getModelClassForResource(final Resource
resource, final Map<String, Class<?>> map) {
if (resource == null) {
return null;
}
ResourceResolver resolver = resource.getResourceResolver();
final String originalResourceType = resource.getResourceType();
+ if (originalResourceType.isEmpty()) {
+ return null;
+ }
+
+ Map<Map<String, Class<?>>, Map<String, Object>> cache =
getModelClassCache(resolver);
+
+ if (cache == null) {
+ // Thread-safe Identity map to prevent O(N) deep equality checks
on the massive map
+ cache = java.util.Collections.synchronizedMap(new
java.util.IdentityHashMap<>());
Review Comment:
A ``ResourceResolver`` is not thread-safe, so this cache does not need to be
thread-safe as well.
##########
src/main/java/org/apache/sling/models/impl/AdapterImplementations.java:
##########
@@ -314,39 +316,71 @@ public Class<?> getModelClassForResource(final Resource
resource) {
return getModelClassForResource(resource,
resourceTypeMappingsForResources);
}
+ @SuppressWarnings("unchecked")
+ private static Map<Map<String, Class<?>>, Map<String, Object>>
getModelClassCache(final ResourceResolver resolver) {
+ return (Map<Map<String, Class<?>>, Map<String, Object>>)
+ resolver.getPropertyMap().get(CACHE_KEY);
+ }
+
protected static Class<?> getModelClassForResource(final Resource
resource, final Map<String, Class<?>> map) {
if (resource == null) {
return null;
}
ResourceResolver resolver = resource.getResourceResolver();
final String originalResourceType = resource.getResourceType();
+ if (originalResourceType.isEmpty()) {
+ return null;
+ }
+
+ Map<Map<String, Class<?>>, Map<String, Object>> cache =
getModelClassCache(resolver);
+
+ if (cache == null) {
+ // Thread-safe Identity map to prevent O(N) deep equality checks
on the massive map
+ cache = java.util.Collections.synchronizedMap(new
java.util.IdentityHashMap<>());
Review Comment:
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?
##########
src/main/java/org/apache/sling/models/impl/AdapterImplementations.java:
##########
@@ -314,39 +316,71 @@ public Class<?> getModelClassForResource(final Resource
resource) {
return getModelClassForResource(resource,
resourceTypeMappingsForResources);
}
+ @SuppressWarnings("unchecked")
+ private static Map<Map<String, Class<?>>, Map<String, Object>>
getModelClassCache(final ResourceResolver resolver) {
+ return (Map<Map<String, Class<?>>, Map<String, Object>>)
+ resolver.getPropertyMap().get(CACHE_KEY);
+ }
+
protected static Class<?> getModelClassForResource(final Resource
resource, final Map<String, Class<?>> map) {
if (resource == null) {
return null;
}
ResourceResolver resolver = resource.getResourceResolver();
final String originalResourceType = resource.getResourceType();
+ if (originalResourceType.isEmpty()) {
+ return null;
+ }
+
+ Map<Map<String, Class<?>>, Map<String, Object>> cache =
getModelClassCache(resolver);
Review Comment:
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?
##########
src/main/java/org/apache/sling/models/impl/AdapterImplementations.java:
##########
@@ -314,39 +316,71 @@ public Class<?> getModelClassForResource(final Resource
resource) {
return getModelClassForResource(resource,
resourceTypeMappingsForResources);
}
+ @SuppressWarnings("unchecked")
+ private static Map<Map<String, Class<?>>, Map<String, Object>>
getModelClassCache(final ResourceResolver resolver) {
+ return (Map<Map<String, Class<?>>, Map<String, Object>>)
+ resolver.getPropertyMap().get(CACHE_KEY);
+ }
+
protected static Class<?> getModelClassForResource(final Resource
resource, final Map<String, Class<?>> map) {
if (resource == null) {
return null;
}
ResourceResolver resolver = resource.getResourceResolver();
final String originalResourceType = resource.getResourceType();
+ if (originalResourceType.isEmpty()) {
+ return null;
+ }
+
+ Map<Map<String, Class<?>>, Map<String, Object>> cache =
getModelClassCache(resolver);
+
+ if (cache == null) {
+ // Thread-safe Identity map to prevent O(N) deep equality checks
on the massive map
+ cache = java.util.Collections.synchronizedMap(new
java.util.IdentityHashMap<>());
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
