keith-turner commented on code in PR #86:
URL: https://github.com/apache/accumulo-access/pull/86#discussion_r1954734336
##########
src/main/java/org/apache/accumulo/access/AccessExpressionImpl.java:
##########
@@ -46,13 +48,12 @@ public String getExpression() {
@Override
public ParsedAccessExpression parse() {
- if (parsed == null) {
- synchronized (this) {
- if (parsed == null) {
- parsed =
ParsedAccessExpressionImpl.parseExpression(expression.getBytes(UTF_8));
- }
+ return parseTreeRef.updateAndGet((parseTree) -> {
Review Comment:
I think these changes are functionally equivalent. One nice feature for
this function to have is that it always returns the same reference and never
spuriously returns a different object. Both algorithms seem to have this
behavior.
This change has two differences. First it will always write to memory
because updateAndGet always writes to memory even when nothing changed. Second
threads may compute a parse tree that is thrown away. Computing a parse tree
and throwing it aways is probably not a big deal as long as the same reference
is always returned.
It would be nice to avoid always writing to the volatile when we read as
that could have cache side effects.
The following is an attempt to avoid the write to memory when its not
needed. Multiple threads may still compute the parse tree, but the method
should always return just one of those.
```java
var parseTree = parsed.get();
if (parseTree == null) {
parsed.compareAndSet(null,
ParsedAccessExpressionImpl.parseExpression(expression.getBytes(UTF_8)));
// must get() again in case another thread won w/ the compare and set,
this ensures this
// method always returns the exact same object
parseTree = parsed.get();
}
return parseTree;
```
--
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]
