From 4402f10247169cce87fe950c7e6647bf44298ebd Mon Sep 17 00:00:00 2001 From: Laura Hamelin Date: Wed, 24 Jul 2024 14:01:38 -0700 Subject: [PATCH] dfs: update getBlockCacheStats to return a List of BlockCacheStats Make available all underlying cache table stats for the used cache table implementation. The existing cache table stats implementation only allows a "global" view of the cache table statistics; it does not differentiate between all possible underlying cache tables used. This change allows callers to get the block cache stats broken down per underlying table. These cache stats are intended to be used for monitoring all cache tables independently. Existing usages of getBlockCacheStats now make use of AggregatedBlockCacheStats.fromStatsList to aggregate the list of BlockCacheStats into a single BlockCacheStats instance. Change-Id: I261b3f2849857172397657e5c674b11e09807f27 --- .../dfs/AggregatedBlockCacheStatsTest.java | 23 ++--- .../storage/dfs/ClockBlockCacheTableTest.java | 22 ++++- .../dfs/PackExtBlockCacheTableTest.java | 93 +++++++++++++++---- .../dfs/AggregatedBlockCacheStats.java | 12 +-- .../storage/dfs/ClockBlockCacheTable.java | 5 +- .../internal/storage/dfs/DfsBlockCache.java | 31 +++++-- .../storage/dfs/DfsBlockCacheTable.java | 15 ++- .../storage/dfs/PackExtBlockCacheTable.java | 9 +- 8 files changed, 150 insertions(+), 60 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStatsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStatsTest.java index ac769498e2..2c4b432a01 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStatsTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStatsTest.java @@ -24,9 +24,10 @@ public class AggregatedBlockCacheStatsTest { @Test public void getName() { BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("name", List.of()); + .fromStatsList(List.of()); - assertThat(aggregatedBlockCacheStats.getName(), equalTo("name")); + assertThat(aggregatedBlockCacheStats.getName(), + equalTo(AggregatedBlockCacheStats.class.getName())); } @Test @@ -46,8 +47,7 @@ public class AggregatedBlockCacheStatsTest { currentSizes[PackExt.INDEX.getPosition()] = 7; BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("name", - List.of(packStats, bitmapStats, indexStats)); + .fromStatsList(List.of(packStats, bitmapStats, indexStats)); assertArrayEquals(aggregatedBlockCacheStats.getCurrentSize(), currentSizes); @@ -73,8 +73,7 @@ public class AggregatedBlockCacheStatsTest { hitCounts[PackExt.INDEX.getPosition()] = 7; BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("name", - List.of(packStats, bitmapStats, indexStats)); + .fromStatsList(List.of(packStats, bitmapStats, indexStats)); assertArrayEquals(aggregatedBlockCacheStats.getHitCount(), hitCounts); } @@ -99,8 +98,7 @@ public class AggregatedBlockCacheStatsTest { missCounts[PackExt.INDEX.getPosition()] = 7; BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("name", - List.of(packStats, bitmapStats, indexStats)); + .fromStatsList(List.of(packStats, bitmapStats, indexStats)); assertArrayEquals(aggregatedBlockCacheStats.getMissCount(), missCounts); } @@ -131,8 +129,7 @@ public class AggregatedBlockCacheStatsTest { totalRequestCounts[PackExt.INDEX.getPosition()] = 14; BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("name", - List.of(packStats, bitmapStats, indexStats)); + .fromStatsList(List.of(packStats, bitmapStats, indexStats)); assertArrayEquals(aggregatedBlockCacheStats.getTotalRequestCount(), totalRequestCounts); @@ -160,8 +157,7 @@ public class AggregatedBlockCacheStatsTest { hitRatios[PackExt.INDEX.getPosition()] = 0; BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("Name", - List.of(packStats, bitmapStats, indexStats)); + .fromStatsList(List.of(packStats, bitmapStats, indexStats)); assertArrayEquals(aggregatedBlockCacheStats.getHitRatio(), hitRatios); } @@ -186,8 +182,7 @@ public class AggregatedBlockCacheStatsTest { evictions[PackExt.INDEX.getPosition()] = 7; BlockCacheStats aggregatedBlockCacheStats = AggregatedBlockCacheStats - .fromStatsList("Name", - List.of(packStats, bitmapStats, indexStats)); + .fromStatsList(List.of(packStats, bitmapStats, indexStats)); assertArrayEquals(aggregatedBlockCacheStats.getEvictions(), evictions); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTableTest.java index cb68bbc515..2e2f86bf80 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTableTest.java @@ -1,8 +1,14 @@ package org.eclipse.jgit.internal.storage.dfs; import static org.eclipse.jgit.internal.storage.dfs.DfsBlockCacheConfig.DEFAULT_NAME; +import static org.eclipse.jgit.internal.storage.dfs.DfsBlockCacheTable.BlockCacheStats; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.isA; + +import java.util.List; import org.junit.Test; @@ -30,7 +36,8 @@ public class ClockBlockCacheTableTest { ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( createBlockCacheConfig()); - assertThat(cacheTable.getBlockCacheStats().getName(), + assertThat(cacheTable.getBlockCacheStats(), hasSize(1)); + assertThat(cacheTable.getBlockCacheStats().get(0).getName(), equalTo(DEFAULT_NAME)); } @@ -39,7 +46,18 @@ public class ClockBlockCacheTableTest { ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( createBlockCacheConfig().setName(NAME)); - assertThat(cacheTable.getBlockCacheStats().getName(), equalTo(NAME)); + assertThat(cacheTable.getBlockCacheStats(), hasSize(1)); + assertThat(cacheTable.getBlockCacheStats().get(0).getName(), + equalTo(NAME)); + } + + @Test + public void getAllBlockCacheStats() { + ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( + createBlockCacheConfig()); + + List blockCacheStats = cacheTable.getBlockCacheStats(); + assertThat(blockCacheStats, contains(isA(BlockCacheStats.class))); } private static DfsBlockCacheConfig createBlockCacheConfig() { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTableTest.java index c5c964bcab..e7627bc4ab 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTableTest.java @@ -10,8 +10,10 @@ package org.eclipse.jgit.internal.storage.dfs; +import static org.eclipse.jgit.internal.storage.dfs.DfsBlockCacheTable.BlockCacheStats; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.sameInstance; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertNotNull; @@ -398,15 +400,51 @@ public class PackExtBlockCacheTableTest { } @Test - public void getBlockCacheStats_getName_returnsPackExtCacheTableName() { - DfsBlockCacheStats packStats = new DfsBlockCacheStats(); - PackExtBlockCacheTable tables = PackExtBlockCacheTable.fromCacheTables( - cacheTableWithStats(/* name= */ "defaultName", packStats), - Map.of(PackExt.PACK, cacheTableWithStats(/* name= */ "packName", - packStats))); + public void getAllBlockCacheStats() { + String defaultTableName = "default table"; + DfsBlockCacheStats defaultStats = new DfsBlockCacheStats( + defaultTableName); + incrementCounter(4, + () -> defaultStats.incrementHit(new TestKey(PackExt.REFTABLE))); + + String packTableName = "pack table"; + DfsBlockCacheStats packStats = new DfsBlockCacheStats(packTableName); + incrementCounter(5, + () -> packStats.incrementHit(new TestKey(PackExt.PACK))); - assertThat(tables.getBlockCacheStats().getName(), - equalTo("defaultName,packName")); + String bitmapTableName = "bitmap table"; + DfsBlockCacheStats bitmapStats = new DfsBlockCacheStats( + bitmapTableName); + incrementCounter(6, () -> bitmapStats + .incrementHit(new TestKey(PackExt.BITMAP_INDEX))); + + DfsBlockCacheTable defaultTable = cacheTableWithStats(defaultStats); + DfsBlockCacheTable packTable = cacheTableWithStats(packStats); + DfsBlockCacheTable bitmapTable = cacheTableWithStats(bitmapStats); + PackExtBlockCacheTable tables = PackExtBlockCacheTable + .fromCacheTables(defaultTable, Map.of(PackExt.PACK, packTable, + PackExt.BITMAP_INDEX, bitmapTable)); + + List statsList = tables.getBlockCacheStats(); + assertThat(statsList, hasSize(3)); + + long[] defaultTableHitCounts = createEmptyStatsArray(); + defaultTableHitCounts[PackExt.REFTABLE.getPosition()] = 4; + assertArrayEquals( + getCacheStatsByName(statsList, defaultTableName).getHitCount(), + defaultTableHitCounts); + + long[] packTableHitCounts = createEmptyStatsArray(); + packTableHitCounts[PackExt.PACK.getPosition()] = 5; + assertArrayEquals( + getCacheStatsByName(statsList, packTableName).getHitCount(), + packTableHitCounts); + + long[] bitmapHitCounts = createEmptyStatsArray(); + bitmapHitCounts[PackExt.BITMAP_INDEX.getPosition()] = 6; + assertArrayEquals( + getCacheStatsByName(statsList, bitmapTableName).getHitCount(), + bitmapHitCounts); } @Test @@ -431,7 +469,8 @@ public class PackExtBlockCacheTableTest { cacheTableWithStats(bitmapStats), PackExt.INDEX, cacheTableWithStats(indexStats))); - assertArrayEquals(tables.getBlockCacheStats().getCurrentSize(), + assertArrayEquals(AggregatedBlockCacheStats + .fromStatsList(tables.getBlockCacheStats()).getCurrentSize(), currentSizes); } @@ -460,7 +499,9 @@ public class PackExtBlockCacheTableTest { cacheTableWithStats(bitmapStats), PackExt.INDEX, cacheTableWithStats(indexStats))); - assertArrayEquals(tables.getBlockCacheStats().getHitCount(), hitCounts); + assertArrayEquals(AggregatedBlockCacheStats + .fromStatsList(tables.getBlockCacheStats()).getHitCount(), + hitCounts); } @Test @@ -488,7 +529,8 @@ public class PackExtBlockCacheTableTest { cacheTableWithStats(bitmapStats), PackExt.INDEX, cacheTableWithStats(indexStats))); - assertArrayEquals(tables.getBlockCacheStats().getMissCount(), + assertArrayEquals(AggregatedBlockCacheStats + .fromStatsList(tables.getBlockCacheStats()).getMissCount(), missCounts); } @@ -523,8 +565,9 @@ public class PackExtBlockCacheTableTest { cacheTableWithStats(bitmapStats), PackExt.INDEX, cacheTableWithStats(indexStats))); - assertArrayEquals(tables.getBlockCacheStats().getTotalRequestCount(), - totalRequestCounts); + assertArrayEquals(AggregatedBlockCacheStats + .fromStatsList(tables.getBlockCacheStats()) + .getTotalRequestCount(), totalRequestCounts); } @Test @@ -554,7 +597,9 @@ public class PackExtBlockCacheTableTest { cacheTableWithStats(bitmapStats), PackExt.INDEX, cacheTableWithStats(indexStats))); - assertArrayEquals(tables.getBlockCacheStats().getHitRatio(), hitRatios); + assertArrayEquals(AggregatedBlockCacheStats + .fromStatsList(tables.getBlockCacheStats()).getHitRatio(), + hitRatios); } @Test @@ -582,10 +627,21 @@ public class PackExtBlockCacheTableTest { cacheTableWithStats(bitmapStats), PackExt.INDEX, cacheTableWithStats(indexStats))); - assertArrayEquals(tables.getBlockCacheStats().getEvictions(), + assertArrayEquals(AggregatedBlockCacheStats + .fromStatsList(tables.getBlockCacheStats()).getEvictions(), evictions); } + private BlockCacheStats getCacheStatsByName( + List blockCacheStats, String name) { + for (BlockCacheStats entry : blockCacheStats) { + if (entry.getName().equals(name)) { + return entry; + } + } + return null; + } + private static void incrementCounter(int amount, Runnable fn) { for (int i = 0; i < amount; i++) { fn.run(); @@ -597,15 +653,16 @@ public class PackExtBlockCacheTableTest { } private static DfsBlockCacheTable cacheTableWithStats( - DfsBlockCacheStats dfsBlockCacheStats) { + BlockCacheStats dfsBlockCacheStats) { return cacheTableWithStats(CACHE_NAME, dfsBlockCacheStats); } private static DfsBlockCacheTable cacheTableWithStats(String name, - DfsBlockCacheStats dfsBlockCacheStats) { + BlockCacheStats dfsBlockCacheStats) { DfsBlockCacheTable cacheTable = mock(DfsBlockCacheTable.class); when(cacheTable.getName()).thenReturn(name); - when(cacheTable.getBlockCacheStats()).thenReturn(dfsBlockCacheStats); + when(cacheTable.getBlockCacheStats()) + .thenReturn(List.of(dfsBlockCacheStats)); return cacheTable; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStats.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStats.java index 743f4606c4..295b702fa7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStats.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/AggregatedBlockCacheStats.java @@ -20,27 +20,23 @@ import org.eclipse.jgit.internal.storage.pack.PackExt; * Aggregates values for all given {@link BlockCacheStats}. */ class AggregatedBlockCacheStats implements BlockCacheStats { - private final String name; - private final List blockCacheStats; - static BlockCacheStats fromStatsList(String name, + static BlockCacheStats fromStatsList( List blockCacheStats) { if (blockCacheStats.size() == 1) { return blockCacheStats.get(0); } - return new AggregatedBlockCacheStats(name, blockCacheStats); + return new AggregatedBlockCacheStats(blockCacheStats); } - private AggregatedBlockCacheStats(String name, - List blockCacheStats) { - this.name = name; + private AggregatedBlockCacheStats(List blockCacheStats) { this.blockCacheStats = blockCacheStats; } @Override public String getName() { - return name; + return AggregatedBlockCacheStats.class.getName(); } @Override diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTable.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTable.java index bbee23b061..587d482583 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTable.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTable.java @@ -12,6 +12,7 @@ package org.eclipse.jgit.internal.storage.dfs; import java.io.IOException; import java.time.Duration; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReferenceArray; @@ -141,8 +142,8 @@ final class ClockBlockCacheTable implements DfsBlockCacheTable { } @Override - public BlockCacheStats getBlockCacheStats() { - return dfsBlockCacheStats; + public List getBlockCacheStats() { + return List.of(dfsBlockCacheStats); } @Override diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java index 0334450fbe..f8e0831e1f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java @@ -11,7 +11,10 @@ package org.eclipse.jgit.internal.storage.dfs; +import static org.eclipse.jgit.internal.storage.dfs.DfsBlockCacheTable.BlockCacheStats; + import java.io.IOException; +import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.LongStream; @@ -124,7 +127,7 @@ public final class DfsBlockCache { * @return total number of bytes in the cache, per pack file extension. */ public long[] getCurrentSize() { - return dfsBlockCacheTable.getBlockCacheStats().getCurrentSize(); + return getAggregatedBlockCacheStats().getCurrentSize(); } /** @@ -143,7 +146,7 @@ public final class DfsBlockCache { * extension. */ public long[] getHitCount() { - return dfsBlockCacheTable.getBlockCacheStats().getHitCount(); + return getAggregatedBlockCacheStats().getHitCount(); } /** @@ -154,7 +157,7 @@ public final class DfsBlockCache { * extension. */ public long[] getMissCount() { - return dfsBlockCacheTable.getBlockCacheStats().getMissCount(); + return getAggregatedBlockCacheStats().getMissCount(); } /** @@ -163,7 +166,7 @@ public final class DfsBlockCache { * @return total number of requests (hit + miss), per pack file extension. */ public long[] getTotalRequestCount() { - return dfsBlockCacheTable.getBlockCacheStats().getTotalRequestCount(); + return getAggregatedBlockCacheStats().getTotalRequestCount(); } /** @@ -172,7 +175,7 @@ public final class DfsBlockCache { * @return hit ratios */ public long[] getHitRatio() { - return dfsBlockCacheTable.getBlockCacheStats().getHitRatio(); + return getAggregatedBlockCacheStats().getHitRatio(); } /** @@ -183,7 +186,18 @@ public final class DfsBlockCache { * file extension. */ public long[] getEvictions() { - return dfsBlockCacheTable.getBlockCacheStats().getEvictions(); + return getAggregatedBlockCacheStats().getEvictions(); + } + + /** + * Get the list of {@link BlockCacheStats} for all underlying caches. + *

+ * Useful in monitoring caches with breakdown. + * + * @return the list of {@link BlockCacheStats} for all underlying caches. + */ + public List getAllBlockCacheStats() { + return dfsBlockCacheTable.getBlockCacheStats(); } /** @@ -263,6 +277,11 @@ public final class DfsBlockCache { return dfsBlockCacheTable.get(key, position); } + private BlockCacheStats getAggregatedBlockCacheStats() { + return AggregatedBlockCacheStats + .fromStatsList(dfsBlockCacheTable.getBlockCacheStats()); + } + static final class Ref { final DfsStreamKey key; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTable.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTable.java index 43ba8c36ab..c3fd07b7bb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTable.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTable.java @@ -11,6 +11,7 @@ package org.eclipse.jgit.internal.storage.dfs; import java.io.IOException; +import java.util.List; /** * Block cache table. @@ -125,13 +126,17 @@ public interface DfsBlockCacheTable { T get(DfsStreamKey key, long position); /** - * Get the {@link BlockCacheStats} object for this block cache table's - * statistics. + * Get the list of {@link BlockCacheStats} held by this cache. + *

+ * The returned list has a {@link BlockCacheStats} per configured cache + * table, with a minimum of 1 {@link BlockCacheStats} object returned. + * + * Use {@link AggregatedBlockCacheStats} to combine the results of the stats + * in the list for an aggregated view of the cache's stats. * - * @return the {@link BlockCacheStats} tracking this block cache table's - * statistics. + * @return the list of {@link BlockCacheStats} held by this cache. */ - BlockCacheStats getBlockCacheStats(); + List getBlockCacheStats(); /** * Get the name of the table. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTable.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTable.java index e45643be84..28b021c68f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTable.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackExtBlockCacheTable.java @@ -188,11 +188,10 @@ class PackExtBlockCacheTable implements DfsBlockCacheTable { } @Override - public BlockCacheStats getBlockCacheStats() { - return AggregatedBlockCacheStats.fromStatsList(name, - blockCacheTableList.stream() - .map(DfsBlockCacheTable::getBlockCacheStats) - .collect(Collectors.toList())); + public List getBlockCacheStats() { + return blockCacheTableList.stream() + .flatMap(cacheTable -> cacheTable.getBlockCacheStats().stream()) + .collect(Collectors.toList()); } @Override -- 2.39.5