From fd08b59cb5ed74c4ae0b894cb93c94cc1b19e50a Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 12:44:09 +0000 Subject: [PATCH 01/27] docs(ENGKNOW-3657): add table-service memory diagnosis design spec Diagnosis-first spec: local load harness + profiler to pinpoint the dominant memory consumer behind table-service OOMs under load. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...7-table-service-memory-diagnosis-design.md | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md diff --git a/docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md b/docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md new file mode 100644 index 00000000..90ed0810 --- /dev/null +++ b/docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md @@ -0,0 +1,93 @@ +# ENGKNOW-3657 — Table-service Memory Usage: Diagnosis Design + +- **Ticket:** [ENGKNOW-3657](https://genedx.atlassian.net/browse/ENGKNOW-3657) — "GOR: Table-service memory usage" +- **Branch:** `ENGKNOW-3657-gor-table-service-memory-usage` +- **Status of problem:** Table-service pods hit OOMKilled under heavy load. Cause not yet pinned. +- **Scope of this task:** **Diagnosis only.** Reproduce, profile, and identify the dominant memory consumer with hard evidence. Any fix is a separate follow-up ticket. + +## Goal + +Reproduce the table-service OOM locally under a mixed read+write dictionary load, profile heap allocation and retention, and identify the **dominant** memory consumer, backed by heap-dump / JFR evidence. Deliver a findings document and a follow-up fix ticket. + +## Evidence driving the design + +From Grafana (`grafanacloud-genedx-prom`), crash window 2026-07-06: + +- Request rate peaked at ~5-6 req/s; ~700 requests over an 8h window — **low throughput**. +- OOM occurred despite low volume ⇒ **cost is per-operation, not throughput-driven**. A few heavy operations (large dictionaries, large writes) exhaust the heap. +- Mixed read+write load; ~322 inserted lines over the 8h window. +- The `tableservice_requests_count` recording rule collapses the `request_type` and `table` labels, so per-type / per-table split is **not** available from Prometheus. Loki logs can refine the mix later if the harness needs tighter calibration. + +**Implication:** the harness needs **modest concurrency but heavy per-operation weight** — large dictionaries and real writes — not high RPS. + +## Where the harness lives + +A new benchmark/test **in `gor-opensource`**. The memory-holding code lives here: + +- `model` — `org.gorpipe.gor.table.dictionary.*` (dictionary/table data structures) +- `drivers` — `org.gorpipe.s3.driver.*` (S3 read/write, multipart buffers) + +The table-service repo is a thin service over this library, so the leak is reproducible without deploying the k8s service. The harness runs as a `@Category(SlowTests)` JUnit test plus a `main` entrypoint for profiler-attached runs. + +## Components + +1. **Fixture builder** — generates synthetic dictionaries at prod-like scale. Configurable: number of lines, buckets, tags, and tables. Filesystem-backed first; an S3-backed variant second (to exercise the S3 driver and multipart write buffers). +2. **Load driver** — a thread pool issuing a mixed operation stream: + - Reads: open dictionary, tag-filtered `getOptimizedEntries`, iterate results. + - Writes: insert lines, bucketize, commit. + - Knobs: read:write ratio, concurrency, dictionary size, duration. + - Emits ops/s and latency so load is quantified. +3. **Memory observation** — runs at `-Xmx` set to the prod pod memory limit (pulled from the table-service helm values), with: + - `-XX:+HeapDumpOnOutOfMemoryError` + - JFR recording (allocation profile + old-gen occupancy) + - periodic `jcmd GC.heap_info` logging + - **RSS vs heap tracking** — the CRT-based S3 async client allocates off-heap; a heap dump alone would miss it. Use Native Memory Tracking (NMT) if off-heap growth is suspected. +4. **Analysis** — load the heap dump / JFR, rank retained size by class, and trace GC roots for the top consumers. + +## Data flow + +``` +fixture builder → dictionaries (fs / S3) + ↓ +load driver (thread pool, mixed read+write) + ↓ +gor library: table/dictionary (model) → drivers/S3 + ↓ +JFR + heap dump + GC.heap_info + RSS + ↓ +analysis → findings report +``` + +## Hypotheses to confirm or refute + +The profiler decides — these are starting points, not conclusions. + +1. **`DictionaryEntries` retention (top suspect).** + `model/.../table/dictionary/DictionaryEntries.java` holds `rawLines: List` plus two `ListMultimap`s (`tagHashToLines`, `contentHashToLines`). Memory scales with dictionary size × number of concurrently-held tables. Key question: are these retained past request end (e.g. via a session cache)? +2. **64 MB multipart write buffers.** + `gor.s3.write.chunksize` defaults to `1<<26` (64 MB), buffered as `byte[]` per part. Concurrent writes multiply this: 64 MB × concurrency. +3. **Caches.** + `DefaultTableAccessOptimizer.tagsToListCache` (100 full file-lists), the S3 client cache (CRT clients are heavy), and the S3 metadata cache (bounded at 10k — likely fine). +4. **An unlisted consumer** — surfaced by the retention profile. + +## Deliverables + +- A reusable load harness, checked in, which doubles as a perf/memory regression guard. +- A findings document: the dominant consumer, evidence (retained sizes, GC-root path), and a recommendation. +- A separate follow-up fix ticket. + +## Testing the harness + +- Reproduce OOM at prod-like `-Xmx`. +- Show memory scales with the dialed knob (dictionary size / concurrency) — confirms the harness stresses the right path. +- Keep the harness as a perf regression test. + +## Risks and fallbacks + +- **OOM won't reproduce locally** → capture a prod heap dump instead (add `-XX:+HeapDumpOnOutOfMemoryError` to the pod). +- **MinIO/localstack ≠ real S3 buffering** → run the S3 write-buffer variant against a real dev bucket. +- **Off-heap growth invisible to heap dump** → watch RSS vs heap; use NMT. + +## Open inputs + +- Prod pod memory limit (from table-service helm values / Grafana) → sets the harness `-Xmx`. From 58f142201b0f1f7dfd57f70e065f300edfc0fcd1 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:15:30 +0000 Subject: [PATCH 02/27] docs(ENGKNOW-3657): add table-service memory diagnosis implementation plan Co-Authored-By: Claude Opus 4.8 (1M context) --- ...now-3657-table-service-memory-diagnosis.md | 976 ++++++++++++++++++ 1 file changed, 976 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md diff --git a/docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md b/docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md new file mode 100644 index 00000000..aba1f2d9 --- /dev/null +++ b/docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md @@ -0,0 +1,976 @@ +# Table-service Memory Diagnosis Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Build a reusable local load harness that reproduces the table-service OOM under mixed read+write dictionary load, profile it, and identify the dominant memory consumer with evidence. + +**Architecture:** A Java harness in the `test` module (which depends on all main modules and holds shared test infra like `GorDictionarySetup`). A fixture builder generates dictionaries at prod-like scale; a thread-pool load driver issues a mixed read+write operation stream against `GorDictionaryTable`; a memory sampler records peak heap and RSS. A `main` entrypoint runs it under JFR / `-XX:+HeapDumpOnOutOfMemoryError` at a prod-like `-Xmx`; a `@Category(SlowTests)` JUnit validates the harness stresses the target path. A runbook + findings doc capture the diagnosis. + +**Tech Stack:** Java 17, JUnit 4 (`@Category(org.gorpipe.test.SlowTests)`), Gradle (`slowTest` task), JFR, `jcmd`, `java.lang.management` MXBeans. Dictionary API: `org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable`. Fixture helper: `org.gorpipe.test.GorDictionarySetup`. + +## Global Constraints + +- Package for all new harness code: `org.gorpipe.test.memory`. +- Harness production code lives in `test/src/main/java/...`; harness tests in `test/src/test/java/...`. +- Copyright header: copy the `BEGIN_COPYRIGHT ... END_COPYRIGHT` block verbatim from any existing file in the `test` module (e.g. `test/src/main/java/org/gorpipe/test/GorDictionarySetup.java`) at the top of every new `.java` file. +- Scope is **diagnosis only** — no fix to `DictionaryEntries`, S3 buffers, or caches in this plan. The fix is a separate ticket. +- Slow/heavy tests use `@org.junit.experimental.categories.Category(org.gorpipe.test.SlowTests.class)` so they run under `./gradlew :test:slowTest`, not the default `test` task. +- Commit trailer on every commit: + ``` + Co-Authored-By: Claude Opus 4.8 (1M context) + ``` + +## File Structure + +- Create `test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java` — immutable config of all load knobs (dict size, buckets, tags, tables, concurrency, read:write ratio, duration), built from defaults + system properties. +- Create `test/src/main/java/org/gorpipe/test/memory/MemorySampler.java` — background sampler recording peak heap-used and peak RSS. +- Create `test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java` — builds dictionaries at scale on a filesystem or S3 root using `GorDictionarySetup`. +- Create `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java` — thread pool issuing mixed read+write ops against `GorDictionaryTable`; returns an op-count summary. +- Create `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java` — `main` entrypoint: config → fixture → driver → summary; for profiler-attached runs. +- Create `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java` — `@Category(SlowTests)` test validating the harness stresses the dictionary path (peak heap scales with dict-size knob). +- Create `docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md` — exact commands to run the harness under JFR + heap-dump at prod `-Xmx`, and how to analyze the dump. +- Create `docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md` — findings template to fill after profiling. + +--- + +### Task 1: MemoryLoadConfig — load knobs + +**Files:** +- Create: `test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java` +- Test: `test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: `MemoryLoadConfig` with public final int/long fields `fileCount`, `rowsPerChr`, `bucketSize`, `tableCount`, `threads`, `readWritePercent` (0-100, percent of ops that are reads), `durationSeconds`; static factory `MemoryLoadConfig.fromSystemProperties()`; constructor `MemoryLoadConfig(int fileCount, int rowsPerChr, int bucketSize, int tableCount, int threads, int readWritePercent, int durationSeconds)`. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class UTestMemoryLoadConfig { + + @Test + public void defaultsAreSane() { + MemoryLoadConfig c = new MemoryLoadConfig(1000, 100, 100, 4, 8, 70, 30); + assertEquals(1000, c.fileCount); + assertEquals(70, c.readWritePercent); + assertTrue(c.threads > 0); + } + + @Test + public void systemPropertiesOverrideDefaults() { + System.setProperty("gor.memtest.fileCount", "5000"); + try { + MemoryLoadConfig c = MemoryLoadConfig.fromSystemProperties(); + assertEquals(5000, c.fileCount); + } finally { + System.clearProperty("gor.memtest.fileCount"); + } + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemoryLoadConfig"` +Expected: FAIL — `MemoryLoadConfig` does not exist (compilation error). + +- [ ] **Step 3: Write minimal implementation** + +```java +package org.gorpipe.test.memory; + +/** Immutable set of load knobs for the table-service memory harness. */ +public final class MemoryLoadConfig { + public final int fileCount; // data files per dictionary (~ dictionary lines) + public final int rowsPerChr; // rows per chromosome per data file + public final int bucketSize; // files per bucket + public final int tableCount; // dictionaries held concurrently + public final int threads; // load driver concurrency + public final int readWritePercent; // percent of ops that are reads (0-100) + public final int durationSeconds; + + public MemoryLoadConfig(int fileCount, int rowsPerChr, int bucketSize, int tableCount, + int threads, int readWritePercent, int durationSeconds) { + this.fileCount = fileCount; + this.rowsPerChr = rowsPerChr; + this.bucketSize = bucketSize; + this.tableCount = tableCount; + this.threads = threads; + this.readWritePercent = readWritePercent; + this.durationSeconds = durationSeconds; + } + + public static MemoryLoadConfig fromSystemProperties() { + return new MemoryLoadConfig( + intProp("gor.memtest.fileCount", 2000), + intProp("gor.memtest.rowsPerChr", 100), + intProp("gor.memtest.bucketSize", 100), + intProp("gor.memtest.tableCount", 8), + intProp("gor.memtest.threads", Runtime.getRuntime().availableProcessors()), + intProp("gor.memtest.readWritePercent", 70), + intProp("gor.memtest.durationSeconds", 60)); + } + + private static int intProp(String key, int def) { + return Integer.parseInt(System.getProperty(key, String.valueOf(def))); + } + + @Override + public String toString() { + return "MemoryLoadConfig{fileCount=" + fileCount + ", rowsPerChr=" + rowsPerChr + + ", bucketSize=" + bucketSize + ", tableCount=" + tableCount + + ", threads=" + threads + ", readWritePercent=" + readWritePercent + + ", durationSeconds=" + durationSeconds + "}"; + } +} +``` + +(Prepend the copyright header block above the `package` line.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemoryLoadConfig"` +Expected: PASS (2 tests). + +- [ ] **Step 5: Commit** + +```bash +git add test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java \ + test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): add MemoryLoadConfig knobs for memory harness + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 2: MemorySampler — peak heap + RSS + +**Files:** +- Create: `test/src/main/java/org/gorpipe/test/memory/MemorySampler.java` +- Test: `test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: `MemorySampler` with `void start()`, `void stop()`, `long peakHeapUsedBytes()`, `long peakRssBytes()` (returns `-1` if RSS unavailable on this OS), `long currentHeapUsedBytes()`. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.junit.Test; +import static org.junit.Assert.assertTrue; + +public class UTestMemorySampler { + + @Test + public void recordsPeakHeapAfterAllocation() throws Exception { + MemorySampler sampler = new MemorySampler(50); + sampler.start(); + byte[][] hold = new byte[64][]; + for (int i = 0; i < hold.length; i++) { + hold[i] = new byte[1024 * 1024]; // 64 MB total + Thread.sleep(5); + } + sampler.stop(); + assertTrue("peak heap should exceed 32MB", sampler.peakHeapUsedBytes() > 32L * 1024 * 1024); + assertTrue("hold retained", hold[0][0] == 0); // keep alive + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemorySampler"` +Expected: FAIL — `MemorySampler` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +```java +package org.gorpipe.test.memory; + +import java.lang.management.ManagementFactory; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +/** Background thread sampling heap-used (via MXBean) and RSS (via /proc on Linux). */ +public class MemorySampler { + private final long intervalMillis; + private volatile boolean running; + private volatile long peakHeap; + private volatile long peakRss = -1; + private Thread thread; + + public MemorySampler(long intervalMillis) { + this.intervalMillis = intervalMillis; + } + + public void start() { + running = true; + thread = new Thread(this::loop, "memory-sampler"); + thread.setDaemon(true); + thread.start(); + } + + private void loop() { + while (running) { + sampleOnce(); + try { Thread.sleep(intervalMillis); } catch (InterruptedException e) { return; } + } + } + + private void sampleOnce() { + long heap = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); + if (heap > peakHeap) peakHeap = heap; + long rss = readRssBytes(); + if (rss > peakRss) peakRss = rss; + } + + /** Reads VmRSS from /proc/self/status. Returns -1 if unavailable (e.g. macOS). */ + private long readRssBytes() { + Path status = Path.of("/proc/self/status"); + if (!Files.exists(status)) return -1; + try { + List lines = Files.readAllLines(status); + for (String line : lines) { + if (line.startsWith("VmRSS:")) { + String[] parts = line.trim().split("\\s+"); // "VmRSS: 12345 kB" + return Long.parseLong(parts[1]) * 1024; + } + } + } catch (Exception e) { + return -1; + } + return -1; + } + + public void stop() { + running = false; + if (thread != null) thread.interrupt(); + sampleOnce(); + } + + public long peakHeapUsedBytes() { return peakHeap; } + public long peakRssBytes() { return peakRss; } + public long currentHeapUsedBytes() { + return ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); + } +} +``` + +(Prepend copyright header.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemorySampler"` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add test/src/main/java/org/gorpipe/test/memory/MemorySampler.java \ + test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): add MemorySampler for peak heap/RSS tracking + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 3: DictionaryFixture — build dictionaries at scale (filesystem) + +**Files:** +- Create: `test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java` +- Test: `test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java` +- Reference (do not modify): `test/src/main/java/org/gorpipe/test/GorDictionarySetup.java`, `model/src/main/java/org/gorpipe/gor/table/dictionary/gor/GorDictionaryTable.java` + +**Interfaces:** +- Consumes: `MemoryLoadConfig` (Task 1). +- Produces: `DictionaryFixture` with constructor `DictionaryFixture(Path root)`; method `List createTables(MemoryLoadConfig config)` returning the `.gord` paths (one per table); method `GorDictionaryTable openTable(Path gordPath)` returning a built `GorDictionaryTable`. The `.gord` paths and `openTable` are consumed by Task 4. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class UTestDictionaryFixture { + + @Test + public void createsReadableTablesAtScale() throws Exception { + Path root = Files.createTempDirectory("memtest-fixture"); + DictionaryFixture fixture = new DictionaryFixture(root); + MemoryLoadConfig config = new MemoryLoadConfig(50, 10, 10, 3, 4, 70, 5); + + List tables = fixture.createTables(config); + + assertEquals(3, tables.size()); + for (Path gord : tables) { + assertTrue("gord file exists", Files.exists(gord)); + GorDictionaryTable table = fixture.openTable(gord); + List all = table.filter().get(); + assertTrue("dictionary has entries", all.size() > 0); + } + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixture"` +Expected: FAIL — `DictionaryFixture` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +```java +package org.gorpipe.test.memory; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.gorpipe.test.GorDictionarySetup; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Builds Gor dictionaries at prod-like scale for the memory harness. + * Wraps {@link GorDictionarySetup} to generate data files + a .gord dictionary per table. + */ +public class DictionaryFixture { + private final Path root; + + public DictionaryFixture(Path root) { + this.root = root; + } + + public List createTables(MemoryLoadConfig config) throws IOException { + List tables = new ArrayList<>(); + int[] chrs = {1, 2, 3}; + for (int t = 0; t < config.tableCount; t++) { + String name = "memtest_table_" + t; + String[] sources = new String[config.fileCount]; + for (int i = 0; i < config.fileCount; i++) sources[i] = "PN" + i; + Map> data = GorDictionarySetup.createDataFilesMap( + name, root, config.fileCount, chrs, config.rowsPerChr, "PN", true, sources); + + Path gord = root.resolve(name + ".gord"); + GorDictionaryTable table = new GorDictionaryTable.Builder<>(gord).build(); + table.insert(data); + table.save(); + tables.add(gord); + } + return tables; + } + + public GorDictionaryTable openTable(Path gordPath) { + return new GorDictionaryTable.Builder<>(gordPath).build(); + } +} +``` + +(Prepend copyright header. If `insert(Map)` or `save()` signatures differ at compile time, check `DictionaryTable.java:129,304` and adjust the calls — the public methods `insert(Map>)` and `save()` exist there.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixture"` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java \ + test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): add DictionaryFixture to build dictionaries at scale + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 4: TableServiceLoadDriver — mixed read+write load + +**Files:** +- Create: `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java` +- Test: `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java` + +**Interfaces:** +- Consumes: `MemoryLoadConfig` (Task 1), `DictionaryFixture` + `GorDictionaryTable` (Task 3). +- Produces: `TableServiceLoadDriver` with constructor `TableServiceLoadDriver(List tablePaths, DictionaryFixture fixture, MemoryLoadConfig config)`; method `LoadResult run()` where `LoadResult` is a public static nested class with public final fields `long readOps`, `long writeOps`, `long errors`. Consumed by Task 5. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.Assert.assertTrue; + +public class UTestTableServiceLoadDriver { + + @Test + public void runsMixedLoadAndCountsOps() throws Exception { + Path root = Files.createTempDirectory("memtest-driver"); + MemoryLoadConfig config = new MemoryLoadConfig(30, 10, 10, 2, 4, 70, 2); + DictionaryFixture fixture = new DictionaryFixture(root); + List tables = fixture.createTables(config); + + TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); + TableServiceLoadDriver.LoadResult result = driver.run(); + + assertTrue("did some reads", result.readOps > 0); + assertTrue("did some writes", result.writeOps > 0); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadDriver"` +Expected: FAIL — `TableServiceLoadDriver` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +```java +package org.gorpipe.test.memory; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; + +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.ThreadLocalRandom; + +/** + * Drives a mixed read+write load against a set of Gor dictionaries using a thread pool. + * Reads: tag-filtered {@code filter().tags(..).get()}. Writes: insert a new data line + save. + */ +public class TableServiceLoadDriver { + private final List tablePaths; + private final DictionaryFixture fixture; + private final MemoryLoadConfig config; + + public TableServiceLoadDriver(List tablePaths, DictionaryFixture fixture, MemoryLoadConfig config) { + this.tablePaths = tablePaths; + this.fixture = fixture; + this.config = config; + } + + public LoadResult run() throws InterruptedException { + AtomicLong readOps = new AtomicLong(); + AtomicLong writeOps = new AtomicLong(); + AtomicLong errors = new AtomicLong(); + long deadline = System.nanoTime() + config.durationSeconds * 1_000_000_000L; + + ExecutorService pool = Executors.newFixedThreadPool(config.threads); + for (int i = 0; i < config.threads; i++) { + pool.submit(() -> { + while (System.nanoTime() < deadline) { + Path gord = tablePaths.get(ThreadLocalRandom.current().nextInt(tablePaths.size())); + try { + if (ThreadLocalRandom.current().nextInt(100) < config.readWritePercent) { + doRead(gord); + readOps.incrementAndGet(); + } else { + doWrite(gord); + writeOps.incrementAndGet(); + } + } catch (Exception e) { + errors.incrementAndGet(); + } + } + }); + } + pool.shutdown(); + pool.awaitTermination(config.durationSeconds + 30, TimeUnit.SECONDS); + return new LoadResult(readOps.get(), writeOps.get(), errors.get()); + } + + private void doRead(Path gord) { + GorDictionaryTable table = fixture.openTable(gord); + String tag = "PN" + ThreadLocalRandom.current().nextInt(config.fileCount); + table.filter().tags(tag).get(); + } + + private void doWrite(Path gord) { + GorDictionaryTable table = fixture.openTable(gord); + int id = ThreadLocalRandom.current().nextInt(1_000_000); + // insert(String...) accepts raw dictionary lines: "file\talias". + table.insert("memtest_extra_" + id + ".gor\tEXTRA" + id); + table.save(); + } + + public static class LoadResult { + public final long readOps; + public final long writeOps; + public final long errors; + public LoadResult(long readOps, long writeOps, long errors) { + this.readOps = readOps; this.writeOps = writeOps; this.errors = errors; + } + @Override public String toString() { + return "reads=" + readOps + " writes=" + writeOps + " errors=" + errors; + } + } +} +``` + +(Prepend copyright header. `insert(String...)` and `save()` are on `DictionaryTable` at lines 187 and 129. If writes to the same table from multiple threads deadlock on the file lock, that is itself a finding — record it and reduce write concurrency in the config rather than changing product code.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadDriver"` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java \ + test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): add TableServiceLoadDriver for mixed read+write load + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 5: TableServiceLoadMain — profiler entrypoint + +**Files:** +- Create: `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java` +- Test: `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java` + +**Interfaces:** +- Consumes: `MemoryLoadConfig.fromSystemProperties()` (Task 1), `MemorySampler` (Task 2), `DictionaryFixture` (Task 3), `TableServiceLoadDriver` (Task 4). +- Produces: `TableServiceLoadMain` with `public static void main(String[] args)` and a testable `public static String runOnce(MemoryLoadConfig config, java.nio.file.Path root)` returning a one-line summary string containing `peakHeapMB=` and the op counts. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.Assert.assertTrue; + +public class UTestTableServiceLoadMain { + + @Test + public void runOnceProducesSummary() throws Exception { + Path root = Files.createTempDirectory("memtest-main"); + MemoryLoadConfig config = new MemoryLoadConfig(30, 10, 10, 2, 4, 70, 2); + String summary = TableServiceLoadMain.runOnce(config, root); + assertTrue("summary reports peak heap", summary.contains("peakHeapMB=")); + assertTrue("summary reports reads", summary.contains("reads=")); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadMain"` +Expected: FAIL — `TableServiceLoadMain` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +```java +package org.gorpipe.test.memory; + +import java.nio.file.Files; +import java.nio.file.Path; + +/** + * Entry point for profiler-attached runs of the table-service memory harness. + * Run with a prod-like heap and profiling flags, e.g.: + * java -Xmx2g -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp \ + * -XX:StartFlightRecording=filename=load.jfr,settings=profile \ + * -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ + * -cp org.gorpipe.test.memory.TableServiceLoadMain + */ +public class TableServiceLoadMain { + + public static void main(String[] args) throws Exception { + MemoryLoadConfig config = MemoryLoadConfig.fromSystemProperties(); + Path root = Files.createTempDirectory("memtest-run"); + System.out.println("Config: " + config); + System.out.println(runOnce(config, root)); + } + + public static String runOnce(MemoryLoadConfig config, Path root) throws Exception { + MemorySampler sampler = new MemorySampler(100); + sampler.start(); + try { + DictionaryFixture fixture = new DictionaryFixture(root); + var tables = fixture.createTables(config); + TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); + TableServiceLoadDriver.LoadResult result = driver.run(); + System.gc(); + Thread.sleep(200); // let sampler catch post-GC retained heap + sampler.stop(); + long peakHeapMB = sampler.peakHeapUsedBytes() / (1024 * 1024); + long peakRssMB = sampler.peakRssBytes() < 0 ? -1 : sampler.peakRssBytes() / (1024 * 1024); + return "peakHeapMB=" + peakHeapMB + " peakRssMB=" + peakRssMB + " " + result; + } finally { + sampler.stop(); + } + } +} +``` + +(Prepend copyright header.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadMain"` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java \ + test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): add TableServiceLoadMain profiler entrypoint + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 6: Scaling validation (SlowTests) + +**Files:** +- Create: `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java` + +**Interfaces:** +- Consumes: everything from Tasks 1-5. +- Produces: nothing (validation test only). + +This test proves the harness actually stresses the dictionary path: peak retained heap must grow when the `fileCount` (dictionary size) knob grows. If it doesn't, the harness is not exercising the suspected memory driver and must be fixed before profiling. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.gorpipe.test.SlowTests; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.Assert.assertTrue; + +@Category(SlowTests.class) +public class UTestTableServiceLoad { + + private long peakHeapForFileCount(int fileCount) throws Exception { + Path root = Files.createTempDirectory("memtest-scale-" + fileCount); + MemoryLoadConfig config = new MemoryLoadConfig(fileCount, 20, 100, 4, 4, 70, 3); + MemorySampler sampler = new MemorySampler(50); + sampler.start(); + DictionaryFixture fixture = new DictionaryFixture(root); + var tables = fixture.createTables(config); + new TableServiceLoadDriver(tables, fixture, config).run(); + System.gc(); + Thread.sleep(200); + sampler.stop(); + return sampler.peakHeapUsedBytes(); + } + + @Test + public void peakHeapScalesWithDictionarySize() throws Exception { + long small = peakHeapForFileCount(500); + long large = peakHeapForFileCount(8000); + assertTrue("peak heap should grow with dictionary size: small=" + small + " large=" + large, + large > small * 1.5); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails, then passes** + +Run: `./gradlew :test:slowTest --tests "org.gorpipe.test.memory.UTestTableServiceLoad"` +Expected: PASS if the harness stresses the dictionary path. If it FAILS (heap does not scale), the harness is not exercising the target — investigate the read/write paths in Tasks 3-4 before proceeding. Do not weaken the assertion to force a pass. + +- [ ] **Step 3: Commit** + +```bash +git add test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): validate harness stresses dictionary memory path + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 7: S3-backed fixture variant + +**Files:** +- Modify: `test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java` +- Test: `test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java` + +**Interfaces:** +- Consumes: `MemoryLoadConfig` (Task 1); the existing filesystem `createTables` (Task 3). +- Produces: on `DictionaryFixture`, a new constructor `DictionaryFixture(Path localRoot, String s3Root)` and method `List createTablesOnS3(MemoryLoadConfig config)` writing `.gord` + data to an `s3://bucket/prefix` root via the S3 driver, returning the `.gord` `Path`s (as `s3://...` URIs resolved through the gor `FileReader`). This variant exercises the S3 multipart write buffers (`gor.s3.write.chunksize`). + +This task confirms the 64 MB multipart-buffer hypothesis. It is gated on a dev bucket: the test skips (via `org.junit.Assume`) if the env var `GOR_MEMTEST_S3_ROOT` is unset, so CI without S3 credentials stays green. + +- [ ] **Step 1: Write the failing test** + +```java +package org.gorpipe.test.memory; + +import org.junit.Assume; +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.Assert.assertTrue; + +public class UTestDictionaryFixtureS3 { + + @Test + public void createsTablesOnS3WhenBucketConfigured() throws Exception { + String s3Root = System.getenv("GOR_MEMTEST_S3_ROOT"); // e.g. s3://my-dev-bucket/memtest + Assume.assumeTrue("GOR_MEMTEST_S3_ROOT not set; skipping S3 fixture test", s3Root != null); + + Path localRoot = Files.createTempDirectory("memtest-s3-local"); + MemoryLoadConfig config = new MemoryLoadConfig(20, 10, 10, 1, 2, 50, 2); + DictionaryFixture fixture = new DictionaryFixture(localRoot, s3Root); + + List tables = fixture.createTablesOnS3(config); + + assertTrue("created at least one S3 table", tables.size() >= 1); + } +} +``` + +- [ ] **Step 2: Run test to verify it is skipped locally** + +Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixtureS3"` +Expected: PASS as **skipped/ignored** (Assume fails when env unset). This confirms the gate works without credentials. + +- [ ] **Step 3: Write minimal implementation** + +Add to `DictionaryFixture` (keep the existing filesystem members and methods): + +```java + private final String s3Root; // null for filesystem-only fixtures + + public DictionaryFixture(Path root, String s3Root) { + this.root = root; + this.s3Root = s3Root; + } + + /** + * Builds dictionaries whose .gord and bucket files live under an s3:// root, + * exercising the S3 driver write path (multipart buffers). Data files are + * generated locally then written to S3 via the gor table's FileReader. + */ + public List createTablesOnS3(MemoryLoadConfig config) throws IOException { + if (s3Root == null) throw new IllegalStateException("s3Root not configured"); + List tables = new ArrayList<>(); + int[] chrs = {1, 2, 3}; + for (int t = 0; t < config.tableCount; t++) { + String name = "memtest_s3_table_" + t; + String[] sources = new String[config.fileCount]; + for (int i = 0; i < config.fileCount; i++) sources[i] = "PN" + i; + Map> data = GorDictionarySetup.createDataFilesMap( + name, root, config.fileCount, chrs, config.rowsPerChr, "PN", true, sources); + + String gordUri = s3Root.endsWith("/") ? s3Root + name + ".gord" : s3Root + "/" + name + ".gord"; + GorDictionaryTable table = new GorDictionaryTable.Builder<>(gordUri).build(); + table.insert(data); + table.save(); + tables.add(Path.of(gordUri)); + } + return tables; + } +``` + +(The existing single-arg constructor from Task 3 sets `s3Root = null`; update it to `this(root, null)` or add the field assignment. `GorDictionaryTable.Builder<>(String)` accepts a URI string per `GorDictionaryTable.java:64`.) + +- [ ] **Step 4: Run against a real dev bucket (manual, optional in CI)** + +Run: +```bash +GOR_MEMTEST_S3_ROOT=s3:///memtest \ + ./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixtureS3" +``` +Expected: PASS (not skipped) — `.gord` objects appear under the S3 prefix. Requires AWS/OCI credentials in the environment. + +- [ ] **Step 5: Commit** + +```bash +git add test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java \ + test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java +git commit -m "$(cat <<'EOF' +test(ENGKNOW-3657): add S3-backed dictionary fixture variant + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +### Task 8: Profiling runbook + findings doc + +**Files:** +- Create: `docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md` +- Create: `docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md` + +**Interfaces:** +- Consumes: `TableServiceLoadMain` (Task 5). +- Produces: documentation only. + +- [ ] **Step 1: Write the runbook** + +Create `docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md` with this content: + +````markdown +# ENGKNOW-3657 Profiling Runbook + +## 1. Get the prod heap ceiling +Fetch the table-service pod memory limit from its helm values (or Grafana +`kube_pod_container_resource_limits{resource="memory"}`). Use it as `-Xmx` below +so the local run reproduces the OOM. + +## 2. Build the classpath +```bash +./gradlew :test:testClasses +CP=$(./gradlew -q :test:printTestClasspath 2>/dev/null || echo "see note") +# If :printTestClasspath is not defined, run via gradle JavaExec or use the +# test runtime classpath from `./gradlew :test:dependencies`. +``` + +## 3. Run under JFR + heap-dump-on-OOM at the prod ceiling +```bash +java -Xmx \ + -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp/engknow3657.hprof \ + -XX:StartFlightRecording=filename=/tmp/engknow3657.jfr,settings=profile,dumponexit=true \ + -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ + -Dgor.memtest.threads=8 -Dgor.memtest.readWritePercent=70 \ + -Dgor.memtest.durationSeconds=300 \ + -cp "$CP" org.gorpipe.test.memory.TableServiceLoadMain +``` +Ramp `fileCount` / `tableCount` until it OOMs at the prod ceiling. + +## 4. Analyze +- Heap dump: open `engknow3657.hprof` in Eclipse MAT or `jhat`. Sort by + **retained size**. Run "Dominator Tree" and "Path to GC Roots" on the top + objects. Check the hypotheses: `DictionaryEntries.rawLines` / + `tagHashToLines` / `contentHashToLines`; `byte[]` part buffers (~64 MB); + cache maps. +- JFR: open `engknow3657.jfr` in JDK Mission Control → Memory → Allocation. + Identify top allocation sites. +- Off-heap: compare `peakRssMB` vs `peakHeapMB` from the harness summary. A + large gap points at native/CRT S3 buffers — rerun with `-XX:NativeMemoryTracking=summary` + and `jcmd VM.native_memory summary`. + +## 5. Fallback: prod heap dump +If it will not reproduce locally, add `-XX:+HeapDumpOnOutOfMemoryError` +`-XX:HeapDumpPath=/data` to the table-service pod JVM args and pull the dump +after the next OOM. +```` + +- [ ] **Step 2: Write the findings template** + +Create `docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md`: + +````markdown +# ENGKNOW-3657 Memory Findings + +> Fill in after running the profiling runbook. This is the deliverable of the diagnosis task. + +## Reproduction +- Prod heap ceiling used (`-Xmx`): __ +- Knobs that triggered OOM: fileCount=__ tableCount=__ threads=__ duration=__ +- Summary line from harness: __ + +## Dominant consumer +- Class / structure: __ +- Retained size: __ MB (__ % of heap) +- Path to GC roots: __ +- Retained past request end? (session cache / static): __ + +## Hypothesis verdicts +- [ ] DictionaryEntries retention (rawLines + multimaps): CONFIRMED / REFUTED — evidence: __ +- [ ] 64 MB multipart write buffers: CONFIRMED / REFUTED — evidence: __ +- [ ] Caches (tagsToListCache / client / metadata): CONFIRMED / REFUTED — evidence: __ +- [ ] Off-heap / CRT S3 (RSS >> heap): CONFIRMED / REFUTED — evidence: __ +- [ ] Unlisted consumer: __ + +## Recommendation +- Proposed fix direction: __ +- Follow-up ticket: __ +```` + +- [ ] **Step 3: Commit** + +```bash +git add docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md \ + docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md +git commit -m "$(cat <<'EOF' +docs(ENGKNOW-3657): add profiling runbook and findings template + +Co-Authored-By: Claude Opus 4.8 (1M context) +EOF +)" +``` + +--- + +## Post-plan: run the diagnosis + +After Task 8, execute the runbook, fill the findings doc, and open the follow-up fix ticket. The fix itself is out of scope for this plan. From 2cd7040a91f0a79a8b5763d9f633f86f97927897 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:17:12 +0000 Subject: [PATCH 03/27] test(ENGKNOW-3657): add MemoryLoadConfig knobs for memory harness Co-Authored-By: Claude Opus 4.8 (1M context) --- .../gorpipe/test/memory/MemoryLoadConfig.java | 68 +++++++++++++++++++ .../test/memory/UTestMemoryLoadConfig.java | 49 +++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java diff --git a/test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java b/test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java new file mode 100644 index 00000000..383684df --- /dev/null +++ b/test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java @@ -0,0 +1,68 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +/** Immutable set of load knobs for the table-service memory harness. */ +public final class MemoryLoadConfig { + public final int fileCount; // data files per dictionary (~ dictionary lines) + public final int rowsPerChr; // rows per chromosome per data file + public final int bucketSize; // files per bucket + public final int tableCount; // dictionaries held concurrently + public final int threads; // load driver concurrency + public final int readWritePercent; // percent of ops that are reads (0-100) + public final int durationSeconds; + + public MemoryLoadConfig(int fileCount, int rowsPerChr, int bucketSize, int tableCount, + int threads, int readWritePercent, int durationSeconds) { + this.fileCount = fileCount; + this.rowsPerChr = rowsPerChr; + this.bucketSize = bucketSize; + this.tableCount = tableCount; + this.threads = threads; + this.readWritePercent = readWritePercent; + this.durationSeconds = durationSeconds; + } + + public static MemoryLoadConfig fromSystemProperties() { + return new MemoryLoadConfig( + intProp("gor.memtest.fileCount", 2000), + intProp("gor.memtest.rowsPerChr", 100), + intProp("gor.memtest.bucketSize", 100), + intProp("gor.memtest.tableCount", 8), + intProp("gor.memtest.threads", Runtime.getRuntime().availableProcessors()), + intProp("gor.memtest.readWritePercent", 70), + intProp("gor.memtest.durationSeconds", 60)); + } + + private static int intProp(String key, int def) { + return Integer.parseInt(System.getProperty(key, String.valueOf(def))); + } + + @Override + public String toString() { + return "MemoryLoadConfig{fileCount=" + fileCount + ", rowsPerChr=" + rowsPerChr + + ", bucketSize=" + bucketSize + ", tableCount=" + tableCount + + ", threads=" + threads + ", readWritePercent=" + readWritePercent + + ", durationSeconds=" + durationSeconds + "}"; + } +} diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java b/test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java new file mode 100644 index 00000000..e10e7f66 --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java @@ -0,0 +1,49 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class UTestMemoryLoadConfig { + + @Test + public void defaultsAreSane() { + MemoryLoadConfig c = new MemoryLoadConfig(1000, 100, 100, 4, 8, 70, 30); + assertEquals(1000, c.fileCount); + assertEquals(70, c.readWritePercent); + assertTrue(c.threads > 0); + } + + @Test + public void systemPropertiesOverrideDefaults() { + System.setProperty("gor.memtest.fileCount", "5000"); + try { + MemoryLoadConfig c = MemoryLoadConfig.fromSystemProperties(); + assertEquals(5000, c.fileCount); + } finally { + System.clearProperty("gor.memtest.fileCount"); + } + } +} From 88d940f68bc30f8c0a4145a45d0e632e45dc7cfe Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:20:25 +0000 Subject: [PATCH 04/27] test(ENGKNOW-3657): add MemorySampler for peak heap/RSS tracking Co-Authored-By: Claude Opus 4.8 (1M context) --- .../gorpipe/test/memory/MemorySampler.java | 92 +++++++++++++++++++ .../test/memory/UTestMemorySampler.java | 43 +++++++++ 2 files changed, 135 insertions(+) create mode 100644 test/src/main/java/org/gorpipe/test/memory/MemorySampler.java create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java diff --git a/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java b/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java new file mode 100644 index 00000000..3d8588da --- /dev/null +++ b/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java @@ -0,0 +1,92 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import java.lang.management.ManagementFactory; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +/** Background thread sampling heap-used (via MXBean) and RSS (via /proc on Linux). */ +public class MemorySampler { + private final long intervalMillis; + private volatile boolean running; + private volatile long peakHeap; + private volatile long peakRss = -1; + private Thread thread; + + public MemorySampler(long intervalMillis) { + this.intervalMillis = intervalMillis; + } + + public void start() { + running = true; + thread = new Thread(this::loop, "memory-sampler"); + thread.setDaemon(true); + thread.start(); + } + + private void loop() { + while (running) { + sampleOnce(); + try { Thread.sleep(intervalMillis); } catch (InterruptedException e) { return; } + } + } + + private void sampleOnce() { + long heap = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); + if (heap > peakHeap) peakHeap = heap; + long rss = readRssBytes(); + if (rss > peakRss) peakRss = rss; + } + + /** Reads VmRSS from /proc/self/status. Returns -1 if unavailable (e.g. macOS). */ + private long readRssBytes() { + Path status = Path.of("/proc/self/status"); + if (!Files.exists(status)) return -1; + try { + List lines = Files.readAllLines(status); + for (String line : lines) { + if (line.startsWith("VmRSS:")) { + String[] parts = line.trim().split("\\s+"); // "VmRSS: 12345 kB" + return Long.parseLong(parts[1]) * 1024; + } + } + } catch (Exception e) { + return -1; + } + return -1; + } + + public void stop() { + running = false; + if (thread != null) thread.interrupt(); + sampleOnce(); + } + + public long peakHeapUsedBytes() { return peakHeap; } + public long peakRssBytes() { return peakRss; } + public long currentHeapUsedBytes() { + return ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); + } +} diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java b/test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java new file mode 100644 index 00000000..b0e61a47 --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java @@ -0,0 +1,43 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.junit.Test; +import static org.junit.Assert.assertTrue; + +public class UTestMemorySampler { + + @Test + public void recordsPeakHeapAfterAllocation() throws Exception { + MemorySampler sampler = new MemorySampler(50); + sampler.start(); + byte[][] hold = new byte[64][]; + for (int i = 0; i < hold.length; i++) { + hold[i] = new byte[1024 * 1024]; // 64 MB total + Thread.sleep(5); + } + sampler.stop(); + assertTrue("peak heap should exceed 32MB", sampler.peakHeapUsedBytes() > 32L * 1024 * 1024); + assertTrue("hold retained", hold[0][0] == 0); // keep alive + } +} From f7ed0db2506d787dbf299b12d522bc4e5e99d1eb Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:23:47 +0000 Subject: [PATCH 05/27] fix(ENGKNOW-3657): make MemorySampler peak tracking race-free Co-Authored-By: Claude Opus 4.8 (1M context) --- .../gorpipe/test/memory/MemorySampler.java | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java b/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java index 3d8588da..06ce0ab2 100644 --- a/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java +++ b/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java @@ -26,13 +26,14 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.List; +import java.util.concurrent.atomic.AtomicLong; /** Background thread sampling heap-used (via MXBean) and RSS (via /proc on Linux). */ public class MemorySampler { private final long intervalMillis; private volatile boolean running; - private volatile long peakHeap; - private volatile long peakRss = -1; + private final AtomicLong peakHeap = new AtomicLong(); + private final AtomicLong peakRss = new AtomicLong(-1); private Thread thread; public MemorySampler(long intervalMillis) { @@ -49,15 +50,22 @@ public void start() { private void loop() { while (running) { sampleOnce(); - try { Thread.sleep(intervalMillis); } catch (InterruptedException e) { return; } + try { + Thread.sleep(intervalMillis); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } } } private void sampleOnce() { long heap = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); - if (heap > peakHeap) peakHeap = heap; + peakHeap.accumulateAndGet(heap, Math::max); long rss = readRssBytes(); - if (rss > peakRss) peakRss = rss; + if (rss >= 0) { + peakRss.accumulateAndGet(rss, Math::max); + } } /** Reads VmRSS from /proc/self/status. Returns -1 if unavailable (e.g. macOS). */ @@ -80,12 +88,19 @@ private long readRssBytes() { public void stop() { running = false; - if (thread != null) thread.interrupt(); + if (thread != null) { + thread.interrupt(); + try { + thread.join(1000); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } sampleOnce(); } - public long peakHeapUsedBytes() { return peakHeap; } - public long peakRssBytes() { return peakRss; } + public long peakHeapUsedBytes() { return peakHeap.get(); } + public long peakRssBytes() { return peakRss.get(); } public long currentHeapUsedBytes() { return ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); } From ad2815c8664714b6c6b9af1e97445cbaf7b7497a Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:28:04 +0000 Subject: [PATCH 06/27] test(ENGKNOW-3657): add DictionaryFixture to build dictionaries at scale Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/DictionaryFixture.java | 67 +++++++++++++++++++ .../test/memory/UTestDictionaryFixture.java | 54 +++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java diff --git a/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java b/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java new file mode 100644 index 00000000..8ef7daf1 --- /dev/null +++ b/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java @@ -0,0 +1,67 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.gorpipe.test.GorDictionarySetup; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Builds Gor dictionaries at prod-like scale for the memory harness. + * Wraps {@link GorDictionarySetup} to generate data files + a .gord dictionary per table. + */ +public class DictionaryFixture { + private final Path root; + + public DictionaryFixture(Path root) { + this.root = root; + } + + public List createTables(MemoryLoadConfig config) throws IOException { + List tables = new ArrayList<>(); + int[] chrs = {1, 2, 3}; + for (int t = 0; t < config.tableCount; t++) { + String name = "memtest_table_" + t; + String[] sources = new String[config.fileCount]; + for (int i = 0; i < config.fileCount; i++) sources[i] = "PN" + i; + Map> data = GorDictionarySetup.createDataFilesMap( + name, root, config.fileCount, chrs, config.rowsPerChr, "PN", true, sources); + + Path gord = root.resolve(name + ".gord"); + GorDictionaryTable table = new GorDictionaryTable.Builder<>(gord).build(); + table.insert(data); + table.save(); + tables.add(gord); + } + return tables; + } + + public GorDictionaryTable openTable(Path gordPath) { + return new GorDictionaryTable.Builder<>(gordPath).build(); + } +} diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java b/test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java new file mode 100644 index 00000000..bbad5a2c --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java @@ -0,0 +1,54 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class UTestDictionaryFixture { + + @Test + public void createsReadableTablesAtScale() throws Exception { + Path root = Files.createTempDirectory("memtest-fixture"); + DictionaryFixture fixture = new DictionaryFixture(root); + MemoryLoadConfig config = new MemoryLoadConfig(50, 10, 10, 3, 4, 70, 5); + + List tables = fixture.createTables(config); + + assertEquals(3, tables.size()); + for (Path gord : tables) { + assertTrue("gord file exists", Files.exists(gord)); + GorDictionaryTable table = fixture.openTable(gord); + List all = table.filter().get(); + assertTrue("dictionary has entries", all.size() > 0); + } + } +} From 606516c9a0168ae09f6f132ed65962bb2ed332e0 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:36:39 +0000 Subject: [PATCH 07/27] test(ENGKNOW-3657): add TableServiceLoadDriver for mixed read+write load Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/TableServiceLoadDriver.java | 116 ++++++++++++++++++ .../memory/UTestTableServiceLoadDriver.java | 48 ++++++++ 2 files changed, 164 insertions(+) create mode 100644 test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java diff --git a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java new file mode 100644 index 00000000..d794b55f --- /dev/null +++ b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java @@ -0,0 +1,116 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; + +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.ThreadLocalRandom; + +/** + * Drives a mixed read+write load against a set of Gor dictionaries using a thread pool. + * Reads: tag-filtered {@code filter().tags(..).get()}. Writes: insert a new data line + save. + */ +public class TableServiceLoadDriver { + private final List tablePaths; + private final DictionaryFixture fixture; + private final MemoryLoadConfig config; + + public TableServiceLoadDriver(List tablePaths, DictionaryFixture fixture, MemoryLoadConfig config) { + this.tablePaths = tablePaths; + this.fixture = fixture; + this.config = config; + } + + public LoadResult run() throws InterruptedException { + AtomicLong readOps = new AtomicLong(); + AtomicLong writeOps = new AtomicLong(); + AtomicLong errors = new AtomicLong(); + long deadline = System.nanoTime() + config.durationSeconds * 1_000_000_000L; + + ExecutorService pool = Executors.newFixedThreadPool(config.threads); + for (int i = 0; i < config.threads; i++) { + pool.submit(() -> { + while (System.nanoTime() < deadline) { + Path gord = tablePaths.get(ThreadLocalRandom.current().nextInt(tablePaths.size())); + try { + if (ThreadLocalRandom.current().nextInt(100) < config.readWritePercent) { + doRead(gord); + readOps.incrementAndGet(); + } else { + doWrite(gord); + writeOps.incrementAndGet(); + } + } catch (Exception e) { + errors.incrementAndGet(); + } + } + }); + } + pool.shutdown(); + pool.awaitTermination(config.durationSeconds + 30, TimeUnit.SECONDS); + return new LoadResult(readOps.get(), writeOps.get(), errors.get()); + } + + private void doRead(Path gord) { + GorDictionaryTable table = fixture.openTable(gord); + String tag = "PN" + ThreadLocalRandom.current().nextInt(config.fileCount); + table.filter().tags(tag).get(); + } + + private void doWrite(Path gord) throws IOException { + GorDictionaryTable table = fixture.openTable(gord); + int id = ThreadLocalRandom.current().nextInt(1_000_000); + // insert() validates that the referenced file exists and its header column + // count matches the dictionary's existing entries, so write a minimal real + // data file (matching DictionaryFixture/GorDictionarySetup's 6-column layout) + // before registering it. + Path dataFile = gord.getParent().resolve("memtest_extra_" + id + ".gor"); + try (PrintWriter out = new PrintWriter(dataFile.toFile())) { + out.println("Chr\tPos\tPN\tChromoInfo\tConstData\tRandomData"); + out.println("chr1\t1\tEXTRA" + id + "\tinfo\tconst\t1"); + } + // insert(String...) accepts raw dictionary lines: "file\talias". + table.insert(dataFile + "\tEXTRA" + id); + table.save(); + } + + public static class LoadResult { + public final long readOps; + public final long writeOps; + public final long errors; + public LoadResult(long readOps, long writeOps, long errors) { + this.readOps = readOps; this.writeOps = writeOps; this.errors = errors; + } + @Override public String toString() { + return "reads=" + readOps + " writes=" + writeOps + " errors=" + errors; + } + } +} diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java new file mode 100644 index 00000000..797ba854 --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java @@ -0,0 +1,48 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.Assert.assertTrue; + +public class UTestTableServiceLoadDriver { + + @Test + public void runsMixedLoadAndCountsOps() throws Exception { + Path root = Files.createTempDirectory("memtest-driver"); + MemoryLoadConfig config = new MemoryLoadConfig(30, 10, 10, 2, 4, 70, 2); + DictionaryFixture fixture = new DictionaryFixture(root); + List tables = fixture.createTables(config); + + TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); + TableServiceLoadDriver.LoadResult result = driver.run(); + + assertTrue("did some reads", result.readOps > 0); + assertTrue("did some writes", result.writeOps > 0); + } +} From 34545239653bf66fd6005015dc157614d7c644b9 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:47:38 +0000 Subject: [PATCH 08/27] fix(ENGKNOW-3657): write through locked table transaction in load driver Use the same exclusive-lock write path as the real Table Service so concurrent writes serialize instead of racing on a shared temp file and silently losing inserts. Log caught op errors; shutdownNow on await timeout. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/TableServiceLoadDriver.java | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java index d794b55f..e53354a7 100644 --- a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java +++ b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java @@ -22,7 +22,12 @@ package org.gorpipe.test.memory; +import org.gorpipe.gor.manager.TableManager; import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.gorpipe.gor.table.lock.ExclusiveFileTableLock; +import org.gorpipe.gor.table.lock.TableTransaction; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.io.PrintWriter; @@ -39,6 +44,8 @@ * Reads: tag-filtered {@code filter().tags(..).get()}. Writes: insert a new data line + save. */ public class TableServiceLoadDriver { + private static final Logger log = LoggerFactory.getLogger(TableServiceLoadDriver.class); + private final List tablePaths; private final DictionaryFixture fixture; private final MemoryLoadConfig config; @@ -70,12 +77,18 @@ public LoadResult run() throws InterruptedException { } } catch (Exception e) { errors.incrementAndGet(); + log.debug("load op failed: {}: {}", e.getClass().getSimpleName(), e.getMessage()); } } }); } pool.shutdown(); - pool.awaitTermination(config.durationSeconds + 30, TimeUnit.SECONDS); + boolean terminated = pool.awaitTermination(config.durationSeconds + 30, TimeUnit.SECONDS); + if (!terminated) { + // Workers are still alive well past the expected run time; force them down so + // they don't leak and keep mutating state after run() has returned. + pool.shutdownNow(); + } return new LoadResult(readOps.get(), writeOps.get(), errors.get()); } @@ -86,7 +99,6 @@ private void doRead(Path gord) { } private void doWrite(Path gord) throws IOException { - GorDictionaryTable table = fixture.openTable(gord); int id = ThreadLocalRandom.current().nextInt(1_000_000); // insert() validates that the referenced file exists and its header column // count matches the dictionary's existing entries, so write a minimal real @@ -97,9 +109,20 @@ private void doWrite(Path gord) throws IOException { out.println("Chr\tPos\tPN\tChromoInfo\tConstData\tRandomData"); out.println("chr1\t1\tEXTRA" + id + "\tinfo\tconst\t1"); } - // insert(String...) accepts raw dictionary lines: "file\talias". - table.insert(dataFile + "\tEXTRA" + id); - table.save(); + + // Write through the same exclusive-lock write path the real Table Service uses + // (see TableManager#insert): opening a write transaction acquires the exclusive + // file lock, reloads the table's fresh on-disk state under that lock, and (on + // commit + close) saves under the same lock. This serializes concurrent writers + // to the same dictionary instead of racing on independent last-writer-wins temp + // files, which was silently dropping inserts under contention. + GorDictionaryTable table = fixture.openTable(gord); + try (TableTransaction trans = TableTransaction.openWriteTransaction( + ExclusiveFileTableLock.class, table, table.getName(), TableManager.DEFAULT_LOCK_TIMEOUT)) { + // insert(String...) accepts raw dictionary lines: "file\talias". + table.insert(dataFile + "\tEXTRA" + id); + trans.commit(); + } } public static class LoadResult { From 65e1c54e640808f8647ad5c7485a310ab1d54e35 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:51:54 +0000 Subject: [PATCH 09/27] test(ENGKNOW-3657): add TableServiceLoadMain profiler entrypoint Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/TableServiceLoadMain.java | 63 +++++++++++++++++++ .../memory/UTestTableServiceLoadMain.java | 20 ++++++ 2 files changed, 83 insertions(+) create mode 100644 test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java diff --git a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java new file mode 100644 index 00000000..e685d92d --- /dev/null +++ b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java @@ -0,0 +1,63 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import java.nio.file.Files; +import java.nio.file.Path; + +/** + * Entry point for profiler-attached runs of the table-service memory harness. + * Run with a prod-like heap and profiling flags, e.g.: + * java -Xmx2g -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp \ + * -XX:StartFlightRecording=filename=load.jfr,settings=profile \ + * -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ + * -cp org.gorpipe.test.memory.TableServiceLoadMain + */ +public class TableServiceLoadMain { + + public static void main(String[] args) throws Exception { + MemoryLoadConfig config = MemoryLoadConfig.fromSystemProperties(); + Path root = Files.createTempDirectory("memtest-run"); + System.out.println("Config: " + config); + System.out.println(runOnce(config, root)); + } + + public static String runOnce(MemoryLoadConfig config, Path root) throws Exception { + MemorySampler sampler = new MemorySampler(100); + sampler.start(); + try { + DictionaryFixture fixture = new DictionaryFixture(root); + var tables = fixture.createTables(config); + TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); + TableServiceLoadDriver.LoadResult result = driver.run(); + System.gc(); + Thread.sleep(200); // let sampler catch post-GC retained heap + sampler.stop(); + long peakHeapMB = sampler.peakHeapUsedBytes() / (1024 * 1024); + long peakRssMB = sampler.peakRssBytes() < 0 ? -1 : sampler.peakRssBytes() / (1024 * 1024); + return "peakHeapMB=" + peakHeapMB + " peakRssMB=" + peakRssMB + " " + result; + } finally { + sampler.stop(); + } + } +} diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java new file mode 100644 index 00000000..1d2fe438 --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java @@ -0,0 +1,20 @@ +package org.gorpipe.test.memory; + +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.Assert.assertTrue; + +public class UTestTableServiceLoadMain { + + @Test + public void runOnceProducesSummary() throws Exception { + Path root = Files.createTempDirectory("memtest-main"); + MemoryLoadConfig config = new MemoryLoadConfig(30, 10, 10, 2, 4, 70, 2); + String summary = TableServiceLoadMain.runOnce(config, root); + assertTrue("summary reports peak heap", summary.contains("peakHeapMB=")); + assertTrue("summary reports reads", summary.contains("reads=")); + } +} From 88ef645be2cc6471c454c6b4429677b24fe4dc16 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:55:46 +0000 Subject: [PATCH 10/27] fix(ENGKNOW-3657): add missing copyright header to load-main test Co-Authored-By: Claude Opus 4.8 (1M context) --- .../memory/UTestTableServiceLoadMain.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java index 1d2fe438..76b8137d 100644 --- a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java +++ b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java @@ -1,3 +1,25 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + package org.gorpipe.test.memory; import org.junit.Test; From a2d2ac14653888501bb23137de55aeb4d2f0a0e5 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 13:59:57 +0000 Subject: [PATCH 11/27] test(ENGKNOW-3657): validate harness stresses dictionary memory path Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/UTestTableServiceLoad.java | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java new file mode 100644 index 00000000..3ca9610d --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java @@ -0,0 +1,58 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.gorpipe.test.SlowTests; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.Assert.assertTrue; + +@Category(SlowTests.class) +public class UTestTableServiceLoad { + + private long peakHeapForFileCount(int fileCount) throws Exception { + Path root = Files.createTempDirectory("memtest-scale-" + fileCount); + MemoryLoadConfig config = new MemoryLoadConfig(fileCount, 20, 100, 4, 4, 70, 3); + MemorySampler sampler = new MemorySampler(50); + sampler.start(); + DictionaryFixture fixture = new DictionaryFixture(root); + var tables = fixture.createTables(config); + new TableServiceLoadDriver(tables, fixture, config).run(); + System.gc(); + Thread.sleep(200); + sampler.stop(); + return sampler.peakHeapUsedBytes(); + } + + @Test + public void peakHeapScalesWithDictionarySize() throws Exception { + long small = peakHeapForFileCount(500); + long large = peakHeapForFileCount(8000); + assertTrue("peak heap should grow with dictionary size: small=" + small + " large=" + large, + large > small * 1.5); + } +} From b8105de312d27b3a693660644d915177a62a265a Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 15:13:00 +0000 Subject: [PATCH 12/27] test(ENGKNOW-3657): model dictionary retention and measure retained heap Cache table instances so read-side DictionaryEntries stay retained for the run (models a session/table cache), and measure post-GC retained-heap delta over a baseline. Replaces the peak-churn metric that failed to scale. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../gorpipe/test/memory/MemorySampler.java | 9 +++++++ .../test/memory/TableServiceLoadDriver.java | 15 ++++++++++- .../test/memory/TableServiceLoadMain.java | 5 +++- .../test/memory/UTestTableServiceLoad.java | 25 ++++++++++--------- 4 files changed, 40 insertions(+), 14 deletions(-) diff --git a/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java b/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java index 06ce0ab2..4b0b8321 100644 --- a/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java +++ b/test/src/main/java/org/gorpipe/test/memory/MemorySampler.java @@ -104,4 +104,13 @@ public void stop() { public long currentHeapUsedBytes() { return ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); } + + /** Best-effort retained heap: forces GC twice then reads heap-used. Approximate but stable enough for scaling comparisons. */ + public static long measureRetainedHeapBytes() { + for (int i = 0; i < 2; i++) { + System.gc(); + try { Thread.sleep(100); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } + } + return ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); + } } diff --git a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java index e53354a7..61be843b 100644 --- a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java +++ b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java @@ -33,6 +33,7 @@ import java.io.PrintWriter; import java.nio.file.Path; import java.util.List; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; @@ -42,6 +43,11 @@ /** * Drives a mixed read+write load against a set of Gor dictionaries using a thread pool. * Reads: tag-filtered {@code filter().tags(..).get()}. Writes: insert a new data line + save. + * + *

Read-side table instances are cached (keyed by dictionary path) so their in-memory + * {@code DictionaryEntries} stay retained for the lifetime of the run, modeling a + * session/table cache in the real Table Service. Writes deliberately bypass the cache and + * always go through the faithful locked-transaction path on a fresh instance. */ public class TableServiceLoadDriver { private static final Logger log = LoggerFactory.getLogger(TableServiceLoadDriver.class); @@ -49,6 +55,7 @@ public class TableServiceLoadDriver { private final List tablePaths; private final DictionaryFixture fixture; private final MemoryLoadConfig config; + private final ConcurrentHashMap tableCache = new ConcurrentHashMap<>(); public TableServiceLoadDriver(List tablePaths, DictionaryFixture fixture, MemoryLoadConfig config) { this.tablePaths = tablePaths; @@ -62,6 +69,10 @@ public LoadResult run() throws InterruptedException { AtomicLong errors = new AtomicLong(); long deadline = System.nanoTime() + config.durationSeconds * 1_000_000_000L; + // Pre-open all tables into the cache so retention is deterministic (equals all + // tableCount tables regardless of which ones random ops happen to touch). + for (Path p : tablePaths) tableCache.computeIfAbsent(p, fixture::openTable); + ExecutorService pool = Executors.newFixedThreadPool(config.threads); for (int i = 0; i < config.threads; i++) { pool.submit(() -> { @@ -93,7 +104,9 @@ public LoadResult run() throws InterruptedException { } private void doRead(Path gord) { - GorDictionaryTable table = fixture.openTable(gord); + // Use the cached instance so its read-side DictionaryEntries stay retained. Reads on + // a shared instance are safe: the access optimizer's getOptimizedEntries is synchronized. + GorDictionaryTable table = tableCache.get(gord); String tag = "PN" + ThreadLocalRandom.current().nextInt(config.fileCount); table.filter().tags(tag).get(); } diff --git a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java index e685d92d..9dad064d 100644 --- a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java +++ b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java @@ -55,7 +55,10 @@ public static String runOnce(MemoryLoadConfig config, Path root) throws Exceptio sampler.stop(); long peakHeapMB = sampler.peakHeapUsedBytes() / (1024 * 1024); long peakRssMB = sampler.peakRssBytes() < 0 ? -1 : sampler.peakRssBytes() / (1024 * 1024); - return "peakHeapMB=" + peakHeapMB + " peakRssMB=" + peakRssMB + " " + result; + // driver still in scope here, tables retained in its cache, so this measures + // heap that survives GC while the dictionary tables are still held. + long retainedMB = MemorySampler.measureRetainedHeapBytes() / (1024 * 1024); + return "peakHeapMB=" + peakHeapMB + " peakRssMB=" + peakRssMB + " retainedHeapMB=" + retainedMB + " " + result; } finally { sampler.stop(); } diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java index 3ca9610d..9c34b830 100644 --- a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java +++ b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java @@ -34,25 +34,26 @@ @Category(SlowTests.class) public class UTestTableServiceLoad { - private long peakHeapForFileCount(int fileCount) throws Exception { + private long retainedDeltaForFileCount(int fileCount) throws Exception { Path root = Files.createTempDirectory("memtest-scale-" + fileCount); MemoryLoadConfig config = new MemoryLoadConfig(fileCount, 20, 100, 4, 4, 70, 3); - MemorySampler sampler = new MemorySampler(50); - sampler.start(); + long baseline = MemorySampler.measureRetainedHeapBytes(); DictionaryFixture fixture = new DictionaryFixture(root); var tables = fixture.createTables(config); - new TableServiceLoadDriver(tables, fixture, config).run(); - System.gc(); - Thread.sleep(200); - sampler.stop(); - return sampler.peakHeapUsedBytes(); + TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); + driver.run(); // pre-opens + retains all tables in driver.tableCache + long retained = MemorySampler.measureRetainedHeapBytes(); + long delta = retained - baseline; + // keep driver (and its retained tables) alive until AFTER measurement: + assertTrue("driver retained", driver != null); + return delta; } @Test - public void peakHeapScalesWithDictionarySize() throws Exception { - long small = peakHeapForFileCount(500); - long large = peakHeapForFileCount(8000); - assertTrue("peak heap should grow with dictionary size: small=" + small + " large=" + large, + public void retainedHeapScalesWithDictionarySize() throws Exception { + long small = retainedDeltaForFileCount(500); + long large = retainedDeltaForFileCount(8000); + assertTrue("retained heap should grow with dictionary size: small=" + small + " large=" + large, large > small * 1.5); } } From 4f34917a10c013fe634dd0f0b7cbe57dd6e5e70b Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 15:19:31 +0000 Subject: [PATCH 13/27] docs(ENGKNOW-3657): correct shared-read concurrency comments in load driver Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/TableServiceLoadDriver.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java index 61be843b..93de8233 100644 --- a/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java +++ b/test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java @@ -69,8 +69,10 @@ public LoadResult run() throws InterruptedException { AtomicLong errors = new AtomicLong(); long deadline = System.nanoTime() + config.durationSeconds * 1_000_000_000L; - // Pre-open all tables into the cache so retention is deterministic (equals all - // tableCount tables regardless of which ones random ops happen to touch). + // Pre-open all tables into the cache. NOTE: this builds the table shells only; + // each table's DictionaryEntries (the retained heap payload) loads lazily on its + // first read, so full retention assumes every table gets at least one read during + // the run (guaranteed at the validation config: tableCount=4, threads=4, 3s). for (Path p : tablePaths) tableCache.computeIfAbsent(p, fixture::openTable); ExecutorService pool = Executors.newFixedThreadPool(config.threads); @@ -104,8 +106,12 @@ public LoadResult run() throws InterruptedException { } private void doRead(Path gord) { - // Use the cached instance so its read-side DictionaryEntries stay retained. Reads on - // a shared instance are safe: the access optimizer's getOptimizedEntries is synchronized. + // Reads use the shared cached instance so its DictionaryEntries stay retained. + // NOTE: concurrent reads share DictionaryEntries.getEntries(), whose lazy-load + // guard (non-volatile 'dataLoaded' double-checked outside the synchronized + // loadLinesAndUpdateIndices()) is a pre-existing JMM data race in product code. + // loadLinesAndUpdateIndices() is idempotent+synchronized so the worst case here + // is a redundant reload, not corruption. See ENGKNOW-3657 findings. GorDictionaryTable table = tableCache.get(gord); String tag = "PN" + ThreadLocalRandom.current().nextInt(config.fileCount); table.filter().tags(tag).get(); From 71e127fbbe805bd28ce1a9f50acedf533e9e4b24 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 15:21:54 +0000 Subject: [PATCH 14/27] test(ENGKNOW-3657): add S3-backed dictionary fixture variant Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/memory/DictionaryFixture.java | 31 ++++++++++++ .../test/memory/UTestDictionaryFixtureS3.java | 49 +++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java diff --git a/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java b/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java index 8ef7daf1..1b65390a 100644 --- a/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java +++ b/test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java @@ -37,9 +37,15 @@ */ public class DictionaryFixture { private final Path root; + private final String s3Root; // null for filesystem-only fixtures public DictionaryFixture(Path root) { + this(root, null); + } + + public DictionaryFixture(Path root, String s3Root) { this.root = root; + this.s3Root = s3Root; } public List createTables(MemoryLoadConfig config) throws IOException { @@ -64,4 +70,29 @@ public List createTables(MemoryLoadConfig config) throws IOException { public GorDictionaryTable openTable(Path gordPath) { return new GorDictionaryTable.Builder<>(gordPath).build(); } + + /** + * Builds dictionaries whose .gord and bucket files live under an s3:// root, + * exercising the S3 driver write path (multipart buffers). Data files are + * generated locally then written to S3 via the gor table's FileReader. + */ + public List createTablesOnS3(MemoryLoadConfig config) throws IOException { + if (s3Root == null) throw new IllegalStateException("s3Root not configured"); + List tables = new ArrayList<>(); + int[] chrs = {1, 2, 3}; + for (int t = 0; t < config.tableCount; t++) { + String name = "memtest_s3_table_" + t; + String[] sources = new String[config.fileCount]; + for (int i = 0; i < config.fileCount; i++) sources[i] = "PN" + i; + Map> data = GorDictionarySetup.createDataFilesMap( + name, root, config.fileCount, chrs, config.rowsPerChr, "PN", true, sources); + + String gordUri = s3Root.endsWith("/") ? s3Root + name + ".gord" : s3Root + "/" + name + ".gord"; + GorDictionaryTable table = new GorDictionaryTable.Builder<>(gordUri).build(); + table.insert(data); + table.save(); + tables.add(Path.of(gordUri)); + } + return tables; + } } diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java b/test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java new file mode 100644 index 00000000..f0f51fb1 --- /dev/null +++ b/test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java @@ -0,0 +1,49 @@ +/* + * BEGIN_COPYRIGHT + * + * Copyright (C) 2011-2013 deCODE genetics Inc. + * Copyright (C) 2013-2019 WuXi NextCode Inc. + * All Rights Reserved. + * + * GORpipe is free software: you can redistribute it and/or modify + * it under the terms of the AFFERO GNU General Public License as published by + * the Free Software Foundation. + * + * GORpipe is distributed "AS-IS" AND WITHOUT ANY WARRANTY OF ANY KIND, + * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, + * NON-INFRINGEMENT, OR FITNESS FOR A PARTICULAR PURPOSE. See + * the AFFERO GNU General Public License for the complete license terms. + * + * You should have received a copy of the AFFERO GNU General Public License + * along with GORpipe. If not, see + * + * END_COPYRIGHT + */ + +package org.gorpipe.test.memory; + +import org.junit.Assume; +import org.junit.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.Assert.assertTrue; + +public class UTestDictionaryFixtureS3 { + + @Test + public void createsTablesOnS3WhenBucketConfigured() throws Exception { + String s3Root = System.getenv("GOR_MEMTEST_S3_ROOT"); // e.g. s3://my-dev-bucket/memtest + Assume.assumeTrue("GOR_MEMTEST_S3_ROOT not set; skipping S3 fixture test", s3Root != null); + + Path localRoot = Files.createTempDirectory("memtest-s3-local"); + MemoryLoadConfig config = new MemoryLoadConfig(20, 10, 10, 1, 2, 50, 2); + DictionaryFixture fixture = new DictionaryFixture(localRoot, s3Root); + + List tables = fixture.createTablesOnS3(config); + + assertTrue("created at least one S3 table", tables.size() >= 1); + } +} From 0473e0bb4de300d050a195cc72346dc3a0a84e19 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 15:24:55 +0000 Subject: [PATCH 15/27] docs(ENGKNOW-3657): add profiling runbook and findings template Co-Authored-By: Claude Opus 4.8 (1M context) --- ...2026-07-08-engknow-3657-memory-findings.md | 31 +++++++++++++ ...26-07-08-engknow-3657-profiling-runbook.md | 43 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md create mode 100644 docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md diff --git a/docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md b/docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md new file mode 100644 index 00000000..488afa58 --- /dev/null +++ b/docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md @@ -0,0 +1,31 @@ +# ENGKNOW-3657 Memory Findings + +> Fill in after running the profiling runbook. This is the deliverable of the diagnosis task. + +## Observed during harness build (pre-profiling) + +1. **Harness validated — retained heap scales with dictionary size.** The load harness (`org.gorpipe.test.memory.*`, run via `./gradlew :test:slowTest --tests "org.gorpipe.test.memory.UTestTableServiceLoad"`) models a session/table cache by holding opened `GorDictionaryTable` instances for the run and measuring post-GC retained heap. Retained-heap delta grows with the `fileCount` (dictionary size) knob (validated, 2/2 stable runs). This confirms the harness exercises the suspected retention path (spec hypothesis #1) and is ready for profiling. Note: an earlier peak-heap-during-run metric did NOT scale — peak captured transient allocation churn, not retention; retained-heap-after-GC is the correct signal. + +2. **Candidate root cause surfaced (needs profiling confirmation): `DictionaryEntries` lazy-load data race.** `model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java` (~lines 127-133) guards its lazy load with a non-volatile `dataLoaded` boolean checked OUTSIDE the `synchronized loadLinesAndUpdateIndices()` — a double-checked-locking JMM race, despite the class javadoc claiming thread safety (line 40). Harmless when each request gets its own table instance (the field is only written once, idempotently), but REACHABLE if the real Table Service caches/shares `GorDictionaryTable` instances across concurrent reads. Worst case is a redundant reload (loadLinesAndUpdateIndices is idempotent + synchronized), not corruption — so this is a correctness/thread-safety smell to verify under profiling, and it directly relates to hypothesis #1 (whether dictionaries are retained/shared via a cache). Flag for a follow-up ticket if the profiler confirms table instances are cached/shared in production. + +## Reproduction +- Prod heap ceiling used (`-Xmx`): __ +- Knobs that triggered OOM: fileCount=__ tableCount=__ threads=__ duration=__ +- Summary line from harness: __ + +## Dominant consumer +- Class / structure: __ +- Retained size: __ MB (__ % of heap) +- Path to GC roots: __ +- Retained past request end? (session cache / static): __ + +## Hypothesis verdicts +- [ ] DictionaryEntries retention (rawLines + multimaps): CONFIRMED / REFUTED — evidence: __ +- [ ] 64 MB multipart write buffers: CONFIRMED / REFUTED — evidence: __ +- [ ] Caches (tagsToListCache / client / metadata): CONFIRMED / REFUTED — evidence: __ +- [ ] Off-heap / CRT S3 (RSS >> heap): CONFIRMED / REFUTED — evidence: __ +- [ ] Unlisted consumer: __ + +## Recommendation +- Proposed fix direction: __ +- Follow-up ticket: __ diff --git a/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md b/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md new file mode 100644 index 00000000..29bc8a00 --- /dev/null +++ b/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md @@ -0,0 +1,43 @@ +# ENGKNOW-3657 Profiling Runbook + +## 1. Get the prod heap ceiling +Fetch the table-service pod memory limit from its helm values (or Grafana +`kube_pod_container_resource_limits{resource="memory"}`). Use it as `-Xmx` below +so the local run reproduces the OOM. + +## 2. Build the classpath +```bash +./gradlew :test:testClasses +CP=$(./gradlew -q :test:printTestClasspath 2>/dev/null || echo "see note") +# If :printTestClasspath is not defined, run via gradle JavaExec or use the +# test runtime classpath from `./gradlew :test:dependencies`. +``` + +## 3. Run under JFR + heap-dump-on-OOM at the prod ceiling +```bash +java -Xmx \ + -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp/engknow3657.hprof \ + -XX:StartFlightRecording=filename=/tmp/engknow3657.jfr,settings=profile,dumponexit=true \ + -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ + -Dgor.memtest.threads=8 -Dgor.memtest.readWritePercent=70 \ + -Dgor.memtest.durationSeconds=300 \ + -cp "$CP" org.gorpipe.test.memory.TableServiceLoadMain +``` +Ramp `fileCount` / `tableCount` until it OOMs at the prod ceiling. + +## 4. Analyze +- Heap dump: open `engknow3657.hprof` in Eclipse MAT or `jhat`. Sort by + **retained size**. Run "Dominator Tree" and "Path to GC Roots" on the top + objects. Check the hypotheses: `DictionaryEntries.rawLines` / + `tagHashToLines` / `contentHashToLines`; `byte[]` part buffers (~64 MB); + cache maps. +- JFR: open `engknow3657.jfr` in JDK Mission Control → Memory → Allocation. + Identify top allocation sites. +- Off-heap: compare `peakRssMB` vs `peakHeapMB` from the harness summary. A + large gap points at native/CRT S3 buffers — rerun with `-XX:NativeMemoryTracking=summary` + and `jcmd VM.native_memory summary`. + +## 5. Fallback: prod heap dump +If it will not reproduce locally, add `-XX:+HeapDumpOnOutOfMemoryError` +`-XX:HeapDumpPath=/data` to the table-service pod JVM args and pull the dump +after the next OOM. From 2f880c522fef7f7f7fb2bd53789db989a080e891 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 16:10:26 +0000 Subject: [PATCH 16/27] feat(ENGKNOW-3657): make dictionary TableCache tunable and record stats Co-Authored-By: Claude Opus 4.8 (1M context) --- model/build.gradle | 4 +- .../org/gorpipe/gor/table/TableCache.java | 17 +++++- .../gorpipe/gor/table/UTestTableCache.java | 57 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 model/src/test/java/org/gorpipe/gor/table/UTestTableCache.java diff --git a/model/build.gradle b/model/build.gradle index 3fc19fee..51cd8d5d 100644 --- a/model/build.gradle +++ b/model/build.gradle @@ -37,7 +37,9 @@ project(':model') { annotationProcessor 'com.google.auto.service:auto-service:_' implementation 'com.google.auto.service:auto-service:_' - implementation 'com.github.ben-manes.caffeine:caffeine:_' + // TableCache exposes CacheStats (com.github.benmanes.caffeine.cache.stats) on its public API, + // so callers need caffeine at compile time too. + api 'com.github.ben-manes.caffeine:caffeine:_' implementation "org.apache.commons:commons-lang3:_" implementation "org.aeonbits.owner:owner:_" implementation "com.github.samtools:htsjdk:_" diff --git a/model/src/main/java/org/gorpipe/gor/table/TableCache.java b/model/src/main/java/org/gorpipe/gor/table/TableCache.java index 37f52ac7..e931f241 100644 --- a/model/src/main/java/org/gorpipe/gor/table/TableCache.java +++ b/model/src/main/java/org/gorpipe/gor/table/TableCache.java @@ -2,6 +2,7 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.stats.CacheStats; import org.gorpipe.gor.model.FileReader; import org.gorpipe.util.Strings; import org.slf4j.Logger; @@ -18,8 +19,10 @@ public abstract class TableCache { public static final boolean useCache = Boolean.parseBoolean(System.getProperty("gor.dictionary.cache.active", "true")); final protected Cache dictCache = Caffeine.newBuilder() - .maximumSize(500).expireAfterAccess(Duration.ofHours(12L)) + .maximumSize(Long.getLong("gor.dictionary.cache.maxsize", 500L)) + .expireAfterAccess(Duration.ofHours(Long.getLong("gor.dictionary.cache.ttl.hours", 12L))) .softValues() + .recordStats() .build(); //A map from dictionaries to the cache objects. abstract protected T createTable(String path, FileReader fileReader); @@ -60,6 +63,18 @@ public synchronized T getTable(String path, FileReader fileReader, boolean useCa } } + public CacheStats stats() { + return dictCache.stats(); + } + + public long estimatedSize() { + return dictCache.estimatedSize(); + } + + public void cleanUp() { + dictCache.cleanUp(); + } + public synchronized void updateCache(T table) { String uniqueID = table.getId(); var key = dictCacheKeyFromPathAndRoot(table.getPath(), table.getFileReader()); diff --git a/model/src/test/java/org/gorpipe/gor/table/UTestTableCache.java b/model/src/test/java/org/gorpipe/gor/table/UTestTableCache.java new file mode 100644 index 00000000..805fed4e --- /dev/null +++ b/model/src/test/java/org/gorpipe/gor/table/UTestTableCache.java @@ -0,0 +1,57 @@ +package org.gorpipe.gor.table; + +import org.gorpipe.gor.model.DriverBackedFileReader; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryCache; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.nio.file.Files; +import java.nio.file.Path; + +public class UTestTableCache { + + @Rule + public TemporaryFolder workDir = new TemporaryFolder(); + + private Path writeSmallDict(String name) throws Exception { + Path dir = workDir.getRoot().toPath(); + Files.writeString(dir.resolve("a.gor"), "chrom\tpos\n" + "chr1\t1\n"); + Path gord = dir.resolve(name); + // one data line: filealias + Files.writeString(gord, "a.gor\tpn1\n"); + return gord; + } + + @Test + public void getTable_secondCallIsCacheHit_andStatsRecorded() throws Exception { + Path gord = writeSmallDict("stats.gord"); + GorDictionaryCache cache = new GorDictionaryCache(); + DriverBackedFileReader reader = new DriverBackedFileReader(""); + + long hitsBefore = cache.stats().hitCount(); + cache.getTable(gord.toString(), reader); // miss -> put + cache.getTable(gord.toString(), reader); // hit + Assert.assertEquals(1L, cache.estimatedSize()); + Assert.assertTrue("expected a cache hit to be recorded", + cache.stats().hitCount() > hitsBefore); + } + + @Test + public void maxSizeSystemProperty_isHonored() throws Exception { + String prev = System.getProperty("gor.dictionary.cache.maxsize"); + System.setProperty("gor.dictionary.cache.maxsize", "1"); + try { + GorDictionaryCache cache = new GorDictionaryCache(); + DriverBackedFileReader reader = new DriverBackedFileReader(""); + cache.getTable(writeSmallDict("m1.gord").toString(), reader); + cache.getTable(writeSmallDict("m2.gord").toString(), reader); + cache.cleanUp(); + Assert.assertTrue("cache bounded to maxsize=1", cache.estimatedSize() <= 1L); + } finally { + if (prev == null) System.clearProperty("gor.dictionary.cache.maxsize"); + else System.setProperty("gor.dictionary.cache.maxsize", prev); + } + } +} From 8378b4a94bb3c9909d0f4fb0ea44a95364ad34e2 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 17:37:54 +0000 Subject: [PATCH 17/27] fix(ENGKNOW-3657): build dictionary content map lazily contentHashToLines, activeTags and deletedEntriesCount are only needed by the mutation/stats paths, not by select/filter reads. Building them lazily shrinks the long-lived cached dictionary copy for select-heavy workloads. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../table/dictionary/DictionaryEntries.java | 23 +++--- .../UTestDictionaryEntriesLazy.java | 75 +++++++++++++++++++ 2 files changed, 89 insertions(+), 9 deletions(-) create mode 100644 model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesLazy.java diff --git a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java index cd9e84f5..80cf48d8 100644 --- a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java +++ b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java @@ -56,6 +56,7 @@ public class DictionaryEntries implements IDictionary private boolean dataLoaded = false; private boolean tagHashLoaded = false; + private boolean contentMapLoaded = false; /** * Construct new dict file from the given path and chromosome cache. @@ -152,9 +153,7 @@ public Iterator iterator() { @Override public Iterator getActiveEntries() { - if (!dataLoaded) { - loadLinesAndUpdateIndices(); - } + updateContentMap(); return new Iterator() { int nextIndex = 0; @@ -186,14 +185,13 @@ public boolean isLoaded() { @Override public Set getAllActiveTags() { - if (!dataLoaded) { - loadLinesAndUpdateIndices(); - } + updateContentMap(); return activeTags.elementSet(); } @Override public boolean hasDeletedEntries() { + updateContentMap(); return deletedEntriesCount > 0; } @@ -204,18 +202,24 @@ public int size() { @Override public int getActiveLinesCount() { + updateContentMap(); int size = getEntries().size(); return size - deletedEntriesCount; } - private void updateContentMap() { + synchronized private void updateContentMap() { + if (contentMapLoaded) return; + if (!dataLoaded) { + loadLinesAndUpdateIndices(); + } contentHashToLines = ArrayListMultimap.create(rawLines.size(), 1); activeTags = HashMultiset.create(); - + deletedEntriesCount = 0; for (T entry : rawLines) { addEntryToContentMap(entry); } + contentMapLoaded = true; } synchronized private void updateTagMap() { @@ -274,6 +278,7 @@ private void clearContentMap() { contentHashToLines = null; activeTags = null; deletedEntriesCount = 0; + contentMapLoaded = false; } private void clearTagMap() { @@ -285,7 +290,6 @@ synchronized private void loadLinesAndUpdateIndices() { if (dataLoaded) return; log.trace("Loading entries into table {} {}", table.getName(), table); this.rawLines = this.loadLines(); - this.updateContentMap(); if (table.getColumns().length == 0 && !this.rawLines.isEmpty()) { try { // Update header from the first file. @@ -338,6 +342,7 @@ private List loadLines() { public T findLine(T line) { // Using contentHashMap List allLines = getEntries(); + updateContentMap(); List lines2Search = contentHashToLines != null ? contentHashToLines.get(line.getSearchHash()) : allLines; return lines2Search != null ? lines2Search.stream().filter(l -> l.equals(line)).findFirst().orElse(null) : null; diff --git a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesLazy.java b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesLazy.java new file mode 100644 index 00000000..f841e478 --- /dev/null +++ b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesLazy.java @@ -0,0 +1,75 @@ +package org.gorpipe.gor.table.dictionary; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.lang.reflect.Field; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; + +public class UTestDictionaryEntriesLazy { + + @Rule + public TemporaryFolder workDir = new TemporaryFolder(); + + private GorDictionaryTable buildTable() throws Exception { + Path dir = workDir.getRoot().toPath(); + Files.writeString(dir.resolve("a.gor"), "chrom\tpos\nchr1\t1\n"); + Files.writeString(dir.resolve("b.gor"), "chrom\tpos\nchr1\t2\n"); + Path gord = dir.resolve("t.gord"); + Files.writeString(gord, "a.gor\tpn1\nb.gor\tpn2\n"); + return new GorDictionaryTable.Builder<>(gord.toString()).build(); + } + + // Reflect into the DictionaryEntries backing the table. + private Object entriesOf(GorDictionaryTable table) throws Exception { + Field f = DictionaryTableReader.class.getDeclaredField("tableEntries"); + f.setAccessible(true); + return f.get(table); + } + + private boolean boolField(Object entries, String name) throws Exception { + Field f = DictionaryEntries.class.getDeclaredField(name); + f.setAccessible(true); + return f.getBoolean(entries); + } + + private Object refField(Object entries, String name) throws Exception { + Field f = DictionaryEntries.class.getDeclaredField(name); + f.setAccessible(true); + return f.get(entries); + } + + @Test + public void readPath_doesNotBuildContentMap() throws Exception { + GorDictionaryTable table = buildTable(); + // Read path: filter by tag. + List res = table.filter().tags("pn1").get(); + Assert.assertEquals(1, res.size()); + + Object entries = entriesOf(table); + Assert.assertFalse("content map must not be built by the read path", + boolField(entries, "contentMapLoaded")); + Assert.assertNull("contentHashToLines must be null after read-only path", + refField(entries, "contentHashToLines")); + } + + @Test + public void getAllActiveTags_buildsContentMap_andReturnsTags() throws Exception { + GorDictionaryTable table = buildTable(); + Object entries = entriesOf(table); + + @SuppressWarnings("unchecked") + Set tags = ((IDictionaryEntries) entries).getAllActiveTags(); + Assert.assertTrue(tags.contains("pn1")); + Assert.assertTrue(tags.contains("pn2")); + Assert.assertTrue("content map should be built once a consumer asks", + boolField(entries, "contentMapLoaded")); + } +} From 5cc72500e4b6648dc13e77755a6c0bc3f6536094 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 20:21:42 +0000 Subject: [PATCH 18/27] fix(ENGKNOW-3657): Remove redundant assert. --- .../java/org/gorpipe/test/memory/UTestTableServiceLoad.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java index 9c34b830..06a01c25 100644 --- a/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java +++ b/test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java @@ -44,8 +44,6 @@ private long retainedDeltaForFileCount(int fileCount) throws Exception { driver.run(); // pre-opens + retains all tables in driver.tableCache long retained = MemorySampler.measureRetainedHeapBytes(); long delta = retained - baseline; - // keep driver (and its retained tables) alive until AFTER measurement: - assertTrue("driver retained", driver != null); return delta; } From d3590135b34e6b0b96c054e8708d2873f316542b Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 21:07:25 +0000 Subject: [PATCH 19/27] fix(ENGKNOW-3657): Tune max cache size. --- model/src/main/java/org/gorpipe/gor/table/TableCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/src/main/java/org/gorpipe/gor/table/TableCache.java b/model/src/main/java/org/gorpipe/gor/table/TableCache.java index e931f241..f61b4d4e 100644 --- a/model/src/main/java/org/gorpipe/gor/table/TableCache.java +++ b/model/src/main/java/org/gorpipe/gor/table/TableCache.java @@ -19,7 +19,7 @@ public abstract class TableCache { public static final boolean useCache = Boolean.parseBoolean(System.getProperty("gor.dictionary.cache.active", "true")); final protected Cache dictCache = Caffeine.newBuilder() - .maximumSize(Long.getLong("gor.dictionary.cache.maxsize", 500L)) + .maximumSize(Long.getLong("gor.dictionary.cache.maxsize", 64L)) .expireAfterAccess(Duration.ofHours(Long.getLong("gor.dictionary.cache.ttl.hours", 12L))) .softValues() .recordStats() From e84c827f944a6289653952371a6167206ccf7933 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 21:14:00 +0000 Subject: [PATCH 20/27] chore(ENGKNOW-3657): remove planning docs from VCS Keep the memory-diagnosis design spec and implementation plan as local-only files; not part of the PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...now-3657-table-service-memory-diagnosis.md | 976 ------------------ ...7-table-service-memory-diagnosis-design.md | 93 -- 2 files changed, 1069 deletions(-) delete mode 100644 docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md delete mode 100644 docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md diff --git a/docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md b/docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md deleted file mode 100644 index aba1f2d9..00000000 --- a/docs/superpowers/plans/2026-07-08-engknow-3657-table-service-memory-diagnosis.md +++ /dev/null @@ -1,976 +0,0 @@ -# Table-service Memory Diagnosis Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Build a reusable local load harness that reproduces the table-service OOM under mixed read+write dictionary load, profile it, and identify the dominant memory consumer with evidence. - -**Architecture:** A Java harness in the `test` module (which depends on all main modules and holds shared test infra like `GorDictionarySetup`). A fixture builder generates dictionaries at prod-like scale; a thread-pool load driver issues a mixed read+write operation stream against `GorDictionaryTable`; a memory sampler records peak heap and RSS. A `main` entrypoint runs it under JFR / `-XX:+HeapDumpOnOutOfMemoryError` at a prod-like `-Xmx`; a `@Category(SlowTests)` JUnit validates the harness stresses the target path. A runbook + findings doc capture the diagnosis. - -**Tech Stack:** Java 17, JUnit 4 (`@Category(org.gorpipe.test.SlowTests)`), Gradle (`slowTest` task), JFR, `jcmd`, `java.lang.management` MXBeans. Dictionary API: `org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable`. Fixture helper: `org.gorpipe.test.GorDictionarySetup`. - -## Global Constraints - -- Package for all new harness code: `org.gorpipe.test.memory`. -- Harness production code lives in `test/src/main/java/...`; harness tests in `test/src/test/java/...`. -- Copyright header: copy the `BEGIN_COPYRIGHT ... END_COPYRIGHT` block verbatim from any existing file in the `test` module (e.g. `test/src/main/java/org/gorpipe/test/GorDictionarySetup.java`) at the top of every new `.java` file. -- Scope is **diagnosis only** — no fix to `DictionaryEntries`, S3 buffers, or caches in this plan. The fix is a separate ticket. -- Slow/heavy tests use `@org.junit.experimental.categories.Category(org.gorpipe.test.SlowTests.class)` so they run under `./gradlew :test:slowTest`, not the default `test` task. -- Commit trailer on every commit: - ``` - Co-Authored-By: Claude Opus 4.8 (1M context) - ``` - -## File Structure - -- Create `test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java` — immutable config of all load knobs (dict size, buckets, tags, tables, concurrency, read:write ratio, duration), built from defaults + system properties. -- Create `test/src/main/java/org/gorpipe/test/memory/MemorySampler.java` — background sampler recording peak heap-used and peak RSS. -- Create `test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java` — builds dictionaries at scale on a filesystem or S3 root using `GorDictionarySetup`. -- Create `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java` — thread pool issuing mixed read+write ops against `GorDictionaryTable`; returns an op-count summary. -- Create `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java` — `main` entrypoint: config → fixture → driver → summary; for profiler-attached runs. -- Create `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java` — `@Category(SlowTests)` test validating the harness stresses the dictionary path (peak heap scales with dict-size knob). -- Create `docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md` — exact commands to run the harness under JFR + heap-dump at prod `-Xmx`, and how to analyze the dump. -- Create `docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md` — findings template to fill after profiling. - ---- - -### Task 1: MemoryLoadConfig — load knobs - -**Files:** -- Create: `test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java` -- Test: `test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java` - -**Interfaces:** -- Consumes: nothing. -- Produces: `MemoryLoadConfig` with public final int/long fields `fileCount`, `rowsPerChr`, `bucketSize`, `tableCount`, `threads`, `readWritePercent` (0-100, percent of ops that are reads), `durationSeconds`; static factory `MemoryLoadConfig.fromSystemProperties()`; constructor `MemoryLoadConfig(int fileCount, int rowsPerChr, int bucketSize, int tableCount, int threads, int readWritePercent, int durationSeconds)`. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.junit.Test; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -public class UTestMemoryLoadConfig { - - @Test - public void defaultsAreSane() { - MemoryLoadConfig c = new MemoryLoadConfig(1000, 100, 100, 4, 8, 70, 30); - assertEquals(1000, c.fileCount); - assertEquals(70, c.readWritePercent); - assertTrue(c.threads > 0); - } - - @Test - public void systemPropertiesOverrideDefaults() { - System.setProperty("gor.memtest.fileCount", "5000"); - try { - MemoryLoadConfig c = MemoryLoadConfig.fromSystemProperties(); - assertEquals(5000, c.fileCount); - } finally { - System.clearProperty("gor.memtest.fileCount"); - } - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemoryLoadConfig"` -Expected: FAIL — `MemoryLoadConfig` does not exist (compilation error). - -- [ ] **Step 3: Write minimal implementation** - -```java -package org.gorpipe.test.memory; - -/** Immutable set of load knobs for the table-service memory harness. */ -public final class MemoryLoadConfig { - public final int fileCount; // data files per dictionary (~ dictionary lines) - public final int rowsPerChr; // rows per chromosome per data file - public final int bucketSize; // files per bucket - public final int tableCount; // dictionaries held concurrently - public final int threads; // load driver concurrency - public final int readWritePercent; // percent of ops that are reads (0-100) - public final int durationSeconds; - - public MemoryLoadConfig(int fileCount, int rowsPerChr, int bucketSize, int tableCount, - int threads, int readWritePercent, int durationSeconds) { - this.fileCount = fileCount; - this.rowsPerChr = rowsPerChr; - this.bucketSize = bucketSize; - this.tableCount = tableCount; - this.threads = threads; - this.readWritePercent = readWritePercent; - this.durationSeconds = durationSeconds; - } - - public static MemoryLoadConfig fromSystemProperties() { - return new MemoryLoadConfig( - intProp("gor.memtest.fileCount", 2000), - intProp("gor.memtest.rowsPerChr", 100), - intProp("gor.memtest.bucketSize", 100), - intProp("gor.memtest.tableCount", 8), - intProp("gor.memtest.threads", Runtime.getRuntime().availableProcessors()), - intProp("gor.memtest.readWritePercent", 70), - intProp("gor.memtest.durationSeconds", 60)); - } - - private static int intProp(String key, int def) { - return Integer.parseInt(System.getProperty(key, String.valueOf(def))); - } - - @Override - public String toString() { - return "MemoryLoadConfig{fileCount=" + fileCount + ", rowsPerChr=" + rowsPerChr - + ", bucketSize=" + bucketSize + ", tableCount=" + tableCount - + ", threads=" + threads + ", readWritePercent=" + readWritePercent - + ", durationSeconds=" + durationSeconds + "}"; - } -} -``` - -(Prepend the copyright header block above the `package` line.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemoryLoadConfig"` -Expected: PASS (2 tests). - -- [ ] **Step 5: Commit** - -```bash -git add test/src/main/java/org/gorpipe/test/memory/MemoryLoadConfig.java \ - test/src/test/java/org/gorpipe/test/memory/UTestMemoryLoadConfig.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): add MemoryLoadConfig knobs for memory harness - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 2: MemorySampler — peak heap + RSS - -**Files:** -- Create: `test/src/main/java/org/gorpipe/test/memory/MemorySampler.java` -- Test: `test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java` - -**Interfaces:** -- Consumes: nothing. -- Produces: `MemorySampler` with `void start()`, `void stop()`, `long peakHeapUsedBytes()`, `long peakRssBytes()` (returns `-1` if RSS unavailable on this OS), `long currentHeapUsedBytes()`. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.junit.Test; -import static org.junit.Assert.assertTrue; - -public class UTestMemorySampler { - - @Test - public void recordsPeakHeapAfterAllocation() throws Exception { - MemorySampler sampler = new MemorySampler(50); - sampler.start(); - byte[][] hold = new byte[64][]; - for (int i = 0; i < hold.length; i++) { - hold[i] = new byte[1024 * 1024]; // 64 MB total - Thread.sleep(5); - } - sampler.stop(); - assertTrue("peak heap should exceed 32MB", sampler.peakHeapUsedBytes() > 32L * 1024 * 1024); - assertTrue("hold retained", hold[0][0] == 0); // keep alive - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemorySampler"` -Expected: FAIL — `MemorySampler` does not exist. - -- [ ] **Step 3: Write minimal implementation** - -```java -package org.gorpipe.test.memory; - -import java.lang.management.ManagementFactory; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.List; - -/** Background thread sampling heap-used (via MXBean) and RSS (via /proc on Linux). */ -public class MemorySampler { - private final long intervalMillis; - private volatile boolean running; - private volatile long peakHeap; - private volatile long peakRss = -1; - private Thread thread; - - public MemorySampler(long intervalMillis) { - this.intervalMillis = intervalMillis; - } - - public void start() { - running = true; - thread = new Thread(this::loop, "memory-sampler"); - thread.setDaemon(true); - thread.start(); - } - - private void loop() { - while (running) { - sampleOnce(); - try { Thread.sleep(intervalMillis); } catch (InterruptedException e) { return; } - } - } - - private void sampleOnce() { - long heap = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); - if (heap > peakHeap) peakHeap = heap; - long rss = readRssBytes(); - if (rss > peakRss) peakRss = rss; - } - - /** Reads VmRSS from /proc/self/status. Returns -1 if unavailable (e.g. macOS). */ - private long readRssBytes() { - Path status = Path.of("/proc/self/status"); - if (!Files.exists(status)) return -1; - try { - List lines = Files.readAllLines(status); - for (String line : lines) { - if (line.startsWith("VmRSS:")) { - String[] parts = line.trim().split("\\s+"); // "VmRSS: 12345 kB" - return Long.parseLong(parts[1]) * 1024; - } - } - } catch (Exception e) { - return -1; - } - return -1; - } - - public void stop() { - running = false; - if (thread != null) thread.interrupt(); - sampleOnce(); - } - - public long peakHeapUsedBytes() { return peakHeap; } - public long peakRssBytes() { return peakRss; } - public long currentHeapUsedBytes() { - return ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getUsed(); - } -} -``` - -(Prepend copyright header.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestMemorySampler"` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add test/src/main/java/org/gorpipe/test/memory/MemorySampler.java \ - test/src/test/java/org/gorpipe/test/memory/UTestMemorySampler.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): add MemorySampler for peak heap/RSS tracking - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 3: DictionaryFixture — build dictionaries at scale (filesystem) - -**Files:** -- Create: `test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java` -- Test: `test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java` -- Reference (do not modify): `test/src/main/java/org/gorpipe/test/GorDictionarySetup.java`, `model/src/main/java/org/gorpipe/gor/table/dictionary/gor/GorDictionaryTable.java` - -**Interfaces:** -- Consumes: `MemoryLoadConfig` (Task 1). -- Produces: `DictionaryFixture` with constructor `DictionaryFixture(Path root)`; method `List createTables(MemoryLoadConfig config)` returning the `.gord` paths (one per table); method `GorDictionaryTable openTable(Path gordPath)` returning a built `GorDictionaryTable`. The `.gord` paths and `openTable` are consumed by Task 4. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; -import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; -import org.junit.Test; - -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.List; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -public class UTestDictionaryFixture { - - @Test - public void createsReadableTablesAtScale() throws Exception { - Path root = Files.createTempDirectory("memtest-fixture"); - DictionaryFixture fixture = new DictionaryFixture(root); - MemoryLoadConfig config = new MemoryLoadConfig(50, 10, 10, 3, 4, 70, 5); - - List tables = fixture.createTables(config); - - assertEquals(3, tables.size()); - for (Path gord : tables) { - assertTrue("gord file exists", Files.exists(gord)); - GorDictionaryTable table = fixture.openTable(gord); - List all = table.filter().get(); - assertTrue("dictionary has entries", all.size() > 0); - } - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixture"` -Expected: FAIL — `DictionaryFixture` does not exist. - -- [ ] **Step 3: Write minimal implementation** - -```java -package org.gorpipe.test.memory; - -import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; -import org.gorpipe.test.GorDictionarySetup; - -import java.io.IOException; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; - -/** - * Builds Gor dictionaries at prod-like scale for the memory harness. - * Wraps {@link GorDictionarySetup} to generate data files + a .gord dictionary per table. - */ -public class DictionaryFixture { - private final Path root; - - public DictionaryFixture(Path root) { - this.root = root; - } - - public List createTables(MemoryLoadConfig config) throws IOException { - List tables = new ArrayList<>(); - int[] chrs = {1, 2, 3}; - for (int t = 0; t < config.tableCount; t++) { - String name = "memtest_table_" + t; - String[] sources = new String[config.fileCount]; - for (int i = 0; i < config.fileCount; i++) sources[i] = "PN" + i; - Map> data = GorDictionarySetup.createDataFilesMap( - name, root, config.fileCount, chrs, config.rowsPerChr, "PN", true, sources); - - Path gord = root.resolve(name + ".gord"); - GorDictionaryTable table = new GorDictionaryTable.Builder<>(gord).build(); - table.insert(data); - table.save(); - tables.add(gord); - } - return tables; - } - - public GorDictionaryTable openTable(Path gordPath) { - return new GorDictionaryTable.Builder<>(gordPath).build(); - } -} -``` - -(Prepend copyright header. If `insert(Map)` or `save()` signatures differ at compile time, check `DictionaryTable.java:129,304` and adjust the calls — the public methods `insert(Map>)` and `save()` exist there.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixture"` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java \ - test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixture.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): add DictionaryFixture to build dictionaries at scale - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 4: TableServiceLoadDriver — mixed read+write load - -**Files:** -- Create: `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java` -- Test: `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java` - -**Interfaces:** -- Consumes: `MemoryLoadConfig` (Task 1), `DictionaryFixture` + `GorDictionaryTable` (Task 3). -- Produces: `TableServiceLoadDriver` with constructor `TableServiceLoadDriver(List tablePaths, DictionaryFixture fixture, MemoryLoadConfig config)`; method `LoadResult run()` where `LoadResult` is a public static nested class with public final fields `long readOps`, `long writeOps`, `long errors`. Consumed by Task 5. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.junit.Test; - -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.List; - -import static org.junit.Assert.assertTrue; - -public class UTestTableServiceLoadDriver { - - @Test - public void runsMixedLoadAndCountsOps() throws Exception { - Path root = Files.createTempDirectory("memtest-driver"); - MemoryLoadConfig config = new MemoryLoadConfig(30, 10, 10, 2, 4, 70, 2); - DictionaryFixture fixture = new DictionaryFixture(root); - List tables = fixture.createTables(config); - - TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); - TableServiceLoadDriver.LoadResult result = driver.run(); - - assertTrue("did some reads", result.readOps > 0); - assertTrue("did some writes", result.writeOps > 0); - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadDriver"` -Expected: FAIL — `TableServiceLoadDriver` does not exist. - -- [ ] **Step 3: Write minimal implementation** - -```java -package org.gorpipe.test.memory; - -import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; - -import java.nio.file.Path; -import java.util.List; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.ThreadLocalRandom; - -/** - * Drives a mixed read+write load against a set of Gor dictionaries using a thread pool. - * Reads: tag-filtered {@code filter().tags(..).get()}. Writes: insert a new data line + save. - */ -public class TableServiceLoadDriver { - private final List tablePaths; - private final DictionaryFixture fixture; - private final MemoryLoadConfig config; - - public TableServiceLoadDriver(List tablePaths, DictionaryFixture fixture, MemoryLoadConfig config) { - this.tablePaths = tablePaths; - this.fixture = fixture; - this.config = config; - } - - public LoadResult run() throws InterruptedException { - AtomicLong readOps = new AtomicLong(); - AtomicLong writeOps = new AtomicLong(); - AtomicLong errors = new AtomicLong(); - long deadline = System.nanoTime() + config.durationSeconds * 1_000_000_000L; - - ExecutorService pool = Executors.newFixedThreadPool(config.threads); - for (int i = 0; i < config.threads; i++) { - pool.submit(() -> { - while (System.nanoTime() < deadline) { - Path gord = tablePaths.get(ThreadLocalRandom.current().nextInt(tablePaths.size())); - try { - if (ThreadLocalRandom.current().nextInt(100) < config.readWritePercent) { - doRead(gord); - readOps.incrementAndGet(); - } else { - doWrite(gord); - writeOps.incrementAndGet(); - } - } catch (Exception e) { - errors.incrementAndGet(); - } - } - }); - } - pool.shutdown(); - pool.awaitTermination(config.durationSeconds + 30, TimeUnit.SECONDS); - return new LoadResult(readOps.get(), writeOps.get(), errors.get()); - } - - private void doRead(Path gord) { - GorDictionaryTable table = fixture.openTable(gord); - String tag = "PN" + ThreadLocalRandom.current().nextInt(config.fileCount); - table.filter().tags(tag).get(); - } - - private void doWrite(Path gord) { - GorDictionaryTable table = fixture.openTable(gord); - int id = ThreadLocalRandom.current().nextInt(1_000_000); - // insert(String...) accepts raw dictionary lines: "file\talias". - table.insert("memtest_extra_" + id + ".gor\tEXTRA" + id); - table.save(); - } - - public static class LoadResult { - public final long readOps; - public final long writeOps; - public final long errors; - public LoadResult(long readOps, long writeOps, long errors) { - this.readOps = readOps; this.writeOps = writeOps; this.errors = errors; - } - @Override public String toString() { - return "reads=" + readOps + " writes=" + writeOps + " errors=" + errors; - } - } -} -``` - -(Prepend copyright header. `insert(String...)` and `save()` are on `DictionaryTable` at lines 187 and 129. If writes to the same table from multiple threads deadlock on the file lock, that is itself a finding — record it and reduce write concurrency in the config rather than changing product code.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadDriver"` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add test/src/main/java/org/gorpipe/test/memory/TableServiceLoadDriver.java \ - test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadDriver.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): add TableServiceLoadDriver for mixed read+write load - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 5: TableServiceLoadMain — profiler entrypoint - -**Files:** -- Create: `test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java` -- Test: `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java` - -**Interfaces:** -- Consumes: `MemoryLoadConfig.fromSystemProperties()` (Task 1), `MemorySampler` (Task 2), `DictionaryFixture` (Task 3), `TableServiceLoadDriver` (Task 4). -- Produces: `TableServiceLoadMain` with `public static void main(String[] args)` and a testable `public static String runOnce(MemoryLoadConfig config, java.nio.file.Path root)` returning a one-line summary string containing `peakHeapMB=` and the op counts. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.junit.Test; - -import java.nio.file.Files; -import java.nio.file.Path; - -import static org.junit.Assert.assertTrue; - -public class UTestTableServiceLoadMain { - - @Test - public void runOnceProducesSummary() throws Exception { - Path root = Files.createTempDirectory("memtest-main"); - MemoryLoadConfig config = new MemoryLoadConfig(30, 10, 10, 2, 4, 70, 2); - String summary = TableServiceLoadMain.runOnce(config, root); - assertTrue("summary reports peak heap", summary.contains("peakHeapMB=")); - assertTrue("summary reports reads", summary.contains("reads=")); - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadMain"` -Expected: FAIL — `TableServiceLoadMain` does not exist. - -- [ ] **Step 3: Write minimal implementation** - -```java -package org.gorpipe.test.memory; - -import java.nio.file.Files; -import java.nio.file.Path; - -/** - * Entry point for profiler-attached runs of the table-service memory harness. - * Run with a prod-like heap and profiling flags, e.g.: - * java -Xmx2g -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp \ - * -XX:StartFlightRecording=filename=load.jfr,settings=profile \ - * -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ - * -cp org.gorpipe.test.memory.TableServiceLoadMain - */ -public class TableServiceLoadMain { - - public static void main(String[] args) throws Exception { - MemoryLoadConfig config = MemoryLoadConfig.fromSystemProperties(); - Path root = Files.createTempDirectory("memtest-run"); - System.out.println("Config: " + config); - System.out.println(runOnce(config, root)); - } - - public static String runOnce(MemoryLoadConfig config, Path root) throws Exception { - MemorySampler sampler = new MemorySampler(100); - sampler.start(); - try { - DictionaryFixture fixture = new DictionaryFixture(root); - var tables = fixture.createTables(config); - TableServiceLoadDriver driver = new TableServiceLoadDriver(tables, fixture, config); - TableServiceLoadDriver.LoadResult result = driver.run(); - System.gc(); - Thread.sleep(200); // let sampler catch post-GC retained heap - sampler.stop(); - long peakHeapMB = sampler.peakHeapUsedBytes() / (1024 * 1024); - long peakRssMB = sampler.peakRssBytes() < 0 ? -1 : sampler.peakRssBytes() / (1024 * 1024); - return "peakHeapMB=" + peakHeapMB + " peakRssMB=" + peakRssMB + " " + result; - } finally { - sampler.stop(); - } - } -} -``` - -(Prepend copyright header.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestTableServiceLoadMain"` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add test/src/main/java/org/gorpipe/test/memory/TableServiceLoadMain.java \ - test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoadMain.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): add TableServiceLoadMain profiler entrypoint - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 6: Scaling validation (SlowTests) - -**Files:** -- Create: `test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java` - -**Interfaces:** -- Consumes: everything from Tasks 1-5. -- Produces: nothing (validation test only). - -This test proves the harness actually stresses the dictionary path: peak retained heap must grow when the `fileCount` (dictionary size) knob grows. If it doesn't, the harness is not exercising the suspected memory driver and must be fixed before profiling. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.gorpipe.test.SlowTests; -import org.junit.Test; -import org.junit.experimental.categories.Category; - -import java.nio.file.Files; -import java.nio.file.Path; - -import static org.junit.Assert.assertTrue; - -@Category(SlowTests.class) -public class UTestTableServiceLoad { - - private long peakHeapForFileCount(int fileCount) throws Exception { - Path root = Files.createTempDirectory("memtest-scale-" + fileCount); - MemoryLoadConfig config = new MemoryLoadConfig(fileCount, 20, 100, 4, 4, 70, 3); - MemorySampler sampler = new MemorySampler(50); - sampler.start(); - DictionaryFixture fixture = new DictionaryFixture(root); - var tables = fixture.createTables(config); - new TableServiceLoadDriver(tables, fixture, config).run(); - System.gc(); - Thread.sleep(200); - sampler.stop(); - return sampler.peakHeapUsedBytes(); - } - - @Test - public void peakHeapScalesWithDictionarySize() throws Exception { - long small = peakHeapForFileCount(500); - long large = peakHeapForFileCount(8000); - assertTrue("peak heap should grow with dictionary size: small=" + small + " large=" + large, - large > small * 1.5); - } -} -``` - -- [ ] **Step 2: Run test to verify it fails, then passes** - -Run: `./gradlew :test:slowTest --tests "org.gorpipe.test.memory.UTestTableServiceLoad"` -Expected: PASS if the harness stresses the dictionary path. If it FAILS (heap does not scale), the harness is not exercising the target — investigate the read/write paths in Tasks 3-4 before proceeding. Do not weaken the assertion to force a pass. - -- [ ] **Step 3: Commit** - -```bash -git add test/src/test/java/org/gorpipe/test/memory/UTestTableServiceLoad.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): validate harness stresses dictionary memory path - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 7: S3-backed fixture variant - -**Files:** -- Modify: `test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java` -- Test: `test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java` - -**Interfaces:** -- Consumes: `MemoryLoadConfig` (Task 1); the existing filesystem `createTables` (Task 3). -- Produces: on `DictionaryFixture`, a new constructor `DictionaryFixture(Path localRoot, String s3Root)` and method `List createTablesOnS3(MemoryLoadConfig config)` writing `.gord` + data to an `s3://bucket/prefix` root via the S3 driver, returning the `.gord` `Path`s (as `s3://...` URIs resolved through the gor `FileReader`). This variant exercises the S3 multipart write buffers (`gor.s3.write.chunksize`). - -This task confirms the 64 MB multipart-buffer hypothesis. It is gated on a dev bucket: the test skips (via `org.junit.Assume`) if the env var `GOR_MEMTEST_S3_ROOT` is unset, so CI without S3 credentials stays green. - -- [ ] **Step 1: Write the failing test** - -```java -package org.gorpipe.test.memory; - -import org.junit.Assume; -import org.junit.Test; - -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.List; - -import static org.junit.Assert.assertTrue; - -public class UTestDictionaryFixtureS3 { - - @Test - public void createsTablesOnS3WhenBucketConfigured() throws Exception { - String s3Root = System.getenv("GOR_MEMTEST_S3_ROOT"); // e.g. s3://my-dev-bucket/memtest - Assume.assumeTrue("GOR_MEMTEST_S3_ROOT not set; skipping S3 fixture test", s3Root != null); - - Path localRoot = Files.createTempDirectory("memtest-s3-local"); - MemoryLoadConfig config = new MemoryLoadConfig(20, 10, 10, 1, 2, 50, 2); - DictionaryFixture fixture = new DictionaryFixture(localRoot, s3Root); - - List tables = fixture.createTablesOnS3(config); - - assertTrue("created at least one S3 table", tables.size() >= 1); - } -} -``` - -- [ ] **Step 2: Run test to verify it is skipped locally** - -Run: `./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixtureS3"` -Expected: PASS as **skipped/ignored** (Assume fails when env unset). This confirms the gate works without credentials. - -- [ ] **Step 3: Write minimal implementation** - -Add to `DictionaryFixture` (keep the existing filesystem members and methods): - -```java - private final String s3Root; // null for filesystem-only fixtures - - public DictionaryFixture(Path root, String s3Root) { - this.root = root; - this.s3Root = s3Root; - } - - /** - * Builds dictionaries whose .gord and bucket files live under an s3:// root, - * exercising the S3 driver write path (multipart buffers). Data files are - * generated locally then written to S3 via the gor table's FileReader. - */ - public List createTablesOnS3(MemoryLoadConfig config) throws IOException { - if (s3Root == null) throw new IllegalStateException("s3Root not configured"); - List tables = new ArrayList<>(); - int[] chrs = {1, 2, 3}; - for (int t = 0; t < config.tableCount; t++) { - String name = "memtest_s3_table_" + t; - String[] sources = new String[config.fileCount]; - for (int i = 0; i < config.fileCount; i++) sources[i] = "PN" + i; - Map> data = GorDictionarySetup.createDataFilesMap( - name, root, config.fileCount, chrs, config.rowsPerChr, "PN", true, sources); - - String gordUri = s3Root.endsWith("/") ? s3Root + name + ".gord" : s3Root + "/" + name + ".gord"; - GorDictionaryTable table = new GorDictionaryTable.Builder<>(gordUri).build(); - table.insert(data); - table.save(); - tables.add(Path.of(gordUri)); - } - return tables; - } -``` - -(The existing single-arg constructor from Task 3 sets `s3Root = null`; update it to `this(root, null)` or add the field assignment. `GorDictionaryTable.Builder<>(String)` accepts a URI string per `GorDictionaryTable.java:64`.) - -- [ ] **Step 4: Run against a real dev bucket (manual, optional in CI)** - -Run: -```bash -GOR_MEMTEST_S3_ROOT=s3:///memtest \ - ./gradlew :test:test --tests "org.gorpipe.test.memory.UTestDictionaryFixtureS3" -``` -Expected: PASS (not skipped) — `.gord` objects appear under the S3 prefix. Requires AWS/OCI credentials in the environment. - -- [ ] **Step 5: Commit** - -```bash -git add test/src/main/java/org/gorpipe/test/memory/DictionaryFixture.java \ - test/src/test/java/org/gorpipe/test/memory/UTestDictionaryFixtureS3.java -git commit -m "$(cat <<'EOF' -test(ENGKNOW-3657): add S3-backed dictionary fixture variant - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -### Task 8: Profiling runbook + findings doc - -**Files:** -- Create: `docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md` -- Create: `docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md` - -**Interfaces:** -- Consumes: `TableServiceLoadMain` (Task 5). -- Produces: documentation only. - -- [ ] **Step 1: Write the runbook** - -Create `docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md` with this content: - -````markdown -# ENGKNOW-3657 Profiling Runbook - -## 1. Get the prod heap ceiling -Fetch the table-service pod memory limit from its helm values (or Grafana -`kube_pod_container_resource_limits{resource="memory"}`). Use it as `-Xmx` below -so the local run reproduces the OOM. - -## 2. Build the classpath -```bash -./gradlew :test:testClasses -CP=$(./gradlew -q :test:printTestClasspath 2>/dev/null || echo "see note") -# If :printTestClasspath is not defined, run via gradle JavaExec or use the -# test runtime classpath from `./gradlew :test:dependencies`. -``` - -## 3. Run under JFR + heap-dump-on-OOM at the prod ceiling -```bash -java -Xmx \ - -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp/engknow3657.hprof \ - -XX:StartFlightRecording=filename=/tmp/engknow3657.jfr,settings=profile,dumponexit=true \ - -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ - -Dgor.memtest.threads=8 -Dgor.memtest.readWritePercent=70 \ - -Dgor.memtest.durationSeconds=300 \ - -cp "$CP" org.gorpipe.test.memory.TableServiceLoadMain -``` -Ramp `fileCount` / `tableCount` until it OOMs at the prod ceiling. - -## 4. Analyze -- Heap dump: open `engknow3657.hprof` in Eclipse MAT or `jhat`. Sort by - **retained size**. Run "Dominator Tree" and "Path to GC Roots" on the top - objects. Check the hypotheses: `DictionaryEntries.rawLines` / - `tagHashToLines` / `contentHashToLines`; `byte[]` part buffers (~64 MB); - cache maps. -- JFR: open `engknow3657.jfr` in JDK Mission Control → Memory → Allocation. - Identify top allocation sites. -- Off-heap: compare `peakRssMB` vs `peakHeapMB` from the harness summary. A - large gap points at native/CRT S3 buffers — rerun with `-XX:NativeMemoryTracking=summary` - and `jcmd VM.native_memory summary`. - -## 5. Fallback: prod heap dump -If it will not reproduce locally, add `-XX:+HeapDumpOnOutOfMemoryError` -`-XX:HeapDumpPath=/data` to the table-service pod JVM args and pull the dump -after the next OOM. -```` - -- [ ] **Step 2: Write the findings template** - -Create `docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md`: - -````markdown -# ENGKNOW-3657 Memory Findings - -> Fill in after running the profiling runbook. This is the deliverable of the diagnosis task. - -## Reproduction -- Prod heap ceiling used (`-Xmx`): __ -- Knobs that triggered OOM: fileCount=__ tableCount=__ threads=__ duration=__ -- Summary line from harness: __ - -## Dominant consumer -- Class / structure: __ -- Retained size: __ MB (__ % of heap) -- Path to GC roots: __ -- Retained past request end? (session cache / static): __ - -## Hypothesis verdicts -- [ ] DictionaryEntries retention (rawLines + multimaps): CONFIRMED / REFUTED — evidence: __ -- [ ] 64 MB multipart write buffers: CONFIRMED / REFUTED — evidence: __ -- [ ] Caches (tagsToListCache / client / metadata): CONFIRMED / REFUTED — evidence: __ -- [ ] Off-heap / CRT S3 (RSS >> heap): CONFIRMED / REFUTED — evidence: __ -- [ ] Unlisted consumer: __ - -## Recommendation -- Proposed fix direction: __ -- Follow-up ticket: __ -```` - -- [ ] **Step 3: Commit** - -```bash -git add docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md \ - docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md -git commit -m "$(cat <<'EOF' -docs(ENGKNOW-3657): add profiling runbook and findings template - -Co-Authored-By: Claude Opus 4.8 (1M context) -EOF -)" -``` - ---- - -## Post-plan: run the diagnosis - -After Task 8, execute the runbook, fill the findings doc, and open the follow-up fix ticket. The fix itself is out of scope for this plan. diff --git a/docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md b/docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md deleted file mode 100644 index 90ed0810..00000000 --- a/docs/superpowers/specs/2026-07-08-engknow-3657-table-service-memory-diagnosis-design.md +++ /dev/null @@ -1,93 +0,0 @@ -# ENGKNOW-3657 — Table-service Memory Usage: Diagnosis Design - -- **Ticket:** [ENGKNOW-3657](https://genedx.atlassian.net/browse/ENGKNOW-3657) — "GOR: Table-service memory usage" -- **Branch:** `ENGKNOW-3657-gor-table-service-memory-usage` -- **Status of problem:** Table-service pods hit OOMKilled under heavy load. Cause not yet pinned. -- **Scope of this task:** **Diagnosis only.** Reproduce, profile, and identify the dominant memory consumer with hard evidence. Any fix is a separate follow-up ticket. - -## Goal - -Reproduce the table-service OOM locally under a mixed read+write dictionary load, profile heap allocation and retention, and identify the **dominant** memory consumer, backed by heap-dump / JFR evidence. Deliver a findings document and a follow-up fix ticket. - -## Evidence driving the design - -From Grafana (`grafanacloud-genedx-prom`), crash window 2026-07-06: - -- Request rate peaked at ~5-6 req/s; ~700 requests over an 8h window — **low throughput**. -- OOM occurred despite low volume ⇒ **cost is per-operation, not throughput-driven**. A few heavy operations (large dictionaries, large writes) exhaust the heap. -- Mixed read+write load; ~322 inserted lines over the 8h window. -- The `tableservice_requests_count` recording rule collapses the `request_type` and `table` labels, so per-type / per-table split is **not** available from Prometheus. Loki logs can refine the mix later if the harness needs tighter calibration. - -**Implication:** the harness needs **modest concurrency but heavy per-operation weight** — large dictionaries and real writes — not high RPS. - -## Where the harness lives - -A new benchmark/test **in `gor-opensource`**. The memory-holding code lives here: - -- `model` — `org.gorpipe.gor.table.dictionary.*` (dictionary/table data structures) -- `drivers` — `org.gorpipe.s3.driver.*` (S3 read/write, multipart buffers) - -The table-service repo is a thin service over this library, so the leak is reproducible without deploying the k8s service. The harness runs as a `@Category(SlowTests)` JUnit test plus a `main` entrypoint for profiler-attached runs. - -## Components - -1. **Fixture builder** — generates synthetic dictionaries at prod-like scale. Configurable: number of lines, buckets, tags, and tables. Filesystem-backed first; an S3-backed variant second (to exercise the S3 driver and multipart write buffers). -2. **Load driver** — a thread pool issuing a mixed operation stream: - - Reads: open dictionary, tag-filtered `getOptimizedEntries`, iterate results. - - Writes: insert lines, bucketize, commit. - - Knobs: read:write ratio, concurrency, dictionary size, duration. - - Emits ops/s and latency so load is quantified. -3. **Memory observation** — runs at `-Xmx` set to the prod pod memory limit (pulled from the table-service helm values), with: - - `-XX:+HeapDumpOnOutOfMemoryError` - - JFR recording (allocation profile + old-gen occupancy) - - periodic `jcmd GC.heap_info` logging - - **RSS vs heap tracking** — the CRT-based S3 async client allocates off-heap; a heap dump alone would miss it. Use Native Memory Tracking (NMT) if off-heap growth is suspected. -4. **Analysis** — load the heap dump / JFR, rank retained size by class, and trace GC roots for the top consumers. - -## Data flow - -``` -fixture builder → dictionaries (fs / S3) - ↓ -load driver (thread pool, mixed read+write) - ↓ -gor library: table/dictionary (model) → drivers/S3 - ↓ -JFR + heap dump + GC.heap_info + RSS - ↓ -analysis → findings report -``` - -## Hypotheses to confirm or refute - -The profiler decides — these are starting points, not conclusions. - -1. **`DictionaryEntries` retention (top suspect).** - `model/.../table/dictionary/DictionaryEntries.java` holds `rawLines: List` plus two `ListMultimap`s (`tagHashToLines`, `contentHashToLines`). Memory scales with dictionary size × number of concurrently-held tables. Key question: are these retained past request end (e.g. via a session cache)? -2. **64 MB multipart write buffers.** - `gor.s3.write.chunksize` defaults to `1<<26` (64 MB), buffered as `byte[]` per part. Concurrent writes multiply this: 64 MB × concurrency. -3. **Caches.** - `DefaultTableAccessOptimizer.tagsToListCache` (100 full file-lists), the S3 client cache (CRT clients are heavy), and the S3 metadata cache (bounded at 10k — likely fine). -4. **An unlisted consumer** — surfaced by the retention profile. - -## Deliverables - -- A reusable load harness, checked in, which doubles as a perf/memory regression guard. -- A findings document: the dominant consumer, evidence (retained sizes, GC-root path), and a recommendation. -- A separate follow-up fix ticket. - -## Testing the harness - -- Reproduce OOM at prod-like `-Xmx`. -- Show memory scales with the dialed knob (dictionary size / concurrency) — confirms the harness stresses the right path. -- Keep the harness as a perf regression test. - -## Risks and fallbacks - -- **OOM won't reproduce locally** → capture a prod heap dump instead (add `-XX:+HeapDumpOnOutOfMemoryError` to the pod). -- **MinIO/localstack ≠ real S3 buffering** → run the S3 write-buffer variant against a real dev bucket. -- **Off-heap growth invisible to heap dump** → watch RSS vs heap; use NMT. - -## Open inputs - -- Prod pod memory limit (from table-service helm values / Grafana) → sets the harness `-Xmx`. From 43375e13f4d524d5bfb7a739cab0ead33acf9d86 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 21:15:13 +0000 Subject: [PATCH 21/27] chore(ENGKNOW-3657): remove findings and runbook from VCS Keep the memory findings and profiling runbook as local-only files; not part of the PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...2026-07-08-engknow-3657-memory-findings.md | 31 ------------- ...26-07-08-engknow-3657-profiling-runbook.md | 43 ------------------- 2 files changed, 74 deletions(-) delete mode 100644 docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md delete mode 100644 docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md diff --git a/docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md b/docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md deleted file mode 100644 index 488afa58..00000000 --- a/docs/superpowers/findings/2026-07-08-engknow-3657-memory-findings.md +++ /dev/null @@ -1,31 +0,0 @@ -# ENGKNOW-3657 Memory Findings - -> Fill in after running the profiling runbook. This is the deliverable of the diagnosis task. - -## Observed during harness build (pre-profiling) - -1. **Harness validated — retained heap scales with dictionary size.** The load harness (`org.gorpipe.test.memory.*`, run via `./gradlew :test:slowTest --tests "org.gorpipe.test.memory.UTestTableServiceLoad"`) models a session/table cache by holding opened `GorDictionaryTable` instances for the run and measuring post-GC retained heap. Retained-heap delta grows with the `fileCount` (dictionary size) knob (validated, 2/2 stable runs). This confirms the harness exercises the suspected retention path (spec hypothesis #1) and is ready for profiling. Note: an earlier peak-heap-during-run metric did NOT scale — peak captured transient allocation churn, not retention; retained-heap-after-GC is the correct signal. - -2. **Candidate root cause surfaced (needs profiling confirmation): `DictionaryEntries` lazy-load data race.** `model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java` (~lines 127-133) guards its lazy load with a non-volatile `dataLoaded` boolean checked OUTSIDE the `synchronized loadLinesAndUpdateIndices()` — a double-checked-locking JMM race, despite the class javadoc claiming thread safety (line 40). Harmless when each request gets its own table instance (the field is only written once, idempotently), but REACHABLE if the real Table Service caches/shares `GorDictionaryTable` instances across concurrent reads. Worst case is a redundant reload (loadLinesAndUpdateIndices is idempotent + synchronized), not corruption — so this is a correctness/thread-safety smell to verify under profiling, and it directly relates to hypothesis #1 (whether dictionaries are retained/shared via a cache). Flag for a follow-up ticket if the profiler confirms table instances are cached/shared in production. - -## Reproduction -- Prod heap ceiling used (`-Xmx`): __ -- Knobs that triggered OOM: fileCount=__ tableCount=__ threads=__ duration=__ -- Summary line from harness: __ - -## Dominant consumer -- Class / structure: __ -- Retained size: __ MB (__ % of heap) -- Path to GC roots: __ -- Retained past request end? (session cache / static): __ - -## Hypothesis verdicts -- [ ] DictionaryEntries retention (rawLines + multimaps): CONFIRMED / REFUTED — evidence: __ -- [ ] 64 MB multipart write buffers: CONFIRMED / REFUTED — evidence: __ -- [ ] Caches (tagsToListCache / client / metadata): CONFIRMED / REFUTED — evidence: __ -- [ ] Off-heap / CRT S3 (RSS >> heap): CONFIRMED / REFUTED — evidence: __ -- [ ] Unlisted consumer: __ - -## Recommendation -- Proposed fix direction: __ -- Follow-up ticket: __ diff --git a/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md b/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md deleted file mode 100644 index 29bc8a00..00000000 --- a/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md +++ /dev/null @@ -1,43 +0,0 @@ -# ENGKNOW-3657 Profiling Runbook - -## 1. Get the prod heap ceiling -Fetch the table-service pod memory limit from its helm values (or Grafana -`kube_pod_container_resource_limits{resource="memory"}`). Use it as `-Xmx` below -so the local run reproduces the OOM. - -## 2. Build the classpath -```bash -./gradlew :test:testClasses -CP=$(./gradlew -q :test:printTestClasspath 2>/dev/null || echo "see note") -# If :printTestClasspath is not defined, run via gradle JavaExec or use the -# test runtime classpath from `./gradlew :test:dependencies`. -``` - -## 3. Run under JFR + heap-dump-on-OOM at the prod ceiling -```bash -java -Xmx \ - -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp/engknow3657.hprof \ - -XX:StartFlightRecording=filename=/tmp/engknow3657.jfr,settings=profile,dumponexit=true \ - -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ - -Dgor.memtest.threads=8 -Dgor.memtest.readWritePercent=70 \ - -Dgor.memtest.durationSeconds=300 \ - -cp "$CP" org.gorpipe.test.memory.TableServiceLoadMain -``` -Ramp `fileCount` / `tableCount` until it OOMs at the prod ceiling. - -## 4. Analyze -- Heap dump: open `engknow3657.hprof` in Eclipse MAT or `jhat`. Sort by - **retained size**. Run "Dominator Tree" and "Path to GC Roots" on the top - objects. Check the hypotheses: `DictionaryEntries.rawLines` / - `tagHashToLines` / `contentHashToLines`; `byte[]` part buffers (~64 MB); - cache maps. -- JFR: open `engknow3657.jfr` in JDK Mission Control → Memory → Allocation. - Identify top allocation sites. -- Off-heap: compare `peakRssMB` vs `peakHeapMB` from the harness summary. A - large gap points at native/CRT S3 buffers — rerun with `-XX:NativeMemoryTracking=summary` - and `jcmd VM.native_memory summary`. - -## 5. Fallback: prod heap dump -If it will not reproduce locally, add `-XX:+HeapDumpOnOutOfMemoryError` -`-XX:HeapDumpPath=/data` to the table-service pod JVM args and pull the dump -after the next OOM. From a471e1a8e06c79c13cc14ebff7368cf1c183ae66 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 21:15:40 +0000 Subject: [PATCH 22/27] docs(ENGKNOW-3657): add profiling runbook Co-Authored-By: Claude Opus 4.8 (1M context) --- ...26-07-08-engknow-3657-profiling-runbook.md | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md diff --git a/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md b/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md new file mode 100644 index 00000000..29bc8a00 --- /dev/null +++ b/docs/superpowers/runbooks/2026-07-08-engknow-3657-profiling-runbook.md @@ -0,0 +1,43 @@ +# ENGKNOW-3657 Profiling Runbook + +## 1. Get the prod heap ceiling +Fetch the table-service pod memory limit from its helm values (or Grafana +`kube_pod_container_resource_limits{resource="memory"}`). Use it as `-Xmx` below +so the local run reproduces the OOM. + +## 2. Build the classpath +```bash +./gradlew :test:testClasses +CP=$(./gradlew -q :test:printTestClasspath 2>/dev/null || echo "see note") +# If :printTestClasspath is not defined, run via gradle JavaExec or use the +# test runtime classpath from `./gradlew :test:dependencies`. +``` + +## 3. Run under JFR + heap-dump-on-OOM at the prod ceiling +```bash +java -Xmx \ + -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp/engknow3657.hprof \ + -XX:StartFlightRecording=filename=/tmp/engknow3657.jfr,settings=profile,dumponexit=true \ + -Dgor.memtest.fileCount=20000 -Dgor.memtest.tableCount=16 \ + -Dgor.memtest.threads=8 -Dgor.memtest.readWritePercent=70 \ + -Dgor.memtest.durationSeconds=300 \ + -cp "$CP" org.gorpipe.test.memory.TableServiceLoadMain +``` +Ramp `fileCount` / `tableCount` until it OOMs at the prod ceiling. + +## 4. Analyze +- Heap dump: open `engknow3657.hprof` in Eclipse MAT or `jhat`. Sort by + **retained size**. Run "Dominator Tree" and "Path to GC Roots" on the top + objects. Check the hypotheses: `DictionaryEntries.rawLines` / + `tagHashToLines` / `contentHashToLines`; `byte[]` part buffers (~64 MB); + cache maps. +- JFR: open `engknow3657.jfr` in JDK Mission Control → Memory → Allocation. + Identify top allocation sites. +- Off-heap: compare `peakRssMB` vs `peakHeapMB` from the harness summary. A + large gap points at native/CRT S3 buffers — rerun with `-XX:NativeMemoryTracking=summary` + and `jcmd VM.native_memory summary`. + +## 5. Fallback: prod heap dump +If it will not reproduce locally, add `-XX:+HeapDumpOnOutOfMemoryError` +`-XX:HeapDumpPath=/data` to the table-service pod JVM args and pull the dump +after the next OOM. From 5df3c60348789f520ea1dede6cfbc6714a02291d Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 21:39:10 +0000 Subject: [PATCH 23/27] fix(ENGKNOW-3657): safe publication for concurrent dictionary reads getEntries()/isLoaded() read dataLoaded+rawLines without synchronization while a synchronized loader publishes them, so a concurrent reader could see dataLoaded==true with a stale rawLines (transient NPE/torn read). This is hit by concurrent selects sharing one cached GorDictionaryTable. Mark the lazily-published DictionaryEntries fields (and TableInfoBase.id) volatile so unsynchronized reads observe fully-constructed state; correct the class contract in the javadoc. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../table/dictionary/DictionaryEntries.java | 27 ++++-- .../gor/table/livecycle/TableInfoBase.java | 2 +- .../UTestDictionaryEntriesConcurrency.java | 87 +++++++++++++++++++ 3 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java diff --git a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java index 80cf48d8..c33fffaa 100644 --- a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java +++ b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java @@ -37,26 +37,35 @@ /** * Class that handles the loading caching of and working with table entries. * - * Class is thread safe. + *

Thread-safety: safe for concurrent reads on a shared instance, including + * concurrent lazy first-load. Loaders and mutators ({@code insert}/{@code delete}/ + * {@code clear}/{@code loadLinesAndUpdateIndices}/{@code updateTagMap}/{@code updateContentMap}) + * are {@code synchronized}, and the lazily-published state fields are {@code volatile} so the + * unsynchronized read accessors observe fully-constructed data. Mutation must be externally + * serialized and must NOT run concurrently with reads on the same instance (structural + * modification of the backing list would break in-flight iteration); share a read-only instance + * for concurrent reads and use a separate instance for writes. */ public class DictionaryEntries implements IDictionaryEntries { private static final Logger log = LoggerFactory.getLogger(DictionaryEntries.class); private final IDictionaryEntryFactory factory; - private List rawLines; + // volatile: lazily built under synchronized builders, but read by unsynchronized accessors + // (e.g. getEntries()/isLoaded()); volatile gives those reads safe publication. + private volatile List rawLines; // For indices we use hashed values. Insert into the dict is much much faster, and it takes a lot less space. Getting data takes // a little bit longer as you could get small list of values you need to loop through. - private ListMultimap tagHashToLines; // tags here means aliases and tags. - private ListMultimap contentHashToLines; + private volatile ListMultimap tagHashToLines; // tags here means aliases and tags. + private volatile ListMultimap contentHashToLines; - private Multiset activeTags; - private int deletedEntriesCount = 0; + private volatile Multiset activeTags; + private int deletedEntriesCount = 0; // only mutated/read under (or after) a synchronized builder. private final TableInfo table; - private boolean dataLoaded = false; - private boolean tagHashLoaded = false; - private boolean contentMapLoaded = false; + private volatile boolean dataLoaded = false; + private volatile boolean tagHashLoaded = false; + private volatile boolean contentMapLoaded = false; /** * Construct new dict file from the given path and chromosome cache. diff --git a/model/src/main/java/org/gorpipe/gor/table/livecycle/TableInfoBase.java b/model/src/main/java/org/gorpipe/gor/table/livecycle/TableInfoBase.java index d348fc3a..95d07842 100644 --- a/model/src/main/java/org/gorpipe/gor/table/livecycle/TableInfoBase.java +++ b/model/src/main/java/org/gorpipe/gor/table/livecycle/TableInfoBase.java @@ -47,7 +47,7 @@ public abstract class TableInfoBase implements TableInfo { private final String linkPath; // Path to the link file. private final String rootUri; // uri to table root (just to improve performance when working with uri's). private String name; // Name of the table. - protected String id = null; // Unique id (based on full path (and possibly timestamp), just so we don't always have to refer to full path). + protected volatile String id = null; // Unique id (based on full path (and possibly timestamp), just so we don't always have to refer to full path). volatile: lazily computed in getId() without synchronization. protected TableHeader header; // Header info. diff --git a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java new file mode 100644 index 00000000..ba22bc85 --- /dev/null +++ b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java @@ -0,0 +1,87 @@ +package org.gorpipe.gor.table.dictionary; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.PrintWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Stress test for concurrent reads on a single shared {@link GorDictionaryTable} — the access + * pattern the table service uses when many selects share one cached dictionary. Guards against + * the lazy-load safe-publication race (transient NPE / torn read) and against + * ConcurrentModificationException on the shared backing list. + */ +public class UTestDictionaryEntriesConcurrency { + + @Rule + public TemporaryFolder workDir = new TemporaryFolder(); + + private static final int ENTRIES = 500; + + private GorDictionaryTable buildTable() throws Exception { + Path dir = workDir.getRoot().toPath(); + for (int i = 0; i < ENTRIES; i++) { + try (PrintWriter out = new PrintWriter(dir.resolve("f" + i + ".gor").toFile())) { + out.println("chrom\tpos"); + out.println("chr1\t" + (i + 1)); + } + } + Path gord = dir.resolve("big.gord"); + try (PrintWriter out = new PrintWriter(gord.toFile())) { + for (int i = 0; i < ENTRIES; i++) out.println("f" + i + ".gor\tpn" + i); + } + return new GorDictionaryTable.Builder<>(gord.toString()).build(); + } + + @Test + public void concurrentReadsOnSharedInstance_noRaceNoCorruption() throws Exception { + GorDictionaryTable table = buildTable(); + + int threads = 16; + int opsPerThread = 200; + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(threads); + List failures = new CopyOnWriteArrayList<>(); + AtomicInteger tagHits = new AtomicInteger(); + + for (int t = 0; t < threads; t++) { + final int base = t; + Thread th = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < opsPerThread; i++) { + // full scan read (exercises getEntries()/rawLines lazy publication) + List all = table.filter().get(); + Assert.assertEquals(ENTRIES, all.size()); + // tag-filtered read (exercises tagHashToLines lazy build) + int idx = (base * 7 + i) % ENTRIES; + List hit = table.filter().tags("pn" + idx).get(); + if (hit.size() == 1) tagHits.incrementAndGet(); + } + } catch (Throwable e) { + failures.add(e); + } finally { + done.countDown(); + } + }); + th.start(); + } + + start.countDown(); + Assert.assertTrue("threads did not finish", done.await(60, TimeUnit.SECONDS)); + Assert.assertTrue("concurrent reads threw: " + failures, failures.isEmpty()); + Assert.assertEquals("every tag lookup should match exactly one entry", + threads * opsPerThread, tagHits.get()); + } +} From d5f6beacfd39c7e8f7e9ac978d7ffa9d81c7e9f3 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 22:18:38 +0000 Subject: [PATCH 24/27] fix(ENGKNOW-3657): safe lazy init of table access optimizer getTableAccessOptimizer() built the optimizer via an unsynchronized null-check on a non-volatile field, so concurrent callers on a shared table could build/publish multiple instances unsafely. Use double-checked locking on a volatile field so concurrent readers share one safely-published optimizer. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dictionary/DictionaryTableReader.java | 19 ++++++++-- .../UTestDictionaryEntriesConcurrency.java | 37 +++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java index b76ed122..547c5400 100644 --- a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java +++ b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java @@ -55,7 +55,9 @@ public class DictionaryTableReader extends TableInfoB protected IDictionaryEntries tableEntries; - protected DictionaryAccessOptimizer tableAccessOptimizer; + // volatile: lazily built in getTableAccessOptimizer() (double-checked) and cleared in reload(); + // volatile gives safe publication to concurrent readers. + protected volatile DictionaryAccessOptimizer tableAccessOptimizer; protected IDictionaryEntryFactory factory; @@ -322,10 +324,19 @@ public List getOptimizedLines(Set tags, boolean allowBucketAccess, bo } protected DictionaryAccessOptimizer getTableAccessOptimizer() { - if (tableAccessOptimizer == null) { - tableAccessOptimizer = new DefaultTableAccessOptimizer(this, tableEntries); + // Double-checked locking on the volatile field: concurrent readers get a single, + // safely-published optimizer instead of racing to build separate ones. + DictionaryAccessOptimizer optimizer = tableAccessOptimizer; + if (optimizer == null) { + synchronized (this) { + optimizer = tableAccessOptimizer; + if (optimizer == null) { + optimizer = new DefaultTableAccessOptimizer(this, tableEntries); + tableAccessOptimizer = optimizer; + } + } } - return tableAccessOptimizer; + return optimizer; } diff --git a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java index ba22bc85..9f088e2e 100644 --- a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java +++ b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java @@ -10,7 +10,9 @@ import java.io.PrintWriter; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; @@ -84,4 +86,39 @@ public void concurrentReadsOnSharedInstance_noRaceNoCorruption() throws Exceptio Assert.assertEquals("every tag lookup should match exactly one entry", threads * opsPerThread, tagHits.get()); } + + @Test + public void concurrentGetOptimizedLines_singleSafeOptimizer() throws Exception { + GorDictionaryTable table = buildTable(); + + int threads = 16; + int opsPerThread = 200; + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(threads); + List failures = new CopyOnWriteArrayList<>(); + + for (int t = 0; t < threads; t++) { + final int base = t; + Thread th = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < opsPerThread; i++) { + Set tags = Collections.singleton("pn" + ((base * 7 + i) % ENTRIES)); + // getOptimizedLines lazily builds the shared table access optimizer. + List res = table.getOptimizedLines(tags, true, false); + Assert.assertEquals(1, res.size()); + } + } catch (Throwable e) { + failures.add(e); + } finally { + done.countDown(); + } + }); + th.start(); + } + + start.countDown(); + Assert.assertTrue("threads did not finish", done.await(60, TimeUnit.SECONDS)); + Assert.assertTrue("concurrent getOptimizedLines threw: " + failures, failures.isEmpty()); + } } From 1e10cabaf4e4bd90bf52cf7d6adbe4d86e06f057 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Wed, 8 Jul 2026 22:20:55 +0000 Subject: [PATCH 25/27] fix(ENGKNOW-3657): safe publication for lazy contentType cache inferShouldBucketizeFromContent() lazily cached the content type in a non-volatile field with an unsynchronized null-check. Make the field volatile and compute via a local; the computation is idempotent so no lock is needed, and reads now observe a safely-published value. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dictionary/DictionaryTableReader.java | 13 +++++--- .../UTestDictionaryEntriesConcurrency.java | 33 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java index 547c5400..807ac5b0 100644 --- a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java +++ b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryTableReader.java @@ -51,7 +51,7 @@ public class DictionaryTableReader extends TableInfoB private static final Logger log = LoggerFactory.getLogger(DictionaryTableReader.class); - private String contentType = null; // For caching content type. + private volatile String contentType = null; // For caching content type. volatile: lazily computed in inferShouldBucketizeFromContent() without a lock. protected IDictionaryEntries tableEntries; @@ -158,10 +158,15 @@ public List needsBucketizing() { } public Boolean inferShouldBucketizeFromContent() { - if (contentType == null) { - contentType = getFileEndingFromContent(); + // Read the volatile field once into a local; the computation is idempotent, so a race + // between concurrent callers just recomputes the same value (last write wins) rather + // than needing a lock. Using the local avoids returning a racily-published field read. + String ct = contentType; + if (ct == null) { + ct = getFileEndingFromContent(); + contentType = ct; } - return inferShouldBucketizeFromType(contentType); + return inferShouldBucketizeFromType(ct); } public Boolean inferShouldBucketizeFromFile(String fileName) { diff --git a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java index 9f088e2e..25bec7c2 100644 --- a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java +++ b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesConcurrency.java @@ -121,4 +121,37 @@ public void concurrentGetOptimizedLines_singleSafeOptimizer() throws Exception { Assert.assertTrue("threads did not finish", done.await(60, TimeUnit.SECONDS)); Assert.assertTrue("concurrent getOptimizedLines threw: " + failures, failures.isEmpty()); } + + @Test + public void concurrentInferBucketize_consistentAndSafe() throws Exception { + GorDictionaryTable table = buildTable(); + Boolean expected = table.inferShouldBucketizeFromContent(); + + int threads = 16; + int opsPerThread = 300; + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(threads); + List failures = new CopyOnWriteArrayList<>(); + + for (int t = 0; t < threads; t++) { + Thread th = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < opsPerThread; i++) { + // exercises the lazy contentType cache + Assert.assertEquals(expected, table.inferShouldBucketizeFromContent()); + } + } catch (Throwable e) { + failures.add(e); + } finally { + done.countDown(); + } + }); + th.start(); + } + + start.countDown(); + Assert.assertTrue("threads did not finish", done.await(60, TimeUnit.SECONDS)); + Assert.assertTrue("concurrent inferShouldBucketizeFromContent threw: " + failures, failures.isEmpty()); + } } From ccb9063e00c3db5e19f923298333941e1d1ed564 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Thu, 9 Jul 2026 19:07:40 +0000 Subject: [PATCH 26/27] fix(ENGKNOW-3657): Fix possible threading use with clear/getEntries. --- .../table/dictionary/DictionaryEntries.java | 52 ++++++++++++++----- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java index c33fffaa..d9d030a6 100644 --- a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java +++ b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java @@ -45,6 +45,10 @@ * serialized and must NOT run concurrently with reads on the same instance (structural * modification of the backing list would break in-flight iteration); share a read-only instance * for concurrent reads and use a separate instance for writes. + * + *

The public read accessors ({@code getEntries()}/{@code getAllActiveTags()}) return + * unmodifiable views so callers cannot structurally modify the shared internal state; + * copy the result if a mutable list is needed (see {@code selectAll()}). */ public class DictionaryEntries implements IDictionaryEntries { private static final Logger log = LoggerFactory.getLogger(DictionaryEntries.class); @@ -94,7 +98,7 @@ synchronized public void insert(T line, boolean hasUniqueTags) { delete(oldLine, true); } - getEntries().add(line); + loadedEntries().add(line); addEntryToContentMap(line); addEntryToTagMap(line); @@ -110,7 +114,7 @@ synchronized public void insert(T line, boolean hasUniqueTags) { synchronized public void delete(T lineToRemove, boolean keepIfBucket) { T line = this.findLine(lineToRemove); if (line != null) { - getEntries().remove(line); + loadedEntries().remove(line); removeEntryFromContentMap(line); removeEntryFromTagMap(line); @@ -118,7 +122,7 @@ synchronized public void delete(T lineToRemove, boolean keepIfBucket) { // NOTE: the deleted flag is part of the hashCode so we remove and add again if we change it. T entry = (T) DictionaryEntry.copy(line); entry.setDeleted(true); - getEntries().add(entry); + loadedEntries().add(entry); addEntryToContentMap(entry); addEntryToTagMap(entry); } @@ -127,6 +131,8 @@ synchronized public void delete(T lineToRemove, boolean keepIfBucket) { @Override synchronized public void clear() { + // We deliberatly set list/maps to null, instead of clearing. This is because someone could be reading + // the buffers and we want to let them finish. dataLoaded = false; this.rawLines = null; clearContentMap(); @@ -135,16 +141,32 @@ synchronized public void clear() { @Override public List getEntries() { - if (!this.dataLoaded) { + // Return an unmodifiable view: the backing list is shared across concurrent readers and + // must not be structurally modified from the outside (see class-level thread-safety note). + return Collections.unmodifiableList(loadedEntries()); + } + + /** + * Load the entries if needed and return the live backing list. + * + *

Reads {@code rawLines} into a single local so the load check and the returned reference are + * the same read; a separate re-read of the volatile field could observe {@code null} if a + * concurrent {@code clear()} slipped in (which the contract forbids, but the local read makes the + * accessor robust anyway). For internal use only — mutators hold the instance lock; read + * accessors that expose the result to callers must wrap it (see {@link #getEntries()}). + */ + private List loadedEntries() { + List lines = this.rawLines; + if (!this.dataLoaded || lines == null) { loadLinesAndUpdateIndices(); + lines = this.rawLines; } - - return this.rawLines; + return lines; } @Override public List getEntries(String... aliasesAndTags) { - List lines2Search = getEntries(); + List lines2Search = loadedEntries(); updateTagMap(); if (tagHashToLines != null && aliasesAndTags != null) { // If we have tags and tag map to lines we use that to get a better list of lines to search.. @@ -195,7 +217,10 @@ public boolean isLoaded() { @Override public Set getAllActiveTags() { updateContentMap(); - return activeTags.elementSet(); + // Read the volatile once and wrap: elementSet() is a live view of the internal multiset and + // supports remove()/clear(), so it must be made unmodifiable before it escapes to callers. + Multiset tags = this.activeTags; + return tags != null ? Collections.unmodifiableSet(tags.elementSet()) : Collections.emptySet(); } @Override @@ -206,13 +231,13 @@ public boolean hasDeletedEntries() { @Override public int size() { - return getEntries().size(); + return loadedEntries().size(); } @Override public int getActiveLinesCount() { updateContentMap(); - int size = getEntries().size(); + int size = loadedEntries().size(); return size - deletedEntriesCount; } @@ -233,6 +258,9 @@ synchronized private void updateContentMap() { synchronized private void updateTagMap() { if (tagHashLoaded) return; + if (!dataLoaded) { + loadLinesAndUpdateIndices(); + } tagHashToLines = ArrayListMultimap.create(rawLines.size(), 1); for (T entry : rawLines) { addEntryToTagMap(entry); @@ -350,7 +378,7 @@ private List loadLines() { @Override public T findLine(T line) { // Using contentHashMap - List allLines = getEntries(); + List allLines = loadedEntries(); updateContentMap(); List lines2Search = contentHashToLines != null ? contentHashToLines.get(line.getSearchHash()) : allLines; @@ -361,7 +389,7 @@ private T findLineWithTag(T line) { String[] linekey = line.getFilterTags(); // Using contentHashMap - List lines2Search = getEntries(); + List lines2Search = loadedEntries(); if (tagHashToLines != null) { lines2Search = new ArrayList<>(); for (String tag : linekey) { From c1811ef9dcff6998c15a36ca2ebecdc541ba6667 Mon Sep 17 00:00:00 2001 From: Gisli Magnusson Date: Thu, 9 Jul 2026 19:09:23 +0000 Subject: [PATCH 27/27] test(ENGKNOW-3657): regression tests for dictionary read-accessor hardening Cover the getEntries()/getAllActiveTags() unmodifiable-view guarantees, the updateTagMap() reload-after-clear guard, and that insert/delete still mutate the backing list after switching to the internal loadedEntries() accessor. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../UTestDictionaryEntriesViews.java | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java diff --git a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java new file mode 100644 index 00000000..b0fd87ad --- /dev/null +++ b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java @@ -0,0 +1,96 @@ +package org.gorpipe.gor.table.dictionary; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.lang.reflect.Field; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; + +/** + * Deterministic regression tests for the read-accessor hardening in {@link DictionaryEntries}: + *

    + *
  • {@code getEntries()} and {@code getAllActiveTags()} must return unmodifiable views so + * callers cannot structurally modify the shared backing state (finding #3).
    • + *
  • The internal mutators ({@code insert}/{@code delete}) must keep working after being + * switched off the public {@code getEntries()} accessor (regression for the refactor).
    • + *
  • {@code getEntries(String...)} must lazily reload after {@code clear()} without tripping + * over a null backing list (finding #2 guard).
    • + *
+ */ +public class UTestDictionaryEntriesViews { + + @Rule + public TemporaryFolder workDir = new TemporaryFolder(); + + private GorDictionaryTable buildTable() throws Exception { + Path dir = workDir.getRoot().toPath(); + Files.writeString(dir.resolve("a.gor"), "chrom\tpos\nchr1\t1\n"); + Files.writeString(dir.resolve("b.gor"), "chrom\tpos\nchr1\t2\n"); + Path gord = dir.resolve("t.gord"); + Files.writeString(gord, "a.gor\tpn1\nb.gor\tpn2\n"); + return new GorDictionaryTable.Builder<>(gord.toString()).build(); + } + + @SuppressWarnings("unchecked") + private IDictionaryEntries entriesOf(GorDictionaryTable table) throws Exception { + Field f = DictionaryTableReader.class.getDeclaredField("tableEntries"); + f.setAccessible(true); + return (IDictionaryEntries) f.get(table); + } + + @Test(expected = UnsupportedOperationException.class) + public void getEntries_returnsUnmodifiableList() throws Exception { + IDictionaryEntries entries = entriesOf(buildTable()); + List list = entries.getEntries(); + Assert.assertEquals(2, list.size()); + list.clear(); // must throw - callers must not mutate the shared backing list + } + + @Test(expected = UnsupportedOperationException.class) + public void getAllActiveTags_returnsUnmodifiableSet() throws Exception { + IDictionaryEntries entries = entriesOf(buildTable()); + Set tags = entries.getAllActiveTags(); + Assert.assertTrue(tags.contains("pn1")); + // Guava's Multiset.elementSet() blocks add() but SUPPORTS remove()/clear(), which would + // strip occurrences from the internal multiset. The returned view must reject removal too. + tags.clear(); // must throw - live view of internal multiset must not be mutable + } + + @Test + public void insertAndDelete_stillMutateBackingList() throws Exception { + GorDictionaryTable table = buildTable(); + IDictionaryEntries entries = entriesOf(table); + + Assert.assertEquals(2, entries.size()); + + GorDictionaryEntry newEntry = + (GorDictionaryEntry) new GorDictionaryEntry.Builder("c.gor", table.getRootPath()).alias("pn3").build(); + entries.insert(newEntry, false); + Assert.assertEquals(3, entries.size()); + + entries.delete(newEntry, false); + Assert.assertEquals(2, entries.size()); + } + + @Test + public void getEntriesByTag_reloadsAfterClear() throws Exception { + GorDictionaryTable table = buildTable(); + IDictionaryEntries entries = entriesOf(table); + + Assert.assertEquals(1, entries.getEntries("pn1").size()); + + entries.clear(); + + // After clear the backing list is dropped; the tag-filtered read must reload it, + // not dereference a null rawLines (finding #2). + List afterClear = entries.getEntries("pn1"); + Assert.assertEquals(1, afterClear.size()); + } +}