Added AccessEvaluator.canAccess(ParsedAccessExpression) #93
Conversation
In the MultiAccessEvaluatorImpl case it was calling AccessEvaluator.canAccess(byte[]) for each AccessEvaluator. canAccess(byte[]) would re-parse the AccessExpression each time. This change enables the MultiAccessEvaluatorImpl to get the ParsedAccessExpression and call the new method on each AccessEvaluator. The new method traverses the parse tree for evalation so that parsing is not performed multiple times. The downside is that there are two different methods in use, but the tests were already checking this and still pass. Closes apache#78
core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public boolean canAccess(AccessExpression accessExpression) { | ||
| return canAccess(accessExpression.parse()); |
There was a problem hiding this comment.
Have you done any testing to confirm this is faster? The code that does not create the parse tree was a lot faster when benchmarking, but not sure by how much. Not sure what the numbers are but if it runs in 1/5 of the time then would need 5 auth sets before it makes sense to parse.
There was a problem hiding this comment.
I think with the suggested change above, this issue is OBE. AccessEvaluator will only evaluate using the parse tree if parse() has been called.
|
Output of benchmark run from latest commit: |
|
I think based on the benchmark output above, I can close this issue without merging the changes. It looks like reparsing the AccessExpression byte[] and evaluating it is faster than iterating over an existing parse tree. I think the corresponding issue can also be closed with a note that this is a reparsing in MultiAccessEvaluatorImpl, but it's faster than using the parse tree. |
|
Is there a benchmark that covers the case of evaluating w/ a parse tree but does not include the time to create the parse tree? In testing we did long ago using the Accumulo code it seems like it was really fast at evaluating if you happened to already have a parse tree and did not include the time to create the parse tree. Seems like those test created the parse trees outside the benchmarked code. |
|
No, I don't think so. The benchmark currently contains methods that do the following:
Tests 1, 2, and 3 all end up calling ParsedAccessExpression, and it's parse tree, are not used anywhere that I can tell. |
Created #97 to time creating parse trees. It would be useful to look for changes in that timing, I would like to see the timing diff for that between main and #96. Going to experiment w/ creating a slightly different benchmark for this PR. |
|
Actually the thing I was curious about is already done. The benchmarks |
core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| private boolean canAccessParsedAuthorization(String token) { | ||
| var bytesWrapper = ParserEvaluator.lookupWrappers.get(); | ||
| var authToken = token.getBytes(UTF_8); |
There was a problem hiding this comment.
This is allocating objects and copying data, could be the cause of being slower than the accumulo code. Not really sure how to avoid this, and w/ the changes in #96 it may not be worth trying to avoid at this point.
|
Here's the latest benchmark results: |
2 participants
In the MultiAccessEvaluatorImpl case it was calling AccessEvaluator.canAccess(byte[]) for each AccessEvaluator. canAccess(byte[]) would re-parse the AccessExpression each time. This change enables the MultiAccessEvaluatorImpl to get the ParsedAccessExpression and call the new method on each AccessEvaluator. The new method traverses the parse tree for evalation so that parsing is not performed multiple times. The downside is that there are two different methods in use, but the tests were already checking this and still pass.
Closes #78