From 2ce34c2b9e25120384de65cb4b8bf9b735dcd796 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 22 Jan 2018 13:41:32 +0100 Subject: [PATCH] Reduce BooleanCondition predicate tree before evaluating permissions 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 --- .../conditions/BooleanCondition.java | 105 +++++++++++- .../server/extensions/webui/UiActions.java | 4 +- .../PermissionBackendCondition.java | 13 ++ .../google/gerrit/extensions/conditions/BUILD | 10 ++ .../conditions/BooleanConditionTest.java | 152 ++++++++++++++++++ 5 files changed, 281 insertions(+), 3 deletions(-) create mode 100644 javatests/com/google/gerrit/extensions/conditions/BUILD create mode 100644 javatests/com/google/gerrit/extensions/conditions/BooleanConditionTest.java diff --git a/java/com/google/gerrit/extensions/conditions/BooleanCondition.java b/java/com/google/gerrit/extensions/conditions/BooleanCondition.java index 950365af83..162dd99b18 100644 --- a/java/com/google/gerrit/extensions/conditions/BooleanCondition.java +++ b/java/com/google/gerrit/extensions/conditions/BooleanCondition.java @@ -59,6 +59,40 @@ public abstract class BooleanCondition { */ public abstract Iterable children(Class 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. + * + *

+ * 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 + * + * + *

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}. + * + *

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 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); + } } diff --git a/java/com/google/gerrit/server/extensions/webui/UiActions.java b/java/com/google/gerrit/server/extensions/webui/UiActions.java index 79a5d4c6fd..d9397f450d 100644 --- a/java/com/google/gerrit/server/extensions/webui/UiActions.java +++ b/java/com/google/gerrit/server/extensions/webui/UiActions.java @@ -86,11 +86,11 @@ public class UiActions { } private static Iterable visibleCondition(Description u) { - return u.getVisibleCondition().children(PermissionBackendCondition.class); + return u.getVisibleCondition().reduce().children(PermissionBackendCondition.class); } private static Iterable enabledCondition(Description u) { - return u.getEnabledCondition().children(PermissionBackendCondition.class); + return u.getEnabledCondition().reduce().children(PermissionBackendCondition.class); } @Nullable diff --git a/java/com/google/gerrit/server/permissions/PermissionBackendCondition.java b/java/com/google/gerrit/server/permissions/PermissionBackendCondition.java index 8d66e50546..5846a10ce0 100644 --- a/java/com/google/gerrit/server/permissions/PermissionBackendCondition.java +++ b/java/com/google/gerrit/server/permissions/PermissionBackendCondition.java @@ -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; diff --git a/javatests/com/google/gerrit/extensions/conditions/BUILD b/javatests/com/google/gerrit/extensions/conditions/BUILD new file mode 100644 index 0000000000..aebe347200 --- /dev/null +++ b/javatests/com/google/gerrit/extensions/conditions/BUILD @@ -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", + ], +) diff --git a/javatests/com/google/gerrit/extensions/conditions/BooleanConditionTest.java b/javatests/com/google/gerrit/extensions/conditions/BooleanConditionTest.java new file mode 100644 index 0000000000..f9f1fa85fc --- /dev/null +++ b/javatests/com/google/gerrit/extensions/conditions/BooleanConditionTest.java @@ -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 Iterable children(Class type) { + throw new UnsupportedOperationException("children(Class 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); + } +}