Conversation
…iary#145) _pichash and _picblockhashes[].hash hold 64 bit values as variable-width hex strings, so a range condition on pichash answered a plausible but wrong set and a pichash-sorted function search could not page (the cursor compares the sort field with a range operator). Write the values 16 digits wide and string order is numeric order. - settings flag pichash_padded: set on fresh instances, reported through getStats/getStatus; the storage reads it once per process - width-aware _encodePichash; the transpiler allows range operators and the storage allows sorting by pichash (mapped to _pichash) only when the instance is padded, and rejects both with a message naming the migration otherwise - on an unpadded instance every lookup by value (search conditions, isPicHash, getMatchesForPicHash, getMatchesForPicBlockHash) matches both widths, so a migration in flight misses nothing - mcrit/migrations/migrate_pichash_padding.py: resumable in-place pad/unpad walks over functions and query_functions with a leftover sweep, a verify mode that flags unpadded values under a set flag - tests: transpiler on both widths, fresh instance, legacy lookups, mixed widths, numeric range answers, pichash-sorted paging forward and backward, migration round trip and CLI Refs familiary#145
…hs in the aggregation, bound the range values - migrate_pichash_padding builds its URI from STORAGE_MONGODB_USERNAME, _PASSWORD and _FLAGS like MongoDbStorage does, or takes --uri - getPicHashMatchesByFunctionId(s) match either spelling of a value while the instance is not padded, so equal pichashes in opposite widths land in one group during a migration - a pichash bound outside 0..0xffffffffffffffff is rejected: it would format wider than 16 digits and not compare
Collaborator
Author
|
Live migration of a running MongoDB 7 instance (4 samples, 1158 functions) that was created before this change: Before (legacy shape): Migration: server and worker stopped, After: |
One conflict, in _ensureIndexAndUnknownFamily, where both sides added to the same fresh-database branch. They are independent and both are kept, at the nesting each belongs to: this branch's `pichash_padded: True` goes into the settings document that is inserted when there is no settings collection yet, and main's picblockhash-index-completeness check stays outside that `if`, so it still runs for an existing database as well as a fresh one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011EAW1DRkBwmjZtGzQ5pgDA
r0ny123
pushed a commit
to r0ny123/mcrit
that referenced
this pull request
Sep 13, 2026
…amiliary#69 is about Upstream has 17 open PRs and 15 open issues. One of them is this problem: familiary#69 "Workers consume a lot of ram on query" - open, unlabelled, unassigned, no linked PR. Reported against a 20 million function instance, with workers reaching tens of GB and sometimes over 60 GB, one worker starving the others and crashing multi-worker deployments. That is the same defect as the latency problem seen from the memory side, and this project already measured why: docs/TUNING.md records peak RSS correlating 0.98 with bytes fetched from MongoDB and only 0.62 with the sample's function count. Bytes fetched is candidate volume, and candidate volume grows with the corpus. What exists for familiary#69 today is mitigation rather than a fix - MINHASH_MATCHING_MAX_PAIRS caps pairs held at once, its own comment calling the default "a guard against runaway jobs (familiary#69-class)" - and none of it reduces how many candidates a query produces; it spreads the same work over more batches. The shortlist bounds the candidates themselves, so it should bound the peak, which makes this work a candidate answer to familiary#69 and not only to the latency question. That claim needs a number, so the harness now records peak RSS per query alongside the stage timings, noting in the code that ru_maxrss is a process-wide high-water mark rather than the query's own allocation. docs/scaling/UPSTREAM-REVIEW.md records the rest: every other open upstream PR is authored by this fork's owner and pairs with one of the fork's own, and none touch the matching path's scaling behaviour - client surface, job lifecycle, storage schema, metadata, release. It also records the one file with real overlap (MongoDbStorage, where familiary#181 changes how pichashes are stored and this work changes how they are counted - compatible) so the conflict question is answered rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CkCmcpjEzUwYBroCEhrhVo
# Conflicts: # mcrit/storage/MongoDbStorage.py
On an unpadded instance a value may be stored in both spellings, and pichash_counts counts each one separately, so a value over MINHASH_PICHASH_MAX_MATCHES could slip through. Sum the counts over the spellings of a value and keep or drop them as a unit. migrate_pichash_padding rewrites _pichash but left pichash_counts keyed on the old spelling with its complete flag set, so with the cutoff on every PicHash match was dropped after migrating. pad and unpad now clear pichash_counts and mark it incomplete; the cutoff falls back to counting (with a warning) until rebuildPicHashCountIndex() runs again.
Conflicts: - CHANGELOG.md: main's 1.10.0 and 1.11.0 sections are kept as released; this branch's entry moves under [Unreleased]. - MongoDbStorage: MongoSearchTranspiler takes both main's known_values and this branch's pichash_padded, and _get_search_query passes both. The partitioned pichash count rebuild main added pages over the stored _pichash strings, so it counts each stored spelling as its own run; the cutoff already sums the spellings of a value, so only its docstring needed correcting.
The inverted picblockhash index behind getUniqueBlocks is keyed on the stored hex of each block hash, and migrate_pichash_padding rewrote the functions without touching it. It stayed marked complete, so after a pad every candidate with a leading zero missed its index document and was reported unique: on the test data, 15 blocks unique to a sample that shares all of its blocks. pad and unpad now mark both indexes keyed on the stored spelling (pichash_counts and picblockhashes) incomplete and drop them, flag first. getUniqueBlocks then falls back to scanning the functions and the write paths stop maintaining the index until rebuildPicBlockHashIndex runs. verify reports a derived index that is marked complete but keyed on the other width. getUniqueBlocks on an instance that is not yet padded normalises block hashes to the hex() spelling and asks the index for both widths, so an interrupted pad, which leaves both spellings behind, no longer reports shared blocks as unique. The new tests write one database the old, unpadded way and one fresh, migrate the old one, and compare unique blocks, pichash lookups and cutoffs, block hash lookups and searches between them, before and after rebuilding the derived indexes; plus an interrupted pad, a rollback, and verify.
migrate_pichash_padding dropped pichash_counts and the picblockhash index on every run, including a repeated pad - which verify advises for leftovers - and pad on an instance padded from the start, throwing a correct index away and sending getUniqueBlocks to the full scan until someone rebuilt it. It now invalidates them only when a walk rewrote something (counted across an interrupted run) or the flag changes. The CHANGELOG folds the cutoff note into the entry for the padding, since both are unreleased.
Collaborator
Author
|
Merged |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #145.
_pichashand_picblockhashes[].hashhold 64 bit values as variable-width hex strings (hex()), sopichash:<0x99answered a plausible but wrong set and a pichash-sorted function search could not page: the search cursor compares the sort field with a range operator, which the transpiler rejected for exactly that reason. Written 16 digits wide, string order is numeric order.What changes
settings.pichash_padded. Fresh instances get it at creation; it is reported throughgetStats()and/status. The storage reads it once per process._encodePichash(..., padded=);_encodeFunctionandrecalculateAllPicHasheswrite in the instance's width. Import goes throughFunctionEntryintegers, so it follows the instance too._pichash); an unpadded one rejects both with a message naming the migration, instead of answering a wrong set.isPicHash,getMatchesForPicHashandgetMatchesForPicBlockHashmatch both encodings ($inon the indexed field), so a migration in flight misses nothing. On a padded instance the point lookups are single-value again.python -m mcrit.migrations.migrate_pichash_padding --mode pad|verify|unpad: resumable keyset walk overfunctionsandquery_functionswith positional$setper changed value, a leftover sweep for documents a still-running server wrote below the cursor, then the flag.verifycounts unpadded values and exits 1 when the flag says padded but some remain (a 16-digit value is legitimate under either encoding, so only that direction is a problem).unpadrolls back.Operationally: stop server and workers, run
pad, restart. Range/sort by pichash on an unmigrated instance keep failing the same way as before, just with a clearer message; everything else behaves as it did.Verification
tests/testPichashPadding.py(MongoDB): fresh instance is padded; lookups by value on both shapes; a half-migrated instance misses nothing and still refuses range/sort; range conditions answer the numeric set (pichash:<0x99is empty); a pichash-sorted function search pages forward and backward in numeric order; migration round trip, idempotent re-run, leftover detection and CLI exit codes.tests/testMongoSearchTranspiler.pycovers both widths and the sort helpers.