opendev/gerrit
Code Issues Proposed changes

Reduce BooleanCondition predicate tree before evaluating permissions

Browse Source 
We use BooleanConditions to express visibility and the state (enabled/disabled)
of UIDescriptions in Gerrit. This was added so that checks to the
PermissionBackend can be delayed and batched together for better performance.

This has another advantage that was not leveraged until now: We can
reduce the evaluation trees and cut off branches that evaluate trivially
without further checks. This lets us cut down on the number of checks we
have to make to PermissionBackend overall and results in a performance
gain.

This commit adds logic to reduce the evaluation tree as well as tests.

Change-Id: I1ff675554abf0d0e67dc0618b1a4b3b47096026f
This commit is contained in:
Patrick Hiesel
2018-01-22 13:41:32 +01:00
parent 80ba7fa8c7
commit 2ce34c2b9e
5 changed files with 281 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

105
java/com/google/gerrit/extensions/conditions/BooleanCondition.java
View File

@@ -59,6 +59,40 @@ public abstract class BooleanCondition {
*/
public abstract <T> Iterable<T> children(Class<T> type);
/**
* Reduce evaluation tree by cutting off branches that evaluate trivially and replacing them with
* a leave note corresponding to the value the branch evaluated to.
*
* <p><code>
* Example 1 (T=True, F=False, C=non-trivial check):
* OR
* / \ => T
* C T
* Example 2 (cuts off a not-trivial check):
* AND
* / \ => F
* C F
* Example 3:
* AND
* / \ => F
* T F
* </code>
*
* <p>There is no guarantee that the resulting tree is minimal. The only guarantee made is that
* branches that evaluate trivially will be cut off and replaced by primitive values.
*/
public abstract BooleanCondition reduce();
/**
* Check if the condition evaluates to either {@code true} or {@code false} without providing
* additional information to the evaluation tree, e.g. through checks to a remote service such as
* {@code PermissionBackend}.
*
* <p>In this case, the tree can be reduced to skip all non-trivial checks resulting in a
* performance gain.
*/
protected abstract boolean evaluatesTrivially();
private static final class And extends BooleanCondition {
private final BooleanCondition a;
private final BooleanCondition b;
@@ -70,6 +104,10 @@ public abstract class BooleanCondition {
@Override
public boolean value() {
if (evaluatesTriviallyToExpectedValue(a, false)
|| evaluatesTriviallyToExpectedValue(b, false)) {
return false;
}
return a.value() && b.value();
}
@@ -78,6 +116,14 @@ public abstract class BooleanCondition {
return Iterables.concat(a.children(type), b.children(type));
}
@Override
public BooleanCondition reduce() {
if (evaluatesTrivially()) {
return Value.valueOf(value());
}
return new And(a.reduce(), b.reduce());
}
@Override
public int hashCode() {
return a.hashCode() * 31 + b.hashCode();
@@ -96,6 +142,13 @@ public abstract class BooleanCondition {
public String toString() {
return "(" + maybeTrim(a, getClass()) + " && " + maybeTrim(a, getClass()) + ")";
}
@Override
protected boolean evaluatesTrivially() {
return evaluatesTriviallyToExpectedValue(a, false)
|| evaluatesTriviallyToExpectedValue(b, false)
|| (a.evaluatesTrivially() && b.evaluatesTrivially());
}
}
private static final class Or extends BooleanCondition {
@@ -109,6 +162,10 @@ public abstract class BooleanCondition {
@Override
public boolean value() {
if (evaluatesTriviallyToExpectedValue(a, true)
|| evaluatesTriviallyToExpectedValue(b, true)) {
return true;
}
return a.value() || b.value();
}
@@ -117,6 +174,14 @@ public abstract class BooleanCondition {
return Iterables.concat(a.children(type), b.children(type));
}
@Override
public BooleanCondition reduce() {
if (evaluatesTrivially()) {
return Value.valueOf(value());
}
return new Or(a.reduce(), b.reduce());
}
@Override
public int hashCode() {
return a.hashCode() * 31 + b.hashCode();
@@ -135,6 +200,13 @@ public abstract class BooleanCondition {
public String toString() {
return "(" + maybeTrim(a, getClass()) + " || " + maybeTrim(a, getClass()) + ")";
}
@Override
protected boolean evaluatesTrivially() {
return evaluatesTriviallyToExpectedValue(a, true)
|| evaluatesTriviallyToExpectedValue(b, true)
|| (a.evaluatesTrivially() && b.evaluatesTrivially());
}
}
private static final class Not extends BooleanCondition {
@@ -154,6 +226,14 @@ public abstract class BooleanCondition {
return cond.children(type);
}
@Override
public BooleanCondition reduce() {
if (evaluatesTrivially()) {
return Value.valueOf(value());
}
return this;
}
@Override
public int hashCode() {
return cond.hashCode() * 31;
@@ -168,6 +248,11 @@ public abstract class BooleanCondition {
public String toString() {
return "!" + cond;
}
@Override
protected boolean evaluatesTrivially() {
return cond.evaluatesTrivially();
}
}
private static final class Value extends BooleanCondition {
@@ -187,6 +272,11 @@ public abstract class BooleanCondition {
return Collections.emptyList();
}
@Override
public BooleanCondition reduce() {
return this;
}
@Override
public int hashCode() {
return value ? 1 : 0;
@@ -201,9 +291,17 @@ public abstract class BooleanCondition {
public String toString() {
return Boolean.toString(value);
}
@Override
protected boolean evaluatesTrivially() {
return true;
}
}
/** Remove leading '(' and trailing ')' if the type is the same as the parent. */
/**
* Helper for use in toString methods. Remove leading '(' and trailing ')' if the type is the same
* as the parent.
*/
static String maybeTrim(BooleanCondition cond, Class<? extends BooleanCondition> type) {
String s = cond.toString();
if (cond.getClass() == type
@@ -214,4 +312,9 @@ public abstract class BooleanCondition {
}
return s;
}
private static boolean evaluatesTriviallyToExpectedValue(
BooleanCondition cond, boolean expectedValue) {
return cond.evaluatesTrivially() && (cond.value() == expectedValue);
}
}

4
java/com/google/gerrit/server/extensions/webui/UiActions.java
View File

@@ -86,11 +86,11 @@ public class UiActions {
}
private static Iterable<PermissionBackendCondition> visibleCondition(Description u) {
return u.getVisibleCondition().children(PermissionBackendCondition.class);
return u.getVisibleCondition().reduce().children(PermissionBackendCondition.class);
}
private static Iterable<PermissionBackendCondition> enabledCondition(Description u) {
return u.getEnabledCondition().children(PermissionBackendCondition.class);
return u.getEnabledCondition().reduce().children(PermissionBackendCondition.class);
}
@Nullable

13
java/com/google/gerrit/server/permissions/PermissionBackendCondition.java
View File

@@ -38,6 +38,19 @@ public abstract class PermissionBackendCondition
@Override
public abstract String toString();
@Override
public boolean evaluatesTrivially() {
// PermissionBackendCondition needs to contact PermissionBackend so trivial evaluation is not
// possible.
return false;
}
@Override
public BooleanCondition reduce() {
// No reductions can be made
return this;
}
public static class WithUser extends PermissionBackendCondition {
private final PermissionBackend.WithUser impl;
private final GlobalOrPluginPermission perm;

10
javatests/com/google/gerrit/extensions/conditions/BUILD Normal file
View File

@@ -0,0 +1,10 @@
load("//tools/bzl:junit.bzl", "junit_tests")
junit_tests(
name = "conditions_tests",
srcs = glob(["*.java"]),
deps = [
"//java/com/google/gerrit/extensions:lib",
"//lib:truth",
],
)

152
javatests/com/google/gerrit/extensions/conditions/BooleanConditionTest.java Normal file
View File

@@ -0,0 +1,152 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.extensions.conditions;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import static com.google.gerrit.extensions.conditions.BooleanCondition.not;
import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import static com.google.gerrit.extensions.conditions.BooleanCondition.valueOf;
import static org.junit.Assert.assertEquals;
import org.junit.Test;
public class BooleanConditionTest {
private static final BooleanCondition NO_TRIVIAL_EVALUATION =
new BooleanCondition() {
@Override
public boolean value() {
throw new UnsupportedOperationException("value() is not supported");
}
@Override
public <T> Iterable<T> children(Class<T> type) {
throw new UnsupportedOperationException("children(Class<T> type) is not supported");
}
@Override
public BooleanCondition reduce() {
return this;
}
@Override
protected boolean evaluatesTrivially() {
return false;
}
};
@Test
public void reduceAnd_CutOffNonTrivialWhenPossible() throws Exception {
BooleanCondition nonReduced = and(false, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceAnd_CutOffNonTrivialWhenPossibleSwapped() throws Exception {
BooleanCondition nonReduced = and(NO_TRIVIAL_EVALUATION, valueOf(false));
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceAnd_KeepNonTrivialWhenNoCutOffPossible() throws Exception {
BooleanCondition nonReduced = and(true, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = and(true, NO_TRIVIAL_EVALUATION);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceAnd_KeepNonTrivialWhenNoCutOffPossibleSwapped() throws Exception {
BooleanCondition nonReduced = and(NO_TRIVIAL_EVALUATION, valueOf(true));
BooleanCondition reduced = and(NO_TRIVIAL_EVALUATION, valueOf(true));
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_CutOffNonTrivialWhenPossible() throws Exception {
BooleanCondition nonReduced = or(true, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = valueOf(true);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_CutOffNonTrivialWhenPossibleSwapped() throws Exception {
BooleanCondition nonReduced = or(NO_TRIVIAL_EVALUATION, valueOf(true));
BooleanCondition reduced = valueOf(true);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_KeepNonTrivialWhenNoCutOffPossible() throws Exception {
BooleanCondition nonReduced = or(false, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = or(false, NO_TRIVIAL_EVALUATION);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_KeepNonTrivialWhenNoCutOffPossibleSwapped() throws Exception {
BooleanCondition nonReduced = or(NO_TRIVIAL_EVALUATION, valueOf(false));
BooleanCondition reduced = or(NO_TRIVIAL_EVALUATION, valueOf(false));
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceNot_ReduceIrrelevant() throws Exception {
BooleanCondition nonReduced = not(valueOf(true));
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceNot_ReduceIrrelevant2() throws Exception {
BooleanCondition nonReduced = not(valueOf(false));
BooleanCondition reduced = valueOf(true);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceNot_KeepNonTrivialWhenNoCutOffPossible() throws Exception {
BooleanCondition nonReduced = not(NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = not(NO_TRIVIAL_EVALUATION);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceComplexTreeToSingleValue() throws Exception {
// AND
// / \
// OR NOT
// / \ \
// NTE NTE TRUE
BooleanCondition nonReduced =
and(or(NO_TRIVIAL_EVALUATION, NO_TRIVIAL_EVALUATION), not(valueOf(true)));
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceComplexTreeToSmallerTree() throws Exception {
// AND
// / \
// OR OR
// / \ / \
// NTE NTE T F
BooleanCondition nonReduced =
and(or(NO_TRIVIAL_EVALUATION, NO_TRIVIAL_EVALUATION), or(valueOf(true), valueOf(false)));
BooleanCondition reduced = and(or(NO_TRIVIAL_EVALUATION, NO_TRIVIAL_EVALUATION), valueOf(true));
assertEquals(nonReduced.reduce(), reduced);
}
}