main

GGitHubFix CI BDN allocation flakiness (#1989)

858bc1c2创建于 7月28日历史提交

文件	最后提交记录	最后更新时间
GarnetJSON	Fix CI BDN allocation flakiness (#1989) * [Test] Fix BDN allocation-gate flakiness for Set/JSON benchmarks Two independent causes of intermittent BDN allocation-gate failures: 1. Server GC with concurrent (background) collections corrupts BenchmarkDotNet's process-wide MemoryDiagnoser: a Gen2 GC in the measurement window makes the per-op allocation swing wildly (0 to 8x). Use blocking (non-concurrent) GC so the measurement is deterministic. Fixes the JSON ModuleJsonGetCommand spikes. 2. SAddPopSingle emptied its set every iteration (SPOP removes the last member), so the key was deleted and recreated, churning the object-store log and allocating 16MB pages that intermittently landed in the measurement window. Seed key1 with a keeper and use deterministic add/remove so the set is never emptied and the ops stay in-place (no log growth). Renamed to SAddRemSingle since it now exercises SADD/SREM, and tightened the SAddRem/SAddRemSingle thresholds to reflect the ~10x lower, stable allocation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [RESP] Reduce JSON.GET allocation, near-zero for the root path JSON.GET allocated ~536 B/op, dominated by: JSONPath parse of "$" (160), LINQ Sum + boxed WriteBulkString (176), a fresh SerializeToUtf8Bytes byte[] (144), a new List<byte[]> (32), and a path string (24). The high gen0 churn also fed the allocation-measurement variance. - Add a root ("$") fast path (GarnetJsonObject.TryGetRoot) that serializes the whole document into thread-static reusable ArrayBufferWriter/Utf8JsonWriter and writes straight to the RESP output, skipping the path string, JSONPath evaluation, the per-item byte[], and the List. - Add a non-boxing WriteBulkString(ReadOnlySpan<byte[]>) overload in RespWriteUtils/RespMemoryWriter and use it from the JSON GET reader, removing the LINQ Sum and boxed IEnumerable enumeration (helps all JSON.GET paths). ModuleJsonGetCommand: 53600 -> 2400 B (-95.5%, ~24 B/op); latency 143 -> 70 us. Non-root paths (deep/array/filter) drop 5-21% from the WriteBulkString change; their remaining cost is in the custom JSONPath SelectNodes. All 44 JSON command tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Mark Set AOF allocation gates warn-only The AOF variants of SAddRem/SAddRemSingle allocate identically to the None and ACL variants (net10 3200 B, net8 6400 B); the AOF enqueue path adds no per-op heap allocation. Under Server GC the MemoryDiagnoser can occasionally over-count one GC allocation-context quantum (~18 KB) when a background collection lands in the AOF measurement window, producing rare spikes (e.g. 21699 B) unrelated to the workload. Keep None/ACL hard-gated at 6400 and make the AOF variants warn-only, matching the existing treatment of the larger noisy AOF set operations in this config. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Stabilize SortedSet pop/remove BDN benchmarks ZPopMax, ZPopMin, ZMPop and ZRemRangeByScore ran against a single-member sorted set, so each iteration emptied and recreated the key, churning the object-store log (~48-113 KB per op, with rare 16 MB log-page spikes up to ~520 KB). Give each its own key seeded with a keeper member "k" scored so the benchmarked command removes the re-added "d" but never "k", keeping the set non-empty and the operation in-place. Local net10 None: ZPopMax/ZPopMin 12000 B, ZMPop 20000 B, ZRemRangeByScore 48000 B. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Right-size over-provisioned object-type BDN warn thresholds Lower the warn-only allocation thresholds for Hash, ZAddRem and the newly stabilized SortedSet pop/remove benchmarks to reflect measured allocations (~2x the stable value) instead of the previous 10-20x over-provisioning (e.g. HSetDel_None 120000->19200, HMSet_None 114000->12800, ZAddRem_None 149000->32000, ZPopMax_None 48001->24000). The _AOF thresholds for ZPopMax/ ZPopMin keep headroom (74000) for the net10 AOF MemoryDiagnoser over-count; denied-path ACL keys stay at the 6400 baseline. Validated against CI run 30191658574 results: no hard fails, no new warnings on the adjusted benchmarks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Make Set add/remove allocation gates warn-only on all params SAddRem/SAddRemSingle allocate ~3200-6400 B but, being object-store RMW benchmarks, occasionally over-count by ~1 GC allocation-context quantum (~17-18 KB) under Server GC when a background collection lands in the measurement window (observed net10 None 20755 B, net10 AOF 21699 B). The over-count can hit any param, so a tight hard gate flakes. Make the None and ACL variants warn-only to match the already-warn-only AOF variant and the object-store benchmark convention (Hash/List/SortedSet are all warn-only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Set object-type BDN warn thresholds to expected measured values Replace the ~2x/headroom placeholders with the actual measured allocations after stabilization (the gate adds its own +10% tolerance): SortedSet ZAddRem 15200, ZMPop 20000, ZPopMax/ZPopMin 12000, ZRemRangeByScore 48000; Hash HSetDel 9600, HMSet/HMGet 6400, HSetNx/HIncrby/HStrLen 3200, HScan 776. Denied-path ACL keys stay at the 6400 baseline. These are warn-only gates, so the intermittent net10 Server-GC MemoryDiagnoser over-count now surfaces as a warning rather than being masked by an inflated threshold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Restore SAddPopSingle as a stable SADD+SPOP BDN benchmark Revert the earlier SAddPopSingle->SAddRemSingle rename, which had turned a distinct SPOP benchmark into a near-duplicate of SAddRem (both SADD+SREM) and dropped SPOP coverage. Instead keep SADD+SPOP but make it stable: each SADD uses a distinct member (a pool of batchSize members, far larger than the set) so a re-added member has always been popped already and the set never shrinks, and key1 is seeded with several keepers so it never empties. With the key never deleted/recreated there is no object-store log churn, giving a deterministic 7200 B/op across all params and both frameworks (measured; verified stable over repeated runs). Add +spop to the benchmark ACL grant so the ACL variant runs the pop in-place rather than leaving SADD to grow the set unbounded. This preserves the SAddPopSingle chart series (no rename) and keeps SAddRem (SREM) and SAddPopSingle (SPOP) as distinct operations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [RESP] Fix JSON.GET length overflow and unbounded root-buffer retention Two issues in the JSON.GET fast path, found in review: - The response length was summed in an unchecked int, so a >2GB result wrapped negative and bypassed the RESP writer bounds check (the prior LINQ Sum was overflow-checked). Sum in long and reject results above int.MaxValue. - The thread-static root-GET ArrayBufferWriter only grows; ResetWrittenCount keeps the backing array, so one large JSON.GET permanently inflated a long-lived session thread's buffer. Release the buffers once they exceed a 64 KB retention cap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Make SAddPopSingle deterministic; tighten BDN gate coverage - SAddPopSingle: replace the 100-distinct-member SPOP batch with `SADD key a b; SPOP key`, which adds two members and pops one so the set deterministically oscillates between 2 and 1 members and never empties -- no reliance on pool-size-vs-set-size probability. Stable 10400 B (net8) / 7200 B (net10), and faster than the distinct-member version. - Config: SAddPopSingle -> 10400 (covers net8); fix two dead gate keys whose `WARN-ON-FAIL_expected_` prefix never matched the parser (LPushPop_ACL, SScan_AOF); ModuleJsonGetCommand -> warn-only 2400 (was hard 360000, which no longer detects regressions after the near-zero root-path optimization). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Correct BDN GC-job comment and note Workstation-GC follow-up The prior comment claimed Server GC keeps the allocation measurement deterministic; review showed the Server-GC allocation quantum still intermittently inflates MemoryDiagnoser (notably on .NET 10), which is why the object-store allocation gates are warn-only. Note the Workstation-GC job as the follow-up that would allow deterministic hard gates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Make ModuleJsonGetRecursive allocation gate warn-only ModuleJsonGetRecursive is a recursive-descent JSONPath query that genuinely allocates ~30-47 MB with large run-to-run variance (it passed earlier runs below the 33 MB hard-gate ceiling and exceeded it at 47 MB in another). A tight hard gate on a benchmark that variable produces false-positive CI failures, so make it warn-only. The large baseline itself comes from the per-node allocations in the custom JSONPath engine (non-root paths) -- reducing that is a separate follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Objects] JSON.GET: write single-path results directly, cutting per-node allocations The single-path JSON.GET path built a List<byte[]> with one byte[] per matched node, then concatenated them into the RESP bulk string. For a recursive wildcard ($..*) matching ~181 nodes this dominated allocation (BDN ModuleJsonGetRecursive = 2.42 MB/op locally, and up to 47 MB under Server-GC over-count on CI, causing allocation-gate flakes). Add GarnetJsonObject.TryGetToWriter: evaluate the JSONPath and serialize the matched nodes as a single JSON array straight into the thread-static buffer/writer already used by the root ("$") fast path, then write once as a RESP bulk string. Eliminates the List and the per-node byte[] arrays. Output is byte-identical (verified: 44 JsonCommandsTest cases incl. $..author multi-node, wildcard, filter, and empty-match paths). Result (net10 None, local): ModuleJsonGetRecursive 2.42 MB -> 0.52 MB (-78%, deterministic across runs) and 6.20 ms -> 4.59 ms (-26%). The residual 0.52 MB is the lazy-iterator JSONPath engine (SelectNodes/SelectMany) -- a separate follow-up. Also apply the reviewer nit: the residual formatted/multi-path length guard now checks Array.MaxLength (the real byte[] cap) instead of int.MaxValue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Right-size JSON.GET allocation gates to post-fix expected values After the JSON.GET direct-writer change, re-baseline the JsonOperations gates to the measured post-fix allocations (max of net8/net10; the gate applies +10%): - ModuleJsonGetRecursive 50000000 -> 521600 (real 521600, was 2421600 pre-fix) - ModuleJsonGetDeepPath 600000 -> 44000 (HARD -> warn; was 13x over-provisioned) - ModuleJsonGetArrayPath 370000 -> 53600 (HARD -> warn; was 7x over) - ModuleJsonGetFilterPath 770000 -> 64800 (HARD -> warn; was 12x over) - ModuleJsonGetArrayElementsPath 800 -> 800 (HARD -> warn; unchanged value) All JSON.GET path gates are warn-only, matching the object-store policy: the allocation MemoryDiagnoser over-counts under Server GC when a background GC lands in the measurement window. Data (CI runs 30221983810 clean, 30219601330 spike) shows this is proportional to per-op allocation volume: only the heavy Recursive ($..* = ~181 nodes) spikes (net10: 15.4M/47.1M vs 2.42M base); the light ops stayed dead-stable even in the spike run. Cutting Recursive's real allocation 4.6x shrinks its spike ceiling proportionally; the residual net10 spikes now warn (non-blocking) instead of failing. A deterministic hard gate needs the deferred Workstation-GC job (noted in Program.cs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Use Workstation GC for BDN so allocation is measured accurately Root cause of the roving BDN allocation-gate flakes (FilterPath 61k->407k, Recursive->47M, and the object-store spikes): under Server GC, BDN's MemoryDiagnoser reads GC.GetTotalAllocatedBytes(precise:false), whose counter includes the unused allocation-context budgets across Server GC's per-core heaps. When a gen1/gen2 GC resets those contexts between BDN's before/after readings, the counter jumps by N-heaps x budget -- a pure measurement artifact that roves across whichever benchmark catches a context reset mid-measurement. Evidence: spiked "GC: 25 1 1 1667621296 4096" (407134 B/op) vs clean "GC: 32 0 0 250675200 4096" (61200 B/op) for the same benchmark in different runs. Fix: run the benchmarks under Workstation GC (single heap, small budget), which makes GetTotalAllocatedBytes accurate/deterministic. Verified locally: allocation is byte-identical to the clean Server-GC value (FilterPath 61200, Recursive 521600, ZAddRem 15200, SAddRem 3200, HSetDel 6400) and the spikes cannot occur. These are single-client benchmarks, so Server GC provided no latency-fidelity benefit; measured latency is equal-or-slightly-better under Workstation GC (charts are alert-only and this is an improvement, so no alert). This enables restoring tight hard allocation gates (follow-up: re-baseline gates from a Workstation-GC CI run). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Restore hard allocation gates for benchmarks stabilized by Workstation GC With Workstation GC, MemoryDiagnoser now measures allocation deterministically (CI run 30233233102: each of these benchmarks reported an identical value across all 16 rows -- 2 OS x 2 runtimes x 4 params -- with zero spikes). Convert the verified-stable, substantial benchmarks from warn-only back to hard `expected_` gates so real allocation regressions block CI again: - JSON GET: Command 2400, DeepPath 44000, ArrayPath 53600, ArrayElements 800, FilterPath 64800, Recursive 521600 - Set: SAddRem 6400, SAddPopSingle 10400 - SortedSet: ZAddRem 15200, ZMPop 20000 (None/AOF), ZPopMax/ZPopMin 12000 (None/AOF), ZRemRangeByScore 48000 (None/AOF) - Hash: HSetDel 9600; List: LPushPop 11200 (was 14400) Thresholds are the max of the net8/net10 measured values; the gate adds +10%, so the deterministic values leave headroom and cannot false-fail. Left warn-only: the ACL-denied pop paths (NOPERM), the churn benchmarks that empty+recreate a key (ZRemRangeByLex ~112k, ZRemRangeByRank ~86k -- genuine allocation variance, not a measurement artifact), and the near-zero read ops (many now correctly report "-" under Workstation GC, which the gate treats as 0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Address PR review: tighten BDN comments - Program.cs: shorten the GC comment to a present-tense, factual statement of why Workstation GC is used (drop transient spike figures and narrative). - SetOperations.cs: SAddPopSingle comment now states the invariant it relies on (the set is never emptied and key1 is never deleted/recreated) instead of the inaccurate "oscillates between 2 and 1 members" claim (key1 is also seeded with a keeper and shared with SAddRem). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a * [Test] Convert deterministic object-store gates from warn-only to hard Now that Workstation GC makes MemoryDiagnoser deterministic, warn-only is no longer needed for benchmarks with stable allocation. Evaluated every warn-only Set/SortedSet/Hash benchmark against TWO independent Workstation-GC CI runs (30233233102, 30236831459): 62 are deterministic (within-runtime spread <=3% in both runs) and are converted to hard `expected_` gates at the measured max (the gate's +10% leaves headroom). Many old warn thresholds were 5-13x over-provisioned because they absorbed the Server-GC over-count (e.g. ZCount 150000->16800, ZRange 320000->24800) -- the new hard gates are both tight and accurate. Kept warn-only: - Genuine run-to-run variance (churn that empties+recreates a key): ZRemRangeByLex, ZRemRangeByRank, ZRangeStore, and the near-zero oscillator HIncrbyFloat. - Pure-read ops that allocate 0 (show "-"): a hard gate at 0 has no absolute headroom, so these stay warn-only to avoid false-fails on any measurement blip. RawString(LTM)/Lua/Script warn gates are left as-is (their results were not part of this evaluation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e66a8a41-4525-473a-9095-a5e614d95e1a	1 个月前
NoOpModule	Update CacheSizeTracker and LogSizeTracker (#1567) * MInor cleanup * Convert LogSizeTracker to be more fully LogRecord-ized More Comments in BufferAndLoad Fix RandomReadCacheTest input failure * WIP changing to {MemorySize, PageSize, PageCount} where MemorySize includes Heap * WIP: SizeTracker conversion * WIP SizeTracker * WIP on sizeTracker * WIP SizeTracker * WIP SizeTracker * WIP SizeTracker - most tests pass * WIP sizeTracker * Tsavorite fixes for ReadOnlyAddress calculation, Recovery, some fixes for tests; make some ReadAtAddrss and Reviv tests [Explicit] temporarily. A couple Garnet test fixes for required minimum page count * Fix StoreWrapper.RecoverCheckpoint() to ensure SizeTrackers are started Fix MultiDBManager not propagating SizeTracker start when adding a DB Fix SortedSetOps using wrong context Fix Recovery not propagating LastPageFreed Remove SizeTracker fields that duplicated LogSizeTracker Fix Recovery eviction-range calculations Disable some tests temporarily until they can be converted * format * Finish AsyncFlushPagesForReadOnly. Still a little WIP for AsyncFlushPagesForSnapshot * Finish size tracker. Fix some issues (resulting from going to GC.AllocateArray) with reusing allocator pages. Replace the last NativeMemory allocations in Allocator with GC.AllocateArray. * Make a couple tests [Explicit] temporarily Move FreePage() up to just below Alloc/ReturnPage() so they're together * Move to OPMROWorker; more fixes in Recovery * format * Re-enable some tests, and disable a couple others temporarily. Make sure OA page header is set correctly for object log position. * TKey and an interface for key bytes and namespaces (#1605) * Rework Tsavorite so TKey is a parameter again, constained by IKey. In .NET 9+ IKey may be a ref struct. IKeyComparer is now generic (on methods). LogRecord and friends implement IKey. ISourceLogRecord implements TKey. * Get Tsavorite tests compiling. Most are even passing. ObjectIterationPushLockTest is flaky, but that is also true on tedhar/size-tracker . Same thing with RevivificationSpanByteTests, fail in this branch but crash or flake in tedhar/size-tracker . fix up more tests, restorign that weird input special casing * benchmarks compile; no attempt to actually run them, converting Garnet first * update Garnet to use new interfaces; everything compiles, nothing is tested * formatting * add namespace bits to Tsavorite * populate namespace in log records * all Tsavorite tests passing again * punch namespace setting and new comparer into Garnet; this has TODOs for Vector Set keys, but does not introduce them * Fix OA page clearing, Recovery EvictionPageRange, async test keys * format * Turn off "disposed" check on GetPhysicalAddress (due to Cluster.test Dispose sequencing) --------- Co-authored-by: kevin-montrose <kmontrose@microsoft.com>	6 个月前
RoaringBitmap	Add Roaring Bitmap support (issue #1270) (#1741) * Roaring Bitmaps as a Garnet module (issue #1270) Addresses PR review feedback from @badrishc: - Move the extension from main/GarnetServer/Extensions/RoaringBitmap to modules/RoaringBitmap so it isn't bundled by default (mirrors GarnetJSON). - Retarget the PR to dev (companion change). Implementation changes for the move: - New modules/RoaringBitmap/GarnetRoaringBitmap.csproj (mirrors GarnetJSON.csproj, signs assembly, exposes InternalsVisibleTo Garnet.test). - New RoaringBitmapModule : ModuleBase entry point that registers the factory and the four R.SETBIT/R.GETBIT/R.BITCOUNT/R.BITPOS commands. - Renamed namespace Garnet.Extensions.RoaringBitmap -> GarnetRoaringBitmap to avoid the namespace/class collision with class RoaringBitmap. - Updated CustomObjectFunctions overrides to dev-branch scoped ReadOnlySpan<byte> signatures for NeedInitialUpdate / Updater. - Updated RoaringBitmapObject to dev-branch CustomObjectBase ctor and HeapMemorySize accounting. - Wired the module into Garnet.slnx and Garnet.test.csproj. - Tests still register via server.Register.NewCommand in [SetUp] (in-process), matching the existing custom-object test pattern. - Updated StringKeyAndCustomObjectKey_AreSeparate to expect WRONGTYPE on the unified store on dev. * ci: retrigger failed cluster test jobs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use NotFound callback for read-only R.* commands Replace the RMW-on-read workaround for R.GETBIT/R.BITCOUNT/R.BITPOS with the new NotFound callback (PR #1820), so reads no longer create keys. Register these three as CommandType.Read. Drop Allure attributes from the Roaring Bitmap tests to match the test project on main. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Prashant Chinnam <prchinnam@microsoft.com>	3 个月前