apache / accumulo-access

Apache Accumulo Access Control Library
https://accumulo.apache.org
Apache License 2.0
4 stars 11 forks source link

Improves performance by avoiding parse tree creation #31

Closed keith-turner closed 8 months ago

keith-turner commented 11 months ago

This is an experimental commit that improves the performance of evaluating and parsing an expression at the same time by avoiding creation of a parse tree. Also some other steps were taken to avoid other object allocations.

Seeing good performance with these changes when running the benchmark. Comparing the following with numbers from #30, seeing an increase from 8.415 to 10.153 which is a 20% increase. The perfomance numbers from #30 were run on the same hardware.

Benchmark                                               Mode  Cnt   Score   Error   Units
AccessExpressionBenchmark.measureEvaluationAndParsing  thrpt   12  10.153 ± 0.147  ops/us

Not sure how this change fits in with overall code which is why it a WIP. Need to determine how it fits in with the existing parse tree code. For example could creating an expression only validate it w/o creating a parse tree? Also this code could be slower than creating a parse tree for the case of having multiple authorizations sets.

Another reason this is a WIP is that it could be improved to short circuit in "and" and "or" expression and switch to a validation mode when the result is known and avoid further set lookups. This is something that is done in the parse tree and could give a further bump in performance.

keith-turner commented 11 months ago

Added shot circuiting in 79dacd4 and seeing good performance from that, however I do not like how its implemented. Feels hacky and still does an operation that could be avoided. This is now at a 47% increase in performance over #30.

Benchmark                                               Mode  Cnt   Score   Error   Units
AccessExpressionBenchmark.measureEvaluationAndParsing  thrpt   12  12.331 ± 1.462  ops/us
keith-turner commented 11 months ago

Accumulo uses column visibility in the following way.

So these changes are ideal for the Accumulo server side code. For Accumulo client side code, it may be useful to offer the ability to validate an expression w/o creating a parse tree. So maybe the direction this PR could take is the following.

  1. Offer functionality to parse and evaluate string or byte[] access expressions all at once w/o any object allocation.
  2. Offer functionality to validate a string or byte[] access expressions w/o any object allocation.
  3. Offer functionality to compile an access expression.

This PR currently implements 1 above. For 2 above that could be implemented in this PR and could be used in the client side Accumulo code to make insertions faster. For 3 above, this library already offers but it could be presented in a different way.

// static method to validate an access expression w/o creating a parse tree, thows an exception if its not valid
AccessExpression.validate("A|B")
// change the existing static methods named 'of' to 'compile'
vat compiledAccessExpression = AccessExpression.compile("A|B");
keith-turner commented 10 months ago

The following is the result of running the benchmark on commit 7ea7b4e from this PR.

Benchmark                                           Mode  Cnt   Score   Error   Units
AccessExpressionBenchmark.measureBytesValidation   thrpt   12  29.083 ± 1.099  ops/us
AccessExpressionBenchmark.measureEvaluation        thrpt   12  12.429 ± 0.280  ops/us
AccessExpressionBenchmark.measureStringValidation  thrpt   12  24.336 ± 1.423  ops/us

The following is the result of running the benchmark on 46b5b877c3eaf433b122812701f47ae5c65ff0c4 from main.

Benchmark                                               Mode  Cnt   Score   Error   Units
AccessExpressionBenchmark.measureBytesParsing          thrpt   12  12.597 ± 0.257  ops/us
AccessExpressionBenchmark.measureEvaluation            thrpt   12  20.407 ± 0.287  ops/us
AccessExpressionBenchmark.measureEvaluationAndParsing  thrpt   12   8.235 ± 0.068  ops/us
AccessExpressionBenchmark.measureStringParsing         thrpt   12  11.267 ± 0.285  ops/us

In main the measureBytesParsing is roughly equivalent to measureBytesValidation in this PR. That goes from 12.597 to 29.083 which is a 2.31 times faster. This will be really nice for creating column visibility objects which only need to validate.

In main measureEvaluation has no equivalent in this PR because that was testing using an existing parse tree, there is no longer a parse tree. This was not a useful benchmark because pre-parsed expression would not be available when streaming through data.

In main measureEvaluationAndParsing is equivalent to measureEvaluation in this PR. That goes from 8.235 to 12.429 which is 1.51 times faster. This will be a nice speedup for evaluating visibility expressions on the server side in Accumulo.

keith-turner commented 10 months ago

Update https://github.com/apache/accumulo/pull/3746 to use the updated API in this PR.