From 9c029eeb419cf140a2180194c1ff18816ba31201 Mon Sep 17 00:00:00 2001 From: Youssef Elghareeb Date: Fri, 19 Mar 2021 15:31:37 +0100 Subject: [PATCH] Convert SectionSortCache to use get/loader instead of getIfPresent/put Motivation: To follow the same pattern implemented by other caches. Also on googlesource.com we export metrics for cache loading times on cache "get" operations. With the current implementation of this cache, we can't inspect load latencies. With this change I also did two minor modifications: if the list is already sorted, we now cache the identity list instead of null (as done previously). Also I removed the check for duplicate entries in the input list; There is no comment of why this was done and I don't see that this is needed. The added test makes sure the logic for sorting works correctly. This is not extensively testing the comparator logic in com.google.gerrit.server.util.MostSpecificComparator as the sorting logic is based on a multitude of factors, but this logic is untouched in this change anyway. Change-Id: I63b15893ebc86d568f3ed09ecce3ac1b1459c034 --- .../server/permissions/SectionSortCache.java | 104 +++++++++--------- .../permissions/SectionSortCacheTest.java | 84 ++++++++++++++ 2 files changed, 136 insertions(+), 52 deletions(-) create mode 100644 javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java diff --git a/java/com/google/gerrit/server/permissions/SectionSortCache.java b/java/com/google/gerrit/server/permissions/SectionSortCache.java index 6081e9a65a..d800782156 100644 --- a/java/com/google/gerrit/server/permissions/SectionSortCache.java +++ b/java/com/google/gerrit/server/permissions/SectionSortCache.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.permissions; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.auto.value.AutoValue; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableList; @@ -28,6 +30,8 @@ import com.google.inject.name.Named; import java.util.ArrayList; import java.util.IdentityHashMap; import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; /** * Caches the order AccessSections should be sorted for evaluation. @@ -60,67 +64,62 @@ public class SectionSortCache { this.cache = cache; } - // Sorts the given sections, but does not disturb ordering between equally exact sections. + /** + * Sorts the given sections in-place, but does not disturb ordering between equally exact + * sections. + */ void sort(String ref, List sections) { final int cnt = sections.size(); if (cnt <= 1) { return; } - EntryKey key = EntryKey.create(ref, sections); - EntryVal val = cache.getIfPresent(key); - if (val != null) { - int[] srcIdx = val.order; - if (srcIdx != null) { - AccessSection[] srcList = copy(sections); - for (int i = 0; i < cnt; i++) { - sections.set(i, srcList[srcIdx[i]]); - } - } else { - // Identity transform. No sorting is required. - } + EntryVal val; + try { + val = cache.get(key, new Loader(key, sections)); + } catch (ExecutionException e) { + logger.atWarning().withCause(e).log("Error happened while sorting access sections."); + return; + } + ImmutableList order = val.order(); + List sorted = new ArrayList<>(); + for (int i = 0; i < cnt; i++) { + sorted.add(sections.get(order.get(i))); + } + for (int i = 0; i < cnt; i++) { + sections.set(i, sorted.get(i)); + } + } - } else { - boolean poison = false; + private static class Loader implements Callable { + private final List sections; + EntryKey key; + + Loader(EntryKey key, List sections) { + this.key = key; + this.sections = sections; + } + + @Override + public EntryVal call() throws Exception { + // We use IdentityHashMap (which uses reference equality for keys/values) to preserve distinct + // entries in the map for identical AccessSection keys IdentityHashMap srcMap = new IdentityHashMap<>(); - for (int i = 0; i < cnt; i++) { - poison |= srcMap.put(sections.get(i), i) != null; + for (int i = 0; i < sections.size(); i++) { + srcMap.put(sections.get(i), i); } - - sections.sort(new MostSpecificComparator(ref)); - - int[] srcIdx; - if (isIdentityTransform(sections, srcMap)) { - srcIdx = null; - } else { - srcIdx = new int[cnt]; - for (int i = 0; i < cnt; i++) { - srcIdx[i] = srcMap.get(sections.get(i)); - } - } - - if (poison) { - logger.atSevere().log("Received duplicate AccessSection instances, not caching sort"); - } else { - cache.put(key, new EntryVal(srcIdx)); + ImmutableList sorted = + sections.stream() + .sorted(new MostSpecificComparator(key.ref())) + .collect(toImmutableList()); + ImmutableList.Builder order = ImmutableList.builderWithExpectedSize(sections.size()); + for (int i = 0; i < sorted.size(); i++) { + order.add(srcMap.get(sorted.get(i))); } + return EntryVal.create(order.build()); } } - private static AccessSection[] copy(List sections) { - return sections.toArray(new AccessSection[sections.size()]); - } - - private static boolean isIdentityTransform( - List sections, IdentityHashMap srcMap) { - for (int i = 0; i < sections.size(); i++) { - if (i != srcMap.get(sections.get(i))) { - return false; - } - } - return true; - } - @AutoValue abstract static class EntryKey { public abstract String ref(); @@ -146,17 +145,18 @@ public class SectionSortCache { } } - static final class EntryVal { + @AutoValue + abstract static class EntryVal { /** * Maps the input index to the output index. * *

For {@code x == order[y]} the expression means move the item at source position {@code x} * to the output position {@code y}. */ - final int[] order; + abstract ImmutableList order(); - EntryVal(int[] order) { - this.order = order; + static EntryVal create(ImmutableList order) { + return new AutoValue_SectionSortCache_EntryVal(order); } } } diff --git a/javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java b/javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java new file mode 100644 index 0000000000..9ec162501b --- /dev/null +++ b/javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java @@ -0,0 +1,84 @@ +// Copyright (C) 2021 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.server.permissions; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.gerrit.entities.AccessSection; +import com.google.gerrit.server.permissions.SectionSortCache.EntryKey; +import com.google.gerrit.server.permissions.SectionSortCache.EntryVal; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.junit.Before; +import org.junit.Test; + +/** Test for {@link SectionSortCache} */ +public class SectionSortCacheTest { + private SectionSortCache sectionSortCache; + private Cache cache; + + private static final AccessSection sectionA = AccessSection.create("refs/heads/branch_1"); + private static final AccessSection sectionB = AccessSection.create("refs/base/branch_2"); + private static final String REF_BASE = "refs/base"; + + @Before + public void setup() { + cache = CacheBuilder.newBuilder().build(); + sectionSortCache = new SectionSortCache(cache); + } + + @Test + public void sortSingleElement() { + List input = new ArrayList<>(); + input.add(sectionA); + sectionSortCache.sort(REF_BASE, input); + assertThat(input).containsExactly(sectionA); + } + + @Test + public void sortMultiElements() { + List input = new ArrayList<>(); + input.add(sectionA); + input.add(sectionB); + sectionSortCache.sort(REF_BASE, input); + assertThat(input).containsExactly(sectionB, sectionA).inOrder(); + } + + @Test + public void sortMultiElementsWhenAlreadyOrdered() { + List input = new ArrayList<>(); + input.add(sectionB); + input.add(sectionA); + sectionSortCache.sort(REF_BASE, input); + assertThat(input).containsExactly(sectionB, sectionA).inOrder(); + } + + @Test + public void sortMultiElementsWithDuplicates() { + AccessSection sectionAClone = sectionA.toBuilder().build(); + AccessSection sectionBClone = sectionB.toBuilder().build(); + AccessSection[] input = {sectionBClone, sectionA, sectionAClone, sectionA, sectionB}; + List sorted = Arrays.asList(input); + sectionSortCache.sort(REF_BASE, sorted); + // Cache preserves relative order (reference equality) for identical elements + AccessSection[] expected = {sectionBClone, sectionB, sectionA, sectionAClone, sectionA}; + for (int i = 0; i < sorted.size(); i++) { + assert (sorted.get(i) == expected[i]); + } + } +}