Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: implement RevIndexOps for mem_revindex, and implement generic CounterGather struct in Rust #3584

Merged
merged 44 commits into from
Mar 24, 2025

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Mar 21, 2025

NOTE: This is a PR into #3545

This PR mimics the Python layer by having RocksDB databases return a CounterGather object directly; doing this responsibly meant fully implementing RevIndexOps for RevIndex in memory, as well as fleshing out the whole Python FFI layer 🎉

Current benchmarks:

prefix s max_rss
fastmultigather_rocksdb 21.5 0.5
pygather_rocksdb+prefetch 101.1 0.9
fastgather 199.8 12.0
fastmultigather 910.9 12.8
pygather 1752.3 20.0
pygather_rocksdb-no-prefetch 3528.9 1.1

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 72.22222% with 220 lines in your changes missing coverage. Please review.

Project coverage is 87.59%. Comparing base (a1565b0) to head (d8c34f4).
Report is 47 commits behind head on impl_revindex.

Files with missing lines Patch % Lines
src/core/src/ffi/index/revindex.rs 0.00% 114 Missing ⚠️
src/core/src/index/revindex/mem_revindex.rs 89.52% 35 Missing ⚠️
src/core/src/index/revindex/mod.rs 61.84% 29 Missing ⚠️
src/core/src/index/revindex/disk_revindex.rs 60.71% 22 Missing ⚠️
src/sourmash/index/revindex.py 88.88% 14 Missing and 6 partials ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           impl_revindex    #3584      +/-   ##
=================================================
- Coverage          87.68%   87.59%   -0.10%     
=================================================
  Files                137      136       -1     
  Lines              22717    23043     +326     
  Branches            2280     2273       -7     
=================================================
+ Hits               19919    20184     +265     
- Misses              2473     2530      +57     
- Partials             325      329       +4     
Flag Coverage Δ
hypothesis-py 25.38% <30.48%> (+0.07%) ⬆️
python 92.36% <89.30%> (+0.01%) ⬆️
rust 81.10% <66.94%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…factor FFI (#3585)

NOTE: PR into #3584

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@ctb ctb changed the title WIP: return a RevIndex directly from DiskRevIndex WIP: implement RevIndexOps for mem_revindex, and implement generic CounterGather struct in Rust Mar 24, 2025
@ctb ctb merged commit 1b6801a into impl_revindex Mar 24, 2025
40 of 41 checks passed
@ctb ctb deleted the impl_revindex_cg branch March 24, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant