ctubbsii commented on code in PR #113:
URL: https://github.com/apache/accumulo-access/pull/113#discussion_r2934044459
##########
modules/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java:
##########
@@ -43,7 +45,27 @@ public final class AccessEvaluatorImpl implements
AccessEvaluator {
*/
AccessEvaluatorImpl(Predicate<String> authorizationChecker,
AuthorizationValidator authorizationValidator) {
- this.authorizedPredicate = auth ->
authorizationChecker.test(auth.toString());
+
+ // This map helps avoid allocating string objects on each call to this
predicate
+ Map<CharsWrapper,Boolean> checkCache = new ConcurrentHashMap<>();
+ this.authorizedPredicate = auth -> {
+ if (auth instanceof CharsWrapper wrapped) {
+ // Try to avoid allocating a string object and copying the char array.
+ Boolean cachedResult = checkCache.get(wrapped);
+ if (cachedResult == null) {
+ // Not in cache, so have to allocate and copy
+ String authStr = wrapped.toString();
+ cachedResult = authorizationChecker.test(authStr);
Review Comment:
I was trying to understand the reuse behavior, but it's not very well
documented. I understand that there's a thread-local instance, but I see
existing places where that thread-local CharsWrapper instance has a reference
in some internal predicates. I tried to track down to see if it could be
modified while the predicate still existed, and it wasn't obvious at a glance.
I think there's probably some room for improvement here, at least with regard
to some comments to explain the design and limitations on when it can/should be
mutated or when it is unsafe to hold a reference to it.
--
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]
