From dd8c3dab8af85a4c367949c06c648d1e6c9a9f4a Mon Sep 17 00:00:00 2001 From: Laura Hamelin Date: Wed, 24 Jul 2024 13:56:28 -0700 Subject: [PATCH] dfs: add configurable name to block cache table stats The addition of a name will help show statistics broken down per inner cache table when more than one cache table is used. The name configuration is obtained from the config subsection name prefixed by `dfs`, or `dfs` for the base case. Change-Id: Ia16c794f094d756441b779e3b1f1a3c992443509 --- .../storage/dfs/ClockBlockCacheTableTest.java | 49 +++++++++++++++++++ .../storage/dfs/DfsBlockCacheConfigTest.java | 34 +++++++++++++ .../dfs/PackExtBlockCacheTableTest.java | 32 ++++++++++++ .../storage/dfs/ClockBlockCacheTable.java | 13 ++++- .../storage/dfs/DfsBlockCacheConfig.java | 33 +++++++++++++ .../storage/dfs/DfsBlockCacheTable.java | 21 ++++++++ .../storage/dfs/PackExtBlockCacheTable.java | 37 +++++++++++--- 7 files changed, 212 insertions(+), 7 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTableTest.java 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 new file mode 100644 index 0000000000..cb68bbc515 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/ClockBlockCacheTableTest.java @@ -0,0 +1,49 @@ +package org.eclipse.jgit.internal.storage.dfs; + +import static org.eclipse.jgit.internal.storage.dfs.DfsBlockCacheConfig.DEFAULT_NAME; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + +import org.junit.Test; + +public class ClockBlockCacheTableTest { + private static final String NAME = "name"; + + @Test + public void getName_nameNotConfigured_returnsDefaultName() { + ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( + createBlockCacheConfig()); + + assertThat(cacheTable.getName(), equalTo(DEFAULT_NAME)); + } + + @Test + public void getName_nameConfigured_returnsConfiguredName() { + ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( + createBlockCacheConfig().setName(NAME)); + + assertThat(cacheTable.getName(), equalTo(NAME)); + } + + @Test + public void getBlockCacheStats_nameNotConfigured_returnsBlockCacheStatsWithDefaultName() { + ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( + createBlockCacheConfig()); + + assertThat(cacheTable.getBlockCacheStats().getName(), + equalTo(DEFAULT_NAME)); + } + + @Test + public void getBlockCacheStats_nameConfigured_returnsBlockCacheStatsWithConfiguredName() { + ClockBlockCacheTable cacheTable = new ClockBlockCacheTable( + createBlockCacheConfig().setName(NAME)); + + assertThat(cacheTable.getBlockCacheStats().getName(), equalTo(NAME)); + } + + private static DfsBlockCacheConfig createBlockCacheConfig() { + return new DfsBlockCacheConfig().setBlockSize(512) + .setConcurrencyLevel(4).setBlockLimit(1024); + } +} \ No newline at end of file diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java index c93f48d79c..4b7aae05f0 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java @@ -38,6 +38,7 @@ package org.eclipse.jgit.internal.storage.dfs; +import static org.eclipse.jgit.internal.storage.dfs.DfsBlockCacheConfig.DEFAULT_NAME; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_CORE_SECTION; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_DFS_CACHE_PREFIX; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_DFS_SECTION; @@ -48,6 +49,7 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACK_EXTENSIONS; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_STREAM_RATIO; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.closeTo; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertThrows; @@ -172,6 +174,38 @@ public class DfsBlockCacheConfigTest { is(indexConfig)); } + @Test + public void fromConfigs_baseConfigOnly_nameSetFromConfigDfsSubSection() { + Config config = new Config(); + + DfsBlockCacheConfig blockCacheConfig = new DfsBlockCacheConfig() + .fromConfig(config); + assertThat(blockCacheConfig.getName(), equalTo(DEFAULT_NAME)); + } + + @Test + public void fromConfigs_namesSetFromConfigDfsCachePrefixSubSections() { + Config config = new Config(); + config.setString(CONFIG_CORE_SECTION, CONFIG_DFS_SECTION, + CONFIG_KEY_STREAM_RATIO, "0.5"); + config.setString(CONFIG_CORE_SECTION, CONFIG_DFS_CACHE_PREFIX + "name1", + CONFIG_KEY_PACK_EXTENSIONS, PackExt.PACK.name()); + config.setString(CONFIG_CORE_SECTION, CONFIG_DFS_CACHE_PREFIX + "name2", + CONFIG_KEY_PACK_EXTENSIONS, PackExt.BITMAP_INDEX.name()); + + DfsBlockCacheConfig blockCacheConfig = new DfsBlockCacheConfig() + .fromConfig(config); + assertThat(blockCacheConfig.getName(), equalTo("dfs")); + assertThat( + blockCacheConfig.getPackExtCacheConfigurations().get(0) + .getPackExtCacheConfiguration().getName(), + equalTo("dfs.name1")); + assertThat( + blockCacheConfig.getPackExtCacheConfigurations().get(1) + .getPackExtCacheConfiguration().getName(), + equalTo("dfs.name2")); + } + @Test public void fromConfigs_dfsBlockCachePackExtConfigWithDuplicateExtensions_throws() { Config config = new Config(); 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 8c003e0cb3..f2a5abcacd 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 @@ -11,6 +11,7 @@ package org.eclipse.jgit.internal.storage.dfs; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.sameInstance; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertNotNull; @@ -37,6 +38,8 @@ import org.mockito.Mockito; @SuppressWarnings({ "boxing", "unchecked" }) public class PackExtBlockCacheTableTest { + private static final String CACHE_NAME = "CacheName"; + @Test public void fromBlockCacheConfigs_createsDfsPackExtBlockCacheTables() { DfsBlockCacheConfig cacheConfig = new DfsBlockCacheConfig(); @@ -384,6 +387,29 @@ public class PackExtBlockCacheTableTest { assertThat(tables.get(dfsStreamKey, 0), sameInstance(ref)); } + @Test + public void getName() { + DfsBlockCacheStats packStats = new DfsBlockCacheStats(); + PackExtBlockCacheTable tables = PackExtBlockCacheTable.fromCacheTables( + cacheTableWithStats(/* name= */ "defaultName", packStats), + Map.of(PackExt.PACK, cacheTableWithStats(/* name= */ "packName", + packStats))); + + assertThat(tables.getName(), equalTo("defaultName,packName")); + } + + @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))); + + assertThat(tables.getBlockCacheStats().getName(), + equalTo("defaultName,packName")); + } + @Test public void getBlockCacheStats_getCurrentSize_consolidatesAllTableCurrentSizes() { long[] currentSizes = createEmptyStatsArray(); @@ -573,7 +599,13 @@ public class PackExtBlockCacheTableTest { private static DfsBlockCacheTable cacheTableWithStats( DfsBlockCacheStats dfsBlockCacheStats) { + return cacheTableWithStats(CACHE_NAME, dfsBlockCacheStats); + } + + private static DfsBlockCacheTable cacheTableWithStats(String name, + DfsBlockCacheStats dfsBlockCacheStats) { DfsBlockCacheTable cacheTable = mock(DfsBlockCacheTable.class); + when(cacheTable.getName()).thenReturn(name); when(cacheTable.getBlockCacheStats()).thenReturn(dfsBlockCacheStats); return cacheTable; } 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 ce71a71d67..bbee23b061 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 @@ -47,6 +47,11 @@ import org.eclipse.jgit.internal.storage.pack.PackExt; * invocations is also fixed in size. */ final class ClockBlockCacheTable implements DfsBlockCacheTable { + /** + * Table name. + */ + private final String name; + /** Number of entries in {@link #table}. */ private final int tableSize; @@ -129,7 +134,8 @@ final class ClockBlockCacheTable implements DfsBlockCacheTable { -1, 0, null); clockHand.next = clockHand; - this.dfsBlockCacheStats = new DfsBlockCacheStats(); + this.name = cfg.getName(); + this.dfsBlockCacheStats = new DfsBlockCacheStats(this.name); this.refLockWaitTime = cfg.getRefLockWaitTimeConsumer(); this.indexEventConsumer = cfg.getIndexEventConsumer(); } @@ -139,6 +145,11 @@ final class ClockBlockCacheTable implements DfsBlockCacheTable { return dfsBlockCacheStats; } + @Override + public String getName() { + return name; + } + @Override public boolean hasBlock0(DfsStreamKey key) { HashEntry e1 = table.get(slot(key, 0)); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java index fa68b979fb..52eb9e0785 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java @@ -49,6 +49,10 @@ public class DfsBlockCacheConfig { /** Default number of max cache hits. */ public static final int DEFAULT_CACHE_HOT_MAX = 1; + static final String DEFAULT_NAME = ""; + + private String name; + private long blockLimit; private int blockSize; @@ -69,6 +73,7 @@ public class DfsBlockCacheConfig { * Create a default configuration. */ public DfsBlockCacheConfig() { + name = DEFAULT_NAME; setBlockLimit(32 * MB); setBlockSize(64 * KB); setStreamRatio(0.30); @@ -77,6 +82,29 @@ public class DfsBlockCacheConfig { packExtCacheConfigurations = Collections.emptyList(); } + /** + * Get the name for the block cache configured by this cache config. + * + * @return the name for the block cache configured by this cache config. + */ + public String getName() { + return name; + } + + /** + * Set the name for the block cache configured by this cache config. + *

+ * Made visible for testing. + * + * @param name + * the name for the block cache configured by this cache config. + * @return {@code this} + */ + DfsBlockCacheConfig setName(String name) { + this.name = name; + return this; + } + /** * Get maximum number bytes of heap memory to dedicate to caching pack file * data. @@ -308,6 +336,11 @@ public class DfsBlockCacheConfig { Long.valueOf(cfgBlockLimit), Long.valueOf(cfgBlockSize))); } + // Set name only if `core dfs` is configured, otherwise fall back to the + // default. + if (rc.getSubsections(section).contains(subSection)) { + this.name = subSection; + } setBlockLimit(cfgBlockLimit); setBlockSize(cfgBlockSize); 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 309f2d1595..de5dc23963 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 @@ -137,10 +137,19 @@ public interface DfsBlockCacheTable { */ BlockCacheStats getBlockCacheStats(); + /** + * Get the name of the table. + * + * @return this table's name. + */ + String getName(); + /** * Provides methods used with Block Cache statistics. */ interface BlockCacheStats { + String getName(); + /** * Get total number of bytes in the cache, per pack file extension. * @@ -195,6 +204,8 @@ public interface DfsBlockCacheTable { * Keeps track of stats for a Block Cache table. */ class DfsBlockCacheStats implements BlockCacheStats { + private final String name; + /** * Number of times a block was found in the cache, per pack file * extension. @@ -220,12 +231,22 @@ public interface DfsBlockCacheTable { private final AtomicReference liveBytes; DfsBlockCacheStats() { + this(""); + } + + DfsBlockCacheStats(String name) { + this.name = name; statHit = new AtomicReference<>(newCounters()); statMiss = new AtomicReference<>(newCounters()); statEvict = new AtomicReference<>(newCounters()); liveBytes = new AtomicReference<>(newCounters()); } + @Override + public String getName() { + return name; + } + /** * Increment the {@code statHit} count. * 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 858f731b70..7f21b3f10a 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 @@ -37,6 +37,11 @@ import org.eclipse.jgit.internal.storage.pack.PackExt; * type. */ class PackExtBlockCacheTable implements DfsBlockCacheTable { + /** + * Table name. + */ + private final String name; + private final DfsBlockCacheTable defaultBlockCacheTable; // Holds the unique tables backing the extBlockCacheTables values. @@ -120,13 +125,19 @@ class PackExtBlockCacheTable implements DfsBlockCacheTable { Set blockCacheTables = new HashSet<>(); blockCacheTables.add(defaultBlockCacheTable); blockCacheTables.addAll(packExtBlockCacheTables.values()); - return new PackExtBlockCacheTable(defaultBlockCacheTable, + String name = defaultBlockCacheTable.getName() + "," + + packExtBlockCacheTables.values().stream() + .map(DfsBlockCacheTable::getName).sorted() + .collect(Collectors.joining(",")); + return new PackExtBlockCacheTable(name, defaultBlockCacheTable, List.copyOf(blockCacheTables), packExtBlockCacheTables); } - private PackExtBlockCacheTable(DfsBlockCacheTable defaultBlockCacheTable, + private PackExtBlockCacheTable(String name, + DfsBlockCacheTable defaultBlockCacheTable, List blockCacheTableList, Map extBlockCacheTables) { + this.name = name; this.defaultBlockCacheTable = defaultBlockCacheTable; this.blockCacheTableList = blockCacheTableList; this.extBlockCacheTables = extBlockCacheTables; @@ -178,9 +189,15 @@ class PackExtBlockCacheTable implements DfsBlockCacheTable { @Override public BlockCacheStats getBlockCacheStats() { - return new CacheStats(blockCacheTableList.stream() - .map(DfsBlockCacheTable::getBlockCacheStats) - .collect(Collectors.toList())); + return new CacheStats(name, + blockCacheTableList.stream() + .map(DfsBlockCacheTable::getBlockCacheStats) + .collect(Collectors.toList())); + } + + @Override + public String getName() { + return name; } private DfsBlockCacheTable getTable(PackExt packExt) { @@ -198,12 +215,20 @@ class PackExtBlockCacheTable implements DfsBlockCacheTable { } private static class CacheStats implements BlockCacheStats { + private final String name; + private final List blockCacheStats; - private CacheStats(List blockCacheStats) { + private CacheStats(String name, List blockCacheStats) { + this.name = name; this.blockCacheStats = blockCacheStats; } + @Override + public String getName() { + return name; + } + @Override public long[] getCurrentSize() { long[] sums = emptyPackStats(); -- 2.39.5