tisonkun commented on PR #306: URL: https://github.com/apache/datasketches-rust/pull/306#issuecomment-6435124382
Local ARM64 follow-up for `2f2cac2` (after inlining `insert_bits`). These results supplement the Xeon measurements in the PR description; the host and harness differ, so they should not be treated as an exact reproduction. ### Method - Apple M4 Max, macOS ARM64, Rust 1.99.1, release defaults. Each revision was built as a separate binary from the same harness. The measurement thread requested user-initiated QoS; there was no CPU pinning or isolated-host guarantee. - Revisions: baseline `675e3e1`, before inlining `7eb0d2d`, after inlining `2f2cac2`. - Each sample executes the same fixed workload on all three revisions. The baseline is calibrated near 300 ms; seven rounds rotate the execution order. Each process warms up before timing. Reported times are measured whole-batch medians, not scaled request latencies. - Ordinary filters have 191,744 bits. Fresh cases construct and drop one filter per 20,000 items inside timing. Repeated cases use prepopulated fixtures; saturated cases use a fully populated 64-bit filter. The 32-byte inputs encode the item number in the first eight bytes and zero-fill the rest. Fixture and byte-input preparation are outside timing. - Targeted serialization/membership checks agreed across all three revisions. No library code changes are included in the test-only follow-up described below. ### Results `Base / inlined` is the median of seven paired time ratios: above 1 is faster, below 1 is slower. It can differ slightly from the ratio of the displayed median times. Counts are inserted items for fresh cases and calls for other cases. | Workload | Hashes | Items/calls per sample | Baseline | Before inlining | After inlining | Base / inlined | | --- | ---: | ---: | ---: | ---: | ---: | ---: | | Fresh `u64` insertion | 1 | 27,380,000 | 303.01 ms | 291.51 ms | 290.68 ms | 1.046x | | Fresh `u64` insertion | 7 | 7,300,000 | 302.21 ms | 123.07 ms | 121.32 ms | 2.486x | | Fresh `u64` insertion | 12 | 4,620,000 | 300.38 ms | 99.89 ms | 98.22 ms | 3.078x | | Fresh 32-byte insertion | 1 | 22,980,000 | 301.19 ms | 354.07 ms | 352.57 ms | 0.850x | | Fresh 32-byte insertion | 7 | 6,220,000 | 305.44 ms | 154.39 ms | 154.46 ms | 1.960x | | Fresh 32-byte insertion | 12 | 4,080,000 | 297.06 ms | 134.15 ms | 133.88 ms | 2.225x | | Repeated `insert` | 1 | 32,180,000 | 310.44 ms | 293.52 ms | 297.00 ms | 1.059x | | Repeated `insert` | 7 | 20,080,000 | 305.27 ms | 337.79 ms | 333.01 ms | 0.915x | | Repeated `insert` | 12 | 14,220,000 | 301.42 ms | 308.08 ms | 304.59 ms | 0.985x | | Saturated-filter `insert` | 1 | 31,100,000 | 293.18 ms | 278.71 ms | 278.61 ms | 1.047x | | Saturated-filter `insert` | 7 | 20,020,000 | 303.77 ms | 337.50 ms | 326.09 ms | 0.931x | | Saturated-filter `insert` | 12 | 15,060,000 | 306.75 ms | 324.24 ms | 314.32 ms | 0.974x | | Repeated `contains_and_insert` | 1 | 28,600,000 | 303.69 ms | 286.26 ms | 292.09 ms | 1.040x | | Repeated `contains_and_insert` | 7 | 19,740,000 | 310.28 ms | 253.18 ms | 253.91 ms | 1.223x | | Repeated `contains_and_insert` | 12 | 13,860,000 | 302.40 ms | 251.74 ms | 249.67 ms | 1.211x | | Membership hits | 7 | 19,320,000 | 308.33 ms | 303.92 ms | 300.54 ms | 1.012x | | Membership misses | 7 | 10,720,000 | 298.40 ms | 289.82 ms | 296.94 ms | 1.006x | Across these 17 workloads, the largest median slowdown introduced by helper inlining was 1.65%. This does not establish that inlining is performance-neutral in every build or environment. Three baseline-relative regressions exceeded 5%. Each was confirmed in a separate nine-pair run, alternating baseline and inlined binaries with the baseline workload near 500 ms: | Workload | Median time increase | Pair range | | --- | ---: | ---: | | Fresh 32-byte insertion (1 hashes) | +17.96% | +15.59% to +26.71% | | Repeated `insert` (7 hashes) | +8.64% | +7.94% to +10.06% | | Saturated-filter `insert` (7 hashes) | +8.22% | +6.79% to +9.87% | ### Assessment I would accept this as an explicitly scoped optimization for fresh multi-hash filter construction, while acknowledging the platform-specific regressions. It is not a universal net win, and it does not satisfy a requirement that every measured workload stay within 5% of baseline on this ARM64 host. The substantial construction gains, improved repeated membership-and-insert path, and nearly unchanged membership queries support the intended optimization direction. API choice should follow the caller's needs: use `insert` when only recording items, `contains` for read-only membership queries, and `contains_and_insert` when the caller needs prior membership as well as insertion. The conditional writes retained by `contains_and_insert` suit repeated items. However, repeated `insert` without a membership result remains legitimate usage; its regressions cannot be dismissed as misuse. In particular, single-hash insertion already delegates to `contains_and_insert`, so choosing a different interface does not explain away the 32-byte single-hash regression. ### Test cleanup and validation The test-only follow-up `7673ce8cbca750345727d027baaa99d486208f9b` splits insertion-path equivalence from collision/repeated-insertion accounting. It removes the four-pass input cycle and documents why 65 hashes on 64 bits force within-item collisions. The accounting test covers both public insertion methods and independently validates the stored count through deserialization. The wider Bloom test review merged duplicate rounded-capacity/allocation setup, replaced repeated partial round-trip checks with full-filter equality, and removed redundant emptiness/count assertions and a second membership sample after round-trip. Java/C++/Go fixture matrices, set-operation contracts, and malformed-input checks remain covered. Validation: `cargo x check`, `cargo x test` (777 passed), and full `cargo x lint` passed. Lint ran on a clean source copy with the same changes because existing generated `target/package` manifests in the checkout fail Taplo formatting. Fault injection in a temporary copy confirmed that the revised accounting test rejects both counting every bit visit and counting each newly set bit twice. No mutation was kept. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
