diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index dc9ff527579..b2113337d4c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -24,7 +24,7 @@ repos: hooks: - id: check-added-large-files # Retain the exact lossless cross-platform performance evidence archives. - exclude: ^docs/benchmarks/((executor-copy-audit|bounded-read-ahead)-20261101|directory-task-slots-20261102)\.json\.gz$ + exclude: ^docs/benchmarks/((executor-copy-audit|bounded-read-ahead)-20261101|(directory-task-slots|packed-visit-snapshots)-20261102)\.json\.gz$ - id: check-merge-conflict - id: check-yaml # BINARY_TYPES below: archive containers are binary but can START with text (a phar begins with diff --git a/CHANGELOG.md b/CHANGELOG.md index de0d1b7f89a..a7852c72d55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ # 0.11.0 +- Pack matcher and deferred entry text into one owned buffer while preserving moves, output order and filesystem lifetimes. + - Reuse typed directory-task records and publish completion without the shared queue lock. - Reuse budgeted native comparison buffers across chunks while preserving legacy decoder reads. diff --git a/TODO.md b/TODO.md index 37527092ee9..532ef81987d 100644 --- a/TODO.md +++ b/TODO.md @@ -395,8 +395,12 @@ EP/JC/P work; candidates are not speedup claims until measured on both platforms - [x] O08: Qualify typed reusable directory-task slots without changing worker/admission policy. [Initial and refined evidence](docs/benchmarks/directory-task-slots-20261102.md) selects atomic ownership handoff, rejecting global completion locking and retaining small regressions. -- [ ] O09-O12: Profile and qualify batch ownership, bounded result reuse, active-worker dispatch - and direct production evaluation. +- [x] O09: Qualify packed independent matcher/deferred snapshots, including short-string moves, + vector relocation, opaque bytes, and archive ownership. Preserve allocation counts, modest + gains and slower controls in [the two-host report](docs/benchmarks/packed-visit-snapshots-20261102.md). + Keep the forced out-of-line constructor experimental; no batch-recycling API or worker change. +- [ ] O10-O12: Profile and qualify bounded result reuse, active-worker dispatch and direct + production evaluation. Preserve existing stage policy while isolating task storage and lifecycle costs. - [ ] O13-O22: Reconcile remaining derived-value handoff, static catalogs, invariant operands, paired streaming compare, reusable stage pools, byte-bounded overlap, archive indexing/admission, @@ -736,6 +740,12 @@ The safety model and directory-scoped permissions are tracked in ## Active engineering priorities +- [ ] Audit older two-variant qualification-only native drivers for participant-order bias. Rotating + then reversing odd rounds cancels rotation for two participants. Keep historical samples and + source identities; reconfirm small attributed gains with rotation alone before relying on them + for another policy decision. The published benchmark runner already rotates without reversal. + O09 records both the original populations and corrected 32-case confirmation on both hosts. + - [ ] Make local clang-tidy header scope resolve Bazel `include_prefix` aliases. The physical `xff_extras_api/read_source.h` is included as `xff/vfs/read_source.h`, which the current workspace-only include graph silently misses. Add build-metadata-derived mappings and diff --git a/docs/benchmarks/packed-visit-snapshots-20261102.json.gz b/docs/benchmarks/packed-visit-snapshots-20261102.json.gz new file mode 100644 index 00000000000..6a3bda9fb2c Binary files /dev/null and b/docs/benchmarks/packed-visit-snapshots-20261102.json.gz differ diff --git a/docs/benchmarks/packed-visit-snapshots-20261102.md b/docs/benchmarks/packed-visit-snapshots-20261102.md new file mode 100644 index 00000000000..1ae8b9b2611 --- /dev/null +++ b/docs/benchmarks/packed-visit-snapshots-20261102.md @@ -0,0 +1,136 @@ +# Packed owned visit snapshots (2026-11-02) + +## Decision + +Retain one owned byte buffer with offsets for matcher-batch and deferred entry snapshots. The basename shares an exact suffix of the path and the root shares an exact prefix when possible; unrelated values are appended to that same allocation. Preserve opaque bytes, metadata observations, root identity and archive filesystem ownership. This reduces allocation traffic and retained text; whole-command elapsed gains are modest and are not uniform. + +Keep the ordinary constructor. A separately measured no-inline constructor offers no consistent cross-platform advantage and is not adopted. No worker admission, batching, traversal, output, collection-arena policy, parser behavior or CLI option changes. This does not revive the rejected matcher input-buffer recycling API or introduce an arena/shared owner per batch. + +## Lifetime and representation + +Offsets are resolved against the current string on access, so short-string moves and vector relocation cannot leave self-referential views dangling. Copying produces an independent snapshot. Moving transfers its filesystem owner and resets the old text slices and observer. A borrowed Visit remains valid only while its snapshot stays in place. Metadata is copied; the traversal-only lazy loader is omitted exactly as in the former owned record. + +Shard grouping borrows directly from these snapshots. It must not retain a view into a temporary materialized name. The existing complete/superfluous/dedup engine tests detected that error in the first prototype; the selected version passes them and the sanitizer checks. + +Logical path/name/root lengths are unchanged even when storage overlaps. Existing collection budget accounting and matcher batch limits remain unchanged; deferred result sets retain their existing lifetime and size policy. This is fewer allocations, not a claim of a new global memory bound. + +## Measurement contract + +The exact measured parent is `5ca865a453acba2437909ec489c528b989bdf315` (the O08 directory-task slots implementation). LLVM 23.1.4 release builds use the normal `clang_release`/ThinLTO configuration. Source variants, executable hashes, commands, fixture descriptions, complete raw repetitions and reproduction scripts are retained below. Formatting and final test integration do not change the timed production mechanism. + +The initial population has 120 native cases per host, three sessions, discarded per-variant warmups and nine interleaved rounds with the fastest seven retained. It contains 14,400 fresh invocations across both hosts, including warmups. Case order is forward, reverse and seeded; variant order rotates and reverses. Every invocation verifies independent path/size digests or complete collection/comparison records. Linux restricts affinity to the requested 1/3/10 CPUs on tmpfs. macOS uses warm APFS without affinity enforcement. + +Fixtures include 10/1,000/10,000 flat files, broad and nested trees through 65,536 files, long collection names, and small/large two-root comparison controls. Added explicit-case listing/name controls are deliberately serial matcher controls: only content expressions and rg searches enter the current match pool. Actual rg file-selection searches exercise batching. Deferred shard fixtures contain 1,000/10,000 independent complete sets with long names and roots. + +Two sessions of 36 in-memory RE2 workloads per variant retain nine repetitions/fastest seven with 1/4 worker allowances, without native affinity or an independent per-iteration oracle. These are separate engine controls, not fresh CLI startup timings. No build, sanitizer or lint runs concurrently with timing on the same host. + +## Initial native results + +Changes are medians of paired session ratios; negative is faster. Every cell remains in the archive, including slower controls. + +| Fixture / task | Workers | Linux change | macOS change | +| :-------------------------- | ------: | -----------: | -----------: | +| flat10 / files | 1 | -0.47% | +0.32% | +| flat10 / files-exact | 10 | +2.33% | -0.13% | +| flat10000 / rg-filter | 3 | -1.37% | -0.10% | +| broad128x512 / rg-filter | 10 | -2.50% | -0.09% | +| broad128x512 / files-exact | 3 | -4.50% | -0.26% | +| nested8x8x64 / content | 10 | -2.94% | +0.42% | +| shard1000 / shard-deferred | 1 | -1.76% | -0.37% | +| shard10000 / shard-deferred | 1 | -1.63% | +0.29% | +| shard10000 / shard-deferred | 3 | -1.77% | -0.95% | +| broad128x512 / collect-long | 3 | +1.93% | -1.64% | +| compare-small / compare | 3 | +4.45% | +0.34% | +| compare-large / compare | 3 | +9.06% | -0.43% | + +The initial Linux comparison controls include +4.45% and +9.06% medians; do not hide those samples. A five-session six-case confirmation includes a second label executing the identical baseline binary. That control itself varies, while several candidate costs remain. A further five-session eight-case constructor experiment uses byte-identical original Linux baseline/packed executables, and the comparison changes become -0.77%/+1.48%. Those costs are not stable across populations. The raw +4.09%/+3.84% intermediate confirmation and all duplicate-baseline results remain available; later runs do not replace earlier ones. + +## Constructor controls + +This supplementary population compares baseline, ordinary packed storage and forced out-of-line construction over five sessions/eight cases per host. It contains 2,400 fresh invocations, plus the earlier Linux duplicate-baseline confirmation has 900. Do not combine absolute times with the initial matrix. Rebuilding on macOS can change executable identity/size slightly; the manifests preserve every binary separately. + +| Fixture / task | Workers | Linux packed | Linux out of line | macOS packed | macOS out of line | +| :-------------------------- | ------: | -----------: | ----------------: | -----------: | ----------------: | +| compare-small / compare | 3 | -0.77% | +0.08% | +0.54% | +0.18% | +| compare-large / compare | 3 | +1.48% | +0.59% | -0.62% | -0.58% | +| flat10 / files | 1 | +1.31% | +1.26% | -0.18% | +0.22% | +| flat10 / files-exact | 10 | +0.53% | +0.82% | +0.46% | +0.44% | +| flat1000 / rg-filter | 3 | -0.18% | +0.52% | -0.48% | -0.08% | +| flat10000 / rg-filter | 3 | -2.73% | -2.34% | -0.21% | -0.25% | +| shard10000 / shard-deferred | 1 | -1.77% | -2.51% | +0.20% | +0.16% | +| shard10000 / shard-deferred | 3 | -3.21% | -3.91% | -1.49% | -1.02% | + +## Final direct-construction confirmation + +Scoped clang-tidy required direct emplace construction in the pending batch. This also removes the temporary snapshot move. Preserve that as a distinct final population: 32 representative native cases per host, three sessions, nine rounds/fastest seven, with the same output oracles. Both hosts also repeat the 36 in-memory controls. The final native population contains 3,840 fresh invocations including warmups. Existing full suites passed before this call-site refinement; the five affected release/ASan suites and Linux thread-safety suites were rerun afterward. + +| Fixture / task | Workers | Linux final change | macOS final change | +| :-------------------------- | ------: | -----------------: | -----------------: | +| flat10 / files | 1 | -1.38% | +0.25% | +| flat10 / content | 10 | +1.92% | +0.26% | +| flat10000 / rg-filter | 3 | -1.60% | -0.48% | +| broad128x512 / content | 3 | -1.44% | -0.62% | +| broad128x512 / rg-filter | 3 | -2.09% | -0.32% | +| shard10000 / shard-deferred | 1 | -1.38% | -1.00% | +| shard10000 / shard-deferred | 3 | -1.50% | -0.07% | +| compare-small / compare | 3 | +4.23% | -0.04% | +| compare-large / compare | 3 | -1.70% | -0.04% | + +## Counterbalanced-order confirmation + +The qualification-only driver originally rotated the participants and reversed odd rounds. For two variants those operations cancel: every round within a case kept the same participant first. The original data remains separately identified and does not establish order-independent tiny gains. The corrected driver rotates without parity reversal and repeats the final 32 native cases, three sessions/nine rounds/fastest seven on both hosts. This adds 3,840 fresh oracle-checked invocations. The published benchmark runner already uses rotation without that reversal and is unaffected. + +| Fixture / task | Workers | Linux balanced change | macOS balanced change | +| :-------------------------- | ------: | --------------------: | --------------------: | +| flat10 / files | 1 | -0.64% | +0.66% | +| flat10 / content | 10 | -1.81% | -0.72% | +| flat10000 / rg-filter | 3 | -3.63% | -0.58% | +| broad128x512 / content | 3 | -1.84% | -0.64% | +| broad128x512 / rg-filter | 3 | -2.17% | -0.65% | +| shard10000 / shard-deferred | 1 | -2.09% | -0.84% | +| shard10000 / shard-deferred | 3 | -3.37% | -0.75% | +| compare-small / compare | 3 | +0.10% | -0.18% | +| compare-large / compare | 3 | +3.33% | -0.08% | + +The native helper also preserves user/system CPU time and child peak RSS per invocation. First-output latency and allocator-retained bytes are not measured separately in this qualification. No default, case-policy, worker-admission or output-order change is inferred from these results. + +## Allocation traffic and footprint + +ASan hooks count only coordinator allocations, with setup and assertions excluded. All snapshots stay alive through counting. These are requested bytes and allocation traffic, not peak RSS, allocator-resident memory, or sanitizer performance timing. Each case snapshots 1,000 entries, including one vector allocation. Short strings retain their allocation-free text path. Long independent name/root values exercise the fallback. + +| Host / text length | Overlap | Old allocations | Packed allocations | Old requested bytes | Packed requested bytes | +| :----------------- | :------- | --------------: | -----------------: | ------------------: | ---------------------: | +| macos / 1 | overlap | 1 | 1 | 232,000 | 224,000 | +| macos / 1 | fallback | 1 | 1 | 232,000 | 224,000 | +| macos / 64 | overlap | 3,001 | 1,001 | 512,000 | 360,000 | +| macos / 64 | fallback | 2,001 | 1,001 | 376,000 | 368,000 | +| macos / 1,024 | overlap | 3,001 | 1,001 | 4,352,000 | 2,280,000 | +| macos / 1,024 | fallback | 2,001 | 1,001 | 2,296,000 | 2,288,000 | +| linux / 1 | overlap | 1 | 1 | 232,000 | 224,000 | +| linux / 1 | fallback | 1 | 1 | 232,000 | 224,000 | +| linux / 64 | overlap | 3,001 | 1,001 | 536,000 | 368,000 | +| linux / 64 | fallback | 2,001 | 1,001 | 392,000 | 368,000 | +| linux / 1,024 | overlap | 3,001 | 1,001 | 4,376,000 | 2,288,000 | +| linux / 1,024 | fallback | 2,001 | 1,001 | 2,312,000 | 2,288,000 | + +Executable sizes below are the original build outputs before release stripping/compression: + +- linux: 12,295,424 -> 12,296,208 bytes (+784). +- macos: 4,051,984 -> 4,051,440 bytes (-544). + +Final direct-construction build outputs are a separate population: + +- linux: 12,295,424 -> 12,294,864 bytes (-560). +- macos: 4,052,000 -> 4,051,440 bytes (-560). + +## Validation and limits + +Both complete core/extension release suites pass 306/306. Linux uses the required-FUSE configuration. Focused ASan includes ownership/move tests, repeated batches, actual rg workers, deferred shard processing and the isolated allocation diagnostic. Linux TSan passes ownership, matcher and rg worker suites. Scoped clang-tidy covers the modified sources and dependent translation units; normal commit/push hooks and current-head CI remain required before merge. + +Dedicated tests cover destroyed caller storage, shared/fallback bytes, counted non-terminated slices, embedded nulls, empty values, short/long copies and moves, vector relocation, metadata and root identity, and filesystem ownership. Existing archive/collection/safety/deferred tests remain in the full suites. No Unicode fixture is mixed into these ordinary byte/lifetime tests. + +The main benefit is reduced allocation traffic and snapshot bytes, with modest elapsed gains in the qualified content/deferred workloads. Do not call every scan faster, infer new worker defaults, or reuse this evidence for a batch arena, input-recycling API, shared executor or network-storage policy. In-memory/session noise and small-process costs remain explicit. + +## Lossless evidence + +[packed-visit-snapshots-20261102.json.gz](packed-visit-snapshots-20261102.json.gz): 1,676,099 bytes; SHA-256 `c841d5d9d961838f776a2b88c663a6d44270b47b273b5db8c455a1943b6764ec`. Native fixture dictionaries are restored from `fixtures[report.fixtures_ref]`. Initial, duplicate-baseline, constructor, kernel and allocation populations retain their distinct provenance. diff --git a/docs/performance-analysis.md b/docs/performance-analysis.md index 6e3fbd5f0c3..a7ea737efd8 100644 --- a/docs/performance-analysis.md +++ b/docs/performance-analysis.md @@ -3240,3 +3240,31 @@ scoped lint pass. The two three-way native phases preserve all 28,800 invocation oracles, fixture definitions, source/binary identities and slower cases. Slot metadata retains its high-water capacity until walker destruction; the per-level request frontier is not a global byte bound. Cache-line layout and wider scheduling changes remain separate, unmeasured follow-ups. + +## O09: packed matcher and deferred entry snapshots + +The collection arena in O01 does not own matcher/deferred input: those records formerly copied +path, basename and root into three independent strings. The qualified replacement keeps one byte +string plus offsets, sharing exact path suffix/prefix bytes and appending unrelated values. Views +are reconstructed on access, so short-string moves and vector growth do not invalidate them. +Metadata, root identity and archive filesystem owners retain their existing lifetimes. + +[The full decision and evidence](benchmarks/packed-visit-snapshots-20261102.md) separate native +whole-command timing, in-memory controls and ASan allocation counters on macOS and Linux. Long +overlapping 1,000-entry snapshots reduce allocation calls from 3,001 to 1,001; short strings retain +their allocation-free text path. This is reduced allocation traffic with modest elapsed gains, not a +universal scan speedup. Initial slower Linux comparison controls and duplicate-baseline confirmations +remain in the evidence; do not replace the original samples with later results. + +Retain ordinary construction. A forced out-of-line constructor offers no consistent cross-platform +advantage. The final direct-emplace call site has a distinct confirmation population. Preserve +existing input-vector consumption, ordered output, logical text lengths, worker admission and +collection budgets. No new batch arena, reference-counted text owner, interner or recycling API is +introduced. O10's bounded result-capacity reuse remains a separate candidate. + +The O09 qualification-only driver exposed a measurement-order issue: rotating participants and +reversing odd rounds cancel each other for two variants. Preserve the original samples as such; +the corrected representative native confirmation rotates participants without that reversal. The +published benchmark runner already uses rotation alone and is unaffected. Audit older two-variant +qualification scripts before treating tiny historical differences as order-independent evidence. +Allocation counters and correctness tests do not depend on participant timing order. diff --git a/release-site.json b/release-site.json index e6f5cf5421f..d0a7270761e 100644 --- a/release-site.json +++ b/release-site.json @@ -66,7 +66,8 @@ "docs/benchmarks/field-lookup-controls-20261102.md": "docs/benchmarks/field-lookup-controls-20261102.html", "docs/benchmarks/host-range-overwrite-20261102.md": "docs/benchmarks/host-range-overwrite-20261102.html", "docs/benchmarks/cursor-owned-read-buffers-20261102.md": "docs/benchmarks/cursor-owned-read-buffers-20261102.html", - "docs/benchmarks/directory-task-slots-20261102.md": "docs/benchmarks/directory-task-slots-20261102.html" + "docs/benchmarks/directory-task-slots-20261102.md": "docs/benchmarks/directory-task-slots-20261102.html", + "docs/benchmarks/packed-visit-snapshots-20261102.md": "docs/benchmarks/packed-visit-snapshots-20261102.html" }, "generated_html": { "XFF.md": { diff --git a/xff/engine/BUILD.bazel b/xff/engine/BUILD.bazel index 853cc489716..68ab2f350ed 100644 --- a/xff/engine/BUILD.bazel +++ b/xff/engine/BUILD.bazel @@ -140,6 +140,31 @@ cc_test( ], ) +cc_library( + name = "owned_visit_cc", + srcs = ["owned_visit.cc"], + hdrs = ["owned_visit.h"], + deps = [ + ":walk_cc", + "//xff/vfs:vfs_cc", + "@mboworks_mbo//mbo/types:optional_ref_cc", + ], +) + +cc_test( + name = "owned_visit_test", + size = "small", + srcs = ["owned_visit_test.cc"], + deps = [ + ":owned_visit_cc", + ":walk_cc", + "//xff/vfs:local_fs_cc", + "//xff/vfs:vfs_cc", + "@googletest//:gtest_main", + "@mboworks_mbo//mbo/testing:matchers_cc", + ], +) + cc_library( name = "collect_cc", srcs = ["collect.cc"], @@ -344,9 +369,9 @@ cc_library( srcs = ["parallel_match.cc"], hdrs = ["parallel_match.h"], deps = [ - ":collect_cc", ":evaluate_cc", ":expression_execution_cc", + ":owned_visit_cc", "//xff/fuzzy:fuzzy_cc", "//xff/parser:parser_cc", "//xff/registry:registry_cc", @@ -365,11 +390,13 @@ cc_test( deps = [ ":evaluate_cc", ":expression_execution_cc", + ":owned_visit_cc", ":parallel_match_cc", "//xff/matching/regex:regex_cc", "//xff/parser:parser_cc", "//xff/vfs:local_fs_cc", "@googletest//:gtest_main", + "@mboworks_mbo//mbo/testing:matchers_cc", "@mboworks_mbo//mbo/testing:status_cc", ], ) @@ -387,6 +414,7 @@ cc_library( ":expression_execution_cc", ":extract_cc", ":mount_cc", + ":owned_visit_cc", ":parallel_compare_cc", ":parallel_match_cc", ":walk_cc", @@ -660,8 +688,10 @@ cc_test( size = "small", srcs = ["rg_test.cc"], deps = [ + ":owned_visit_cc", ":parallel_match_cc", ":run_cc", + ":walk_cc", "//xff/parser:parser_cc", "//xff/vfs:local_fs_cc", "//xff/vfs:vfs_cc", @@ -954,9 +984,11 @@ cc_test( size = "small", srcs = ["expression_allocations_test.cc"], deps = [ + ":collect_cc", ":evaluate_cc", ":expression_execution_cc", ":expression_qualification_cc", + ":owned_visit_cc", ":parallel_compare_cc", ":walk_cc", "//xff/fuzzy:fuzzy_cc", diff --git a/xff/engine/expression_allocations_test.cc b/xff/engine/expression_allocations_test.cc index a128b48faba..f98c459b329 100644 --- a/xff/engine/expression_allocations_test.cc +++ b/xff/engine/expression_allocations_test.cc @@ -22,9 +22,11 @@ #include "gtest/gtest.h" #include "mbo/status/status_macros.h" #include "mbo/testing/status.h" +#include "xff/engine/collect.h" #include "xff/engine/evaluate.h" #include "xff/engine/expression_execution.h" #include "xff/engine/expression_qualification.h" +#include "xff/engine/owned_visit.h" #include "xff/engine/parallel_compare.h" #include "xff/engine/walk.h" #include "xff/fuzzy/fuzzy.h" @@ -327,6 +329,58 @@ TEST_F(AllocationTest, RecordCoordinatorDirectoryTaskAllocationTraffic) { // This diagnostic reports coordinator traffic; sanitizer timing is never performance evidence. } +TEST_F(AllocationTest, PackedVisitTextAvoidsIndependentLongStringAllocations) { + const vfs::Metadata metadata{.type = vfs::FileType::kRegular}; + for (const std::size_t length : {1, 64, 1'024}) { + const std::string root(length, 'r'); + const std::string name(length, 'n'); + std::string path = root; + path += '/'; + path += name; + for (const bool overlap : {true, false}) { + const Visit visit{ + .path = overlap ? std::string_view(path) : std::string_view("independent"), + .name = name, + .root = root, + .metadata = metadata, + }; + const auto separate = MeasureAllocations([&] { + std::vector entries; + entries.reserve(1'000); + for (std::size_t index = 0; index < 1'000; ++index) { + entries.push_back({ + .path = std::string(visit.path), + .name = std::string(visit.name), + .root = std::string(visit.root), + .metadata = metadata, + }); + } + return entries; + }); + const auto packed = MeasureAllocations([&] { + std::vector entries; + entries.reserve(1'000); + for (std::size_t index = 0; index < 1'000; ++index) { + entries.emplace_back(visit); + } + return entries; + }); + EXPECT_THAT(separate.value, SizeIs(1'000)); + EXPECT_THAT(packed.value, SizeIs(1'000)); + EXPECT_THAT(packed.value.back().LogicalTextBytes(), Eq(visit.path.size() + name.size() + root.size())); + if (length >= 64) { + EXPECT_THAT(separate.counts.allocations, Gt(packed.counts.allocations)); + } else { + EXPECT_THAT(separate.counts.allocations, Eq(packed.counts.allocations)); + } + const std::string phase = absl::StrCat(length, overlap ? "_overlap" : "_fallback"); + RecordCounts(phase + "_separate", separate.counts); + RecordCounts(phase + "_packed", packed.counts); + } + } + // Isolated coordinator allocation counts only. Do not use sanitizer duration as timing evidence. +} + TEST_F(AllocationTest, ReusedFuzzyWorkspaceAvoidsHotScoreAllocations) { const fuzzy::FzfQuery query("fl"); fuzzy::ScoreWorkspace workspace; diff --git a/xff/engine/owned_visit.cc b/xff/engine/owned_visit.cc new file mode 100644 index 00000000000..097718e5bd1 --- /dev/null +++ b/xff/engine/owned_visit.cc @@ -0,0 +1,73 @@ +// SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +// SPDX-License-Identifier: Apache-2.0 + +#include "xff/engine/owned_visit.h" + +#include + +namespace xff::engine { + +OwnedVisit::OwnedVisit(const Visit& visit) + : depth(visit.depth), + metadata(visit.metadata), + fs(visit.fs), + fs_owner(visit.fs_owner), + root_index(visit.root_index), + path_size_(visit.path.size()) { + const bool shared_name = visit.path.ends_with(visit.name); + const bool shared_root = visit.path.starts_with(visit.root); + bytes_.reserve(visit.path.size() + (shared_name ? 0 : visit.name.size()) + (shared_root ? 0 : visit.root.size())); + bytes_.append(visit.path); + name_ = {.offset = shared_name ? path_size_ - visit.name.size() : bytes_.size(), .size = visit.name.size()}; + if (!shared_name) { + bytes_.append(visit.name); + } + root_ = {.offset = shared_root ? 0 : bytes_.size(), .size = visit.root.size()}; + if (!shared_root) { + bytes_.append(visit.root); + } +} + +OwnedVisit::OwnedVisit(OwnedVisit&& other) noexcept + : depth(other.depth), + metadata(other.metadata), + fs(other.fs), + fs_owner(std::move(other.fs_owner)), + root_index(other.root_index), + bytes_(std::move(other.bytes_)), + path_size_(std::exchange(other.path_size_, 0)), + name_(std::exchange(other.name_, {})), + root_(std::exchange(other.root_, {})) { + other.fs.reset(); +} + +std::string_view OwnedVisit::Path() const { + return std::string_view(bytes_).substr(0, path_size_); +} + +std::string_view OwnedVisit::Name() const { + return std::string_view(bytes_).substr(name_.offset, name_.size); +} + +std::string_view OwnedVisit::Root() const { + return std::string_view(bytes_).substr(root_.offset, root_.size); +} + +std::size_t OwnedVisit::LogicalTextBytes() const { + return path_size_ + name_.size + root_.size; +} + +Visit OwnedVisit::AsVisit() const { + return { + .path = Path(), + .name = Name(), + .root = Root(), + .depth = depth, + .metadata = metadata, + .fs = fs, + .fs_owner = fs_owner, + .root_index = root_index, + }; +} + +} // namespace xff::engine diff --git a/xff/engine/owned_visit.h b/xff/engine/owned_visit.h new file mode 100644 index 00000000000..2184442a999 --- /dev/null +++ b/xff/engine/owned_visit.h @@ -0,0 +1,56 @@ +// SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +// SPDX-License-Identifier: Apache-2.0 + +#ifndef XFF_ENGINE_OWNED_VISIT_H_ +#define XFF_ENGINE_OWNED_VISIT_H_ + +#include +#include +#include +#include + +#include "mbo/types/optional_ref.h" +#include "xff/engine/walk.h" +#include "xff/vfs/filesystem.h" + +namespace xff::engine { + +// An independent snapshot of opaque path/name/root bytes and entry observations. Text shares +// one owned allocation where the original values overlap, without encoding/path normalization. +// Offsets, rather than stored views, survive short-string moves and vector relocation. +struct OwnedVisit { + OwnedVisit() = default; + explicit OwnedVisit(const Visit& visit); + OwnedVisit(const OwnedVisit&) = default; + OwnedVisit(OwnedVisit&& other) noexcept; + OwnedVisit& operator=(const OwnedVisit&) = delete; + OwnedVisit& operator=(OwnedVisit&&) = delete; + + int depth = 0; + vfs::Metadata metadata; + mbo::types::OptionalRef fs; + std::shared_ptr fs_owner; + std::size_t root_index = 0; + + std::string_view Path() const; + std::string_view Name() const; + std::string_view Root() const; + // Logical lengths remain unchanged even when their physical byte ranges overlap. + std::size_t LogicalTextBytes() const; + Visit AsVisit() const; + + private: + struct Slice { + std::size_t offset = 0; + std::size_t size = 0; + }; + + std::string bytes_; + std::size_t path_size_ = 0; + Slice name_; + Slice root_; +}; + +} // namespace xff::engine + +#endif // XFF_ENGINE_OWNED_VISIT_H_ diff --git a/xff/engine/owned_visit_test.cc b/xff/engine/owned_visit_test.cc new file mode 100644 index 00000000000..743493e261a --- /dev/null +++ b/xff/engine/owned_visit_test.cc @@ -0,0 +1,170 @@ +// SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +// SPDX-License-Identifier: Apache-2.0 + +#include "xff/engine/owned_visit.h" + +#include +#include +#include +#include +#include +#include +#include + +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "mbo/testing/matchers.h" +#include "xff/engine/walk.h" +#include "xff/vfs/local_fs.h" + +namespace xff::engine { +namespace { + +using ::mbo::testing::EqualsText; +using ::testing::Eq; +using ::testing::IsEmpty; +using ::testing::IsFalse; +using ::testing::IsTrue; +using ::testing::Ref; + +struct OwnedVisitTest : ::testing::Test { + const vfs::Metadata metadata; +}; + +TEST_F(OwnedVisitTest, PreservesOverlappingAndIndependentTextAfterSourceDestruction) { + struct Text { + std::string_view path; + std::string_view name; + std::string_view root; + }; + + static constexpr std::array kCases = std::to_array({ + {.path = "", .name = "", .root = ""}, + {.path = "./leaf", .name = "leaf", .root = "."}, + {.path = "leaf", .name = "leaf", .root = "."}, + {.path = "/root/leaf", .name = "other", .root = "/elsewhere"}, + {.path = "/root//leaf/", .name = "leaf", .root = "/root/"}, + {.path = "archive.zip!/nested/leaf", .name = "leaf", .root = "archive.zip"}, + {.path = "prefix-name", .name = "name", .root = "prefix"}, + {.path = "p", .name = "longer-name", .root = "longer-root"}, + }); + for (const auto& expected : kCases) { + const OwnedVisit owned = [&] { + const std::string path(expected.path); + const std::string name(expected.name); + const std::string root(expected.root); + return OwnedVisit(Visit{.path = path, .name = name, .root = root, .metadata = metadata}); + }(); + EXPECT_THAT(owned.Path(), EqualsText(expected.path)); + EXPECT_THAT(owned.Name(), EqualsText(expected.name)); + EXPECT_THAT(owned.Root(), EqualsText(expected.root)); + EXPECT_THAT(owned.LogicalTextBytes(), Eq(expected.path.size() + expected.name.size() + expected.root.size())); + } +} + +TEST_F(OwnedVisitTest, PreservesCountedSlicesAndEmbeddedNullBytes) { + const std::string bytes("ignored/root/a\0b/ignored", 24); + const std::string_view path = std::string_view(bytes).substr(7, 9); + const std::string_view name = path.substr(5); + const std::string_view root = path.substr(0, 4); + const OwnedVisit owned(Visit{.path = path, .name = name, .root = root, .metadata = metadata}); + EXPECT_THAT(owned.Path(), Eq(path)); + EXPECT_THAT(owned.Name(), Eq(name)); + EXPECT_THAT(owned.Root(), Eq(root)); + EXPECT_THAT(owned.LogicalTextBytes(), Eq(path.size() + name.size() + root.size())); + const std::string_view independent("x\0y", 3); + const OwnedVisit fallback(Visit{.path = "a", .name = independent, .root = independent, .metadata = metadata}); + EXPECT_THAT(fallback.Name(), Eq(independent)); + EXPECT_THAT(fallback.Root(), Eq(independent)); +} + +TEST_F(OwnedVisitTest, CopiesAndMovesRebuildViewsForShortAndLongStrings) { + for (const std::size_t length : {1, 16, 256}) { + const std::string root(length, 'r'); + const std::string name(length, 'n'); + std::string path = root; + path += '/'; + path += name; + OwnedVisit original(Visit{.path = path, .name = name, .root = root, .metadata = metadata}); + const OwnedVisit copy(original); + std::vector relocated; + relocated.push_back(std::move(original)); + for (std::size_t index = 0; index < 100; ++index) { + relocated.emplace_back(copy); + } + for (const auto& entry : relocated) { + const Visit visit = entry.AsVisit(); + EXPECT_THAT(visit.path, EqualsText(path)); + EXPECT_THAT(visit.name, EqualsText(name)); + EXPECT_THAT(visit.root, EqualsText(root)); + } + // Moving intentionally leaves observable empty slices. + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_THAT(original.Path(), IsEmpty()); + EXPECT_THAT(original.Name(), IsEmpty()); + EXPECT_THAT(original.Root(), IsEmpty()); + EXPECT_THAT(original.LogicalTextBytes(), Eq(0)); + EXPECT_THAT(copy.Path(), EqualsText(path)); + } +} + +TEST_F(OwnedVisitTest, SnapshotOwnsMetadataAndRetainsFilesystemLifetime) { + auto owner = std::make_shared(); + const std::weak_ptr weak = owner; + const OwnedVisit owned( + Visit{ + .path = "./leaf", + .name = "leaf", + .root = ".", + .depth = 3, + .metadata = {.type = vfs::FileType::kRegular, .size = 42}, + .fs = *owner, + .fs_owner = owner, + .root_index = 7, + }); + owner.reset(); + EXPECT_THAT(weak.expired(), IsFalse()); + const Visit view = owned.AsVisit(); + EXPECT_THAT(view.depth, Eq(3)); + EXPECT_THAT(view.root_index, Eq(7)); + EXPECT_THAT(view.metadata.type, Eq(vfs::FileType::kRegular)); + EXPECT_THAT(view.metadata.size, Eq(42)); + ASSERT_THAT(static_cast(view.fs), IsTrue()); + EXPECT_THAT(*view.fs, Ref(*owned.fs)); + EXPECT_THAT(view.fs_owner, Eq(owned.fs_owner)); +} + +TEST_F(OwnedVisitTest, DefaultSnapshotAndMovedFromObserverAreEmpty) { + const OwnedVisit empty; + EXPECT_THAT(empty.Path(), IsEmpty()); + EXPECT_THAT(empty.Name(), IsEmpty()); + EXPECT_THAT(empty.Root(), IsEmpty()); + EXPECT_THAT(static_cast(empty.fs), IsFalse()); + const vfs::LocalFs fs; + OwnedVisit observed(Visit{.path = "x", .metadata = metadata, .fs = fs}); + const OwnedVisit moved(std::move(observed)); + EXPECT_THAT(moved.Path(), EqualsText("x")); + // The move contract explicitly disengages the old observer. + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_THAT(static_cast(observed.fs), IsFalse()); + EXPECT_THAT(observed.Path(), IsEmpty()); +} + +TEST_F(OwnedVisitTest, FilesystemOwnerSurvivesCopiesAndMovesUntilTheLastSnapshotDies) { + std::weak_ptr weak; + { + auto owner = std::make_shared(); + weak = owner; + OwnedVisit original(Visit{.path = "archive!member", .metadata = metadata, .fs = *owner, .fs_owner = owner}); + const OwnedVisit copy(original); + const OwnedVisit moved(std::move(original)); + owner.reset(); + EXPECT_THAT(weak.expired(), IsFalse()); + EXPECT_THAT(copy.Path(), EqualsText("archive!member")); + EXPECT_THAT(moved.Path(), EqualsText("archive!member")); + } + EXPECT_THAT(weak.expired(), IsTrue()); +} + +} // namespace +} // namespace xff::engine diff --git a/xff/engine/parallel_match.cc b/xff/engine/parallel_match.cc index e00c9c60581..88301050884 100644 --- a/xff/engine/parallel_match.cc +++ b/xff/engine/parallel_match.cc @@ -61,7 +61,7 @@ bool ParallelMatch::Finished() const { return remaining_ == 0; } -const std::vector& ParallelMatch::Match(std::vector entries) { +const std::vector& ParallelMatch::Match(std::vector entries) { const absl::MutexLock lock(mutex_); entries_ = std::move(entries); results_.clear(); diff --git a/xff/engine/parallel_match.h b/xff/engine/parallel_match.h index 7ccd913c985..7a67edc9e77 100644 --- a/xff/engine/parallel_match.h +++ b/xff/engine/parallel_match.h @@ -17,9 +17,9 @@ #include "absl/status/statusor.h" #include "absl/synchronization/mutex.h" #include "mbo/types/optional_ref.h" -#include "xff/engine/collect.h" #include "xff/engine/evaluate.h" #include "xff/engine/expression_execution.h" +#include "xff/engine/owned_visit.h" #include "xff/fuzzy/fuzzy.h" #include "xff/parser/ast.h" @@ -68,9 +68,9 @@ class ParallelMatch final { // Consumes a batch; entry filesystem observers must outlive this call. The // owning entries remain available for ordered output until the next call. - const std::vector& Match(std::vector entries); + const std::vector& Match(std::vector entries); - const std::vector& Entries() const { return entries_; } + const std::vector& Entries() const { return entries_; } private: void Run(); @@ -101,7 +101,7 @@ class ParallelMatch final { std::size_t generation_ ABSL_GUARDED_BY(mutex_) = 0; std::size_t remaining_ ABSL_GUARDED_BY(mutex_) = 0; // Published under mutex_, immutable until all workers have completed the batch. - std::vector entries_; + std::vector entries_; std::vector results_; std::atomic next_ = 0; }; diff --git a/xff/engine/parallel_match_test.cc b/xff/engine/parallel_match_test.cc index 9e270d590b2..be37a46797a 100644 --- a/xff/engine/parallel_match_test.cc +++ b/xff/engine/parallel_match_test.cc @@ -11,6 +11,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "mbo/testing/matchers.h" #include "mbo/testing/status.h" #include "xff/engine/evaluate.h" #include "xff/engine/expression_execution.h" @@ -21,6 +22,7 @@ namespace xff::engine { namespace { +using ::mbo::testing::EqualsText; using ::testing::Eq; using ::testing::IsFalse; using ::testing::IsTrue; @@ -30,15 +32,16 @@ struct ParallelMatchTest : ::testing::Test { // Type-only expressions never call this backend. No real paths are visited. const vfs::LocalFs fs; - std::vector Entries(std::size_t count) const { - std::vector entries; + std::vector Entries(std::size_t count) const { + std::vector entries; entries.reserve(count); for (std::size_t index = 0; index < count; ++index) { - entries.push_back({ - .path = std::to_string(index), - .metadata = {.type = index % 2 == 0 ? vfs::FileType::kRegular : vfs::FileType::kDirectory}, - .fs = fs, - }); + entries.emplace_back( + Visit{ + .path = std::to_string(index), + .metadata = {.type = index % 2 == 0 ? vfs::FileType::kRegular : vfs::FileType::kDirectory}, + .fs = fs, + }); } return entries; } @@ -55,7 +58,7 @@ TEST_F(ParallelMatchTest, RepeatedBatchesRetainEntryOrderAndDoNotReuseOldResults EXPECT_THAT(matcher.Entries(), SizeIs(count)); for (std::size_t index = 0; index < count; ++index) { EXPECT_THAT(results.at(index).evaluation.matched, Eq(index % 2 == 0)); - EXPECT_THAT(matcher.Entries().at(index).path, Eq(std::to_string(index))); + EXPECT_THAT(matcher.Entries().at(index).Path(), EqualsText(std::to_string(index))); } } } diff --git a/xff/engine/rg_test.cc b/xff/engine/rg_test.cc index 7a74ee20ace..a00e7fac88b 100644 --- a/xff/engine/rg_test.cc +++ b/xff/engine/rg_test.cc @@ -33,6 +33,7 @@ #include "gtest/gtest.h" #include "mbo/testing/matchers.h" #include "mbo/testing/status.h" +#include "xff/engine/owned_visit.h" #include "xff/engine/parallel_match.h" #include "xff/engine/run.h" #include "xff/parser/parser.h" @@ -442,14 +443,15 @@ TEST_F(RgEngineTest, MatcherPoolGrowsForLaterBatchesAndRunsShortTailsInline) { fs.files.emplace("tree/" + std::to_string(index), "hit\n"); } const auto entries = [&](std::size_t count) { - std::vector result; + std::vector result; result.reserve(count); for (std::size_t index = 0; index < count; ++index) { - result.push_back({ - .path = "tree/" + std::to_string(index), - .metadata = {.type = vfs::FileType::kRegular}, - .fs = fs, - }); + result.emplace_back( + Visit{ + .path = "tree/" + std::to_string(index), + .metadata = {.type = vfs::FileType::kRegular}, + .fs = fs, + }); } return result; }; diff --git a/xff/engine/run.cc b/xff/engine/run.cc index e8395e8a767..6c3389c8c3d 100644 --- a/xff/engine/run.cc +++ b/xff/engine/run.cc @@ -70,6 +70,7 @@ #include "xff/engine/evaluate.h" #include "xff/engine/extract.h" #include "xff/engine/mount.h" +#include "xff/engine/owned_visit.h" #include "xff/engine/parallel_compare.h" #include "xff/engine/parallel_match.h" #include "xff/engine/walk.h" @@ -1619,7 +1620,7 @@ bool ResultShapingIsValid( } struct DeferredCandidate { - CollectedEntry entry; + OwnedVisit entry; std::vector captures; std::map outputs; EvaluationMemo memo; @@ -1630,19 +1631,6 @@ struct DeferredCandidate { std::size_t order = 0; }; -CollectedEntry OwnVisit(const Visit& visit) { - return CollectedEntry{ - .path = std::string(visit.path), - .name = std::string(visit.name), - .root = std::string(visit.root), - .depth = visit.depth, - .metadata = visit.metadata, - .fs = visit.fs, - .fs_owner = visit.fs_owner, - .root_index = visit.root_index, - }; -} - void AppendDeferredNodes(const parser::Expr& expr, std::vector& nodes) { if (expr.kind == parser::Expr::Kind::kPredicate) { if (expr.descriptor->control == registry::Control::kTop @@ -1681,7 +1669,7 @@ using DeferredCandidateRefs = std::vector IndexCandidatesByDirectory(const DeferredCandidateRefs& entries) { std::map result; for (std::size_t index = 0; index < entries.size(); ++index) { - const std::string_view path = entries[index].get().entry.path; + const std::string_view path = entries[index].get().entry.Path(); const std::string_view::size_type slash = path.rfind('/'); const std::string_view directory = slash == std::string_view::npos ? std::string_view() : path.substr(0, slash); result[std::string(directory)].push_back(index); @@ -1706,14 +1694,15 @@ ShardStatusCohort MakeShardStatusCohort( if (candidate.entry.metadata.type != vfs::FileType::kRegular) { continue; } - const std::optional decoded = shard_matcher.Decode(candidate.entry.name); + const std::optional decoded = shard_matcher.Decode(candidate.entry.Name()); if (!decoded.has_value() || !scheme_allowed(decoded->scheme)) { continue; } - CandidateIndexes& visits = cohort.by_name[candidate.entry.name]; + const std::string_view name = candidate.entry.Name(); + CandidateIndexes& visits = cohort.by_name[name]; if (visits.empty()) { cohort.files.push_back({ - .name = candidate.entry.name, + .name = name, .size = candidate.entry.metadata.size, .mode = candidate.entry.metadata.mode, .mtime = absl::ToUnixNanos(candidate.entry.metadata.mtime), @@ -6061,7 +6050,7 @@ RunResult RunFindCore( parallel_expression, options.workers, full_metadata || rank_by_score, std::move(content_output), executions->Parallel()); } - std::vector pending_matches; + std::vector pending_matches; // Decision-only batches retain no file bytes or rendered line records. Give them more work // between coordinator barriers; keep content-output batches smaller because a line is unbounded. const std::size_t match_batch_size = rg_output || match_output ? 256 : 1'024; @@ -6180,7 +6169,7 @@ RunResult RunFindCore( return WalkAction::kContinue; } if (parallel_entry) { - pending_matches.push_back(OwnVisit(visit)); + pending_matches.emplace_back(visit); if (pending_matches.size() >= match_batch_size) { flush_matches(); } @@ -6290,7 +6279,7 @@ RunResult RunFindCore( } if (evaluated.deferred) { deferred_candidates.push_back({ - .entry = OwnVisit(visit), + .entry = OwnedVisit(visit), .captures = std::move(captures), .outputs = std::move(outputs), .memo = std::move(evaluation_memo),