LuciferYang commented on PR #12253:
URL: https://github.com/apache/gravitino/pull/12253#issuecomment-5128768648
Yes — I ran a quick microbenchmark. Comparing the old `Ognl.getValue(String,
ctx)` (re-parses per call) with the new `Ognl.getValue(tree, ctx)` (AST
cached), 50k warmup + 200k measured iterations, a fresh `OgnlContext` per call,
`MockGravitinoAuthorizer`:
| expression (converted length) | old: re-parse | new: cached AST | saved
per eval |
|---|---|---|---|
| `METALAKE::OWNER \|\| CATALOG::CREATE_CATALOG` (217 ch) | 18.95 µs | 6.81
µs | 12.14 µs (64%) |
| `CATALOG::USE_CATALOG && SCHEMA::USE_SCHEMA && (TABLE::SELECT_TABLE \|\|
TABLE::MODIFY_TABLE)` (558 ch) | 26.64 µs | 7.20 µs | 19.44 µs (73%) |
| `FILTER_TABLE_AUTHORIZATION_EXPRESSION` (2538 ch) | 85.23 µs | 12.39 µs |
72.84 µs (85%) |
The saving is exactly the parse step (~10 µs for a small expression, ~67 µs
for the large list-filter one), and it grows with expression complexity. It
matters most on the per-object list-filter fallback
(`MetadataAuthzHelper.doFilter`), which evaluates once per listed table, so on
a large namespace this drops a per-table re-parse.
Environment: JDK 17 (Zulu), single JVM. This is an indicative loop
benchmark, not a JMH harness (no fork/blackhole), so treat the absolute numbers
as ballpark — the old-vs-new ratio is the point.
<details>
<summary>Benchmark code (throwaway, not part of this PR — drop into
<code>server-common/src/test/java/.../authorization/expression/</code> and run
<code>./gradlew :server-common:test --tests "*BenchOgnlParse*"</code>)</summary>
Gradle swallows test `System.out` by default, so either add `-i` /
`testLogging { showStandardStreams = true }`, or read the captured output under
`server-common/build/test-results/test/TEST-*BenchOgnlParse*.xml`
(`<system-out>`).
```java
package org.apache.gravitino.server.authorization.expression;
import java.util.HashMap;
import java.util.Map;
import ognl.Ognl;
import ognl.OgnlContext;
import org.apache.gravitino.Entity;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.UserPrincipal;
import org.apache.gravitino.authorization.AuthorizationRequestContext;
import org.apache.gravitino.server.authorization.MockGravitinoAuthorizer;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.junit.jupiter.api.Test;
public class BenchOgnlParse {
private static final String[] EXPRESSIONS = {
"METALAKE::OWNER || CATALOG::CREATE_CATALOG",
"CATALOG::USE_CATALOG && SCHEMA::USE_SCHEMA && (TABLE::SELECT_TABLE ||
TABLE::MODIFY_TABLE)",
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION
};
private static Map<Entity.EntityType, NameIdentifier> names() {
Map<Entity.EntityType, NameIdentifier> m = new HashMap<>();
m.put(Entity.EntityType.METALAKE,
NameIdentifierUtil.ofMetalake("testMetalake"));
m.put(Entity.EntityType.CATALOG,
NameIdentifierUtil.ofCatalog("testMetalake", "testCatalog"));
m.put(
Entity.EntityType.SCHEMA,
NameIdentifierUtil.ofSchema("testMetalake", "testCatalog",
"testSchema"));
m.put(
Entity.EntityType.TABLE,
NameIdentifierUtil.ofTable("testMetalake", "testCatalog",
"testSchema", "testTable"));
return m;
}
@Test
public void bench() throws Exception {
UserPrincipal principal = new UserPrincipal("tester");
MockGravitinoAuthorizer authorizer = new MockGravitinoAuthorizer();
for (String raw : EXPRESSIONS) {
String converted =
AuthorizationExpressionConverter.convertToOgnlExpression(raw);
Object tree = Ognl.parseExpression(converted);
for (int i = 0; i < 50_000; i++) { // warmup
Ognl.parseExpression(converted);
evalTree(tree, authorizer, principal);
evalString(converted, authorizer, principal);
}
int n = 200_000;
long t0 = System.nanoTime();
for (int i = 0; i < n; i++) {
Ognl.parseExpression(converted);
}
long parseNs = System.nanoTime() - t0;
long t1 = System.nanoTime();
for (int i = 0; i < n; i++) {
evalTree(tree, authorizer, principal);
}
long treeNs = System.nanoTime() - t1;
long t2 = System.nanoTime();
for (int i = 0; i < n; i++) {
evalString(converted, authorizer, principal);
}
long stringNs = System.nanoTime() - t2;
System.out.printf(
"%n=== expr (%d chars converted) ===%n"
+ "parseExpression only : %.2f us/op%n"
+ "getValue(tree,ctx) : %.2f us/op [new: parse cached]%n"
+ "getValue(string,ctx) : %.2f us/op [old: re-parse each
call]%n"
+ "per-eval saving : %.2f us/op (%.0f%%)%n",
converted.length(),
parseNs / 1000.0 / n,
treeNs / 1000.0 / n,
stringNs / 1000.0 / n,
(stringNs - treeNs) / 1000.0 / n,
100.0 * (stringNs - treeNs) / stringNs);
}
}
private static void evalTree(Object tree, MockGravitinoAuthorizer
authorizer, UserPrincipal p)
throws Exception {
Ognl.getValue(tree, buildContext(authorizer, p));
}
private static void evalString(String expr, MockGravitinoAuthorizer
authorizer, UserPrincipal p)
throws Exception {
Ognl.getValue(expr, buildContext(authorizer, p));
}
private static OgnlContext buildContext(MockGravitinoAuthorizer
authorizer, UserPrincipal p) {
OgnlContext ctx = Ognl.createDefaultContext(null);
ctx.put("principal", p);
ctx.put("authorizer", authorizer);
ctx.put("authorizationContext", new AuthorizationRequestContext());
ctx.put("entityType", null);
names()
.forEach(
(type, ident) -> {
ctx.put(type.name(),
NameIdentifierUtil.toMetadataObject(ident, type));
ctx.put(type.name() + "_NAME_IDENT", ident);
});
ctx.put("METALAKE_NAME", "testMetalake");
return ctx;
}
}
```
</details>
On `enforcer`: sounds good — this change is orthogonal (it only removes the
repeated AST parse), so it shouldn't conflict with removing the enforcer.
--
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]
