jsedding commented on code in PR #107: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/107#discussion_r1401862039
########## src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java: ########## @@ -140,7 +142,10 @@ public class MapEntries implements private Map <String,List <String>> vanityTargets; - private Map<String, Map<String, String>> aliasMap; + /** + * The key of the map is the parent path, while the value is a map with the the resource name as key and the actual aliases as values) + */ + private ConcurrentMap<String, ConcurrentMap<String, List<String>>> aliasMapsMap; Review Comment: If I understand correctly you're changing the data structure from "Map<parentPath, Map<aliasedName, resourceName>" to "Map<parentPath, Map<resourceName, List<aliasedName>>". Is that correct? If that is the case, I like the more compact storage. Just be aware, however, that it makes "lookup by alias" slightly more expensive, as it requires traversal of the entire map returned for a parentPath, including a `Collection#contains(String)` call on the `List`. On the flip side, it is cheaper to generate all aliased paths. Rgd. the implementation, I would avoid using `ConcurrentMap` in the type definition (unless we use some of its API), and rather use `Map<String, Map<String, List<String>>`. I am also wondering about the nature of the `List<String>`, i.e. the aliases IIUC. - Do we need to preserve their order? - Do we need to maintain duplicates? - Is the list updated or replaced? And, given the above questions, would it make sense to replace rather than update it? Depending on the answers, we could consider using a `ConcurrentSkipListSet`, `Collections.newSetFromMap(new ConcurrentHashMap<>())` or an immutable `Set`, e.g. via `Set.of()`. In any case, an implementation that has a fast `Collection#contains(String)` implementation would be good. ########## src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java: ########## @@ -18,32 +18,6 @@ */ package org.apache.sling.resourceresolver.impl.mapping; -import org.apache.commons.collections4.map.LRUMap; Review Comment: I would prefer limiting reordering of the imports. -- 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: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org