keith-turner commented on code in PR #113:
URL: https://github.com/apache/accumulo-access/pull/113#discussion_r2933693258
##########
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:
> Could we avoid this toString here and below by just making the predicate
one of CharSequence instead of String?
We could pass a CharSequence, they are are harder to use correctly though.
For example can not easily use them as a key in a map (there are lots of sharp
edges w/ this). Also the predicate impl should not keep a reference to
CharSequence we pass because our code may mutate the concrete type later. The
reason they are used internally is so that objects can be reused.
> Or would that be a problem because mutable CharSequences could be modified
as a side-effect of the predicate?
It should not be mutable using the methods on CharSequence, would have to
cast it to a concrete type.
--
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]
