lukaszlenart commented on code in PR #1721:
URL: https://github.com/apache/struts/pull/1721#discussion_r3401034501
##########
core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java:
##########
@@ -33,6 +35,15 @@
import static org.apache.commons.lang3.StringUtils.strip;
public class ConfigParseUtil {
+ // Size the cache to prevent excessive memory usage in environments with
many classloaders and/or large numbers of classes being validated.
+ // While still providing a reasonable caching benefit for common cases
(e.g. multiple Struts instances in the same container, or multiple calls to
validate the same class across different containers).
+ private static final int MAX_CLASS_CACHE_SIZE = 50;
+
+ private static final Cache<ClassLoader, Cache<String, Class<?>>>
VALIDATED_CLASS_CACHE = Caffeine.newBuilder()
+ .weakKeys()
+ .weakValues()
Review Comment:
Non-blocking: `weakValues()` on the **outer** cache makes the per-loader
inner `Cache` (the *value*) weakly reachable. Nothing else holds a strong
reference to an inner cache, so GC can reclaim it while its `ClassLoader` key
is still alive and in active use — which intermittently empties the cache and
partially defeats the optimization under memory pressure.
`weakKeys()` already guarantees the entry is dropped as soon as the loader
becomes unreachable, so the outer `weakValues()` isn't needed for leak-safety.
Suggest removing it from the **outer** builder (keep `weakKeys()` here and the
`weakValues()` on the inner cache at line 97, which correctly lets unreferenced
`Class` values be reclaimed). Correctness is fine either way; this is purely
about cache effectiveness.
Also minor: with the redesign `maximumSize(50)` now bounds the number of
*classloaders*, not classes — the `MAX_CLASS_CACHE_SIZE` comment above still
describes bounding classes.
--
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]
