Replace the hard-coded oversized-IndexedDB skip with a measured per-store cap - #348
Open
dchaudhari7177 wants to merge 1 commit into
Open
Replace the hard-coded oversized-IndexedDB skip with a measured per-store cap#348dchaudhari7177 wants to merge 1 commit into
dchaudhari7177 wants to merge 1 commit into
Conversation
…tore cap `get_indexeddb` skipped one specific extension's `.leveldb` by literal directory name, because that store has over a million records. The protection was therefore arbitrary: it covered exactly one known-large store, and any other large store still parsed fully, with the same possible hang or memory blow-up. The literal-ID match is gone. Iteration now carries a per-store record budget, and a store that exhausts it is truncated and reported. Three choices behind that, all reversible: - Measured on record count during iteration rather than directory size on disk. Record volume is the failure mode named in the issue, and counting during the walk needs no second pass and no estimate that a compacted or blob-heavy store would throw off. - Truncate with a note rather than a hard skip. This is strictly more than the rule it replaces: the store that motivated the original skip yielded nothing, and now yields its first records. A threshold set too low then costs completeness, which is reported, rather than correctness. - Configurable. `INDEXEDDB_MAX_RECORDS_PER_STORE` is a module constant, read once into an instance attribute so a caller with a known-good corpus can raise it, or set it to None to disable the cap, without patching the module. The cap is checked before the record is built, so it bounds the work rather than only the rows kept: iterating a million-record store is the hang, and resolving each record's blob references is the memory. The budget is per store directory, not per run, so one huge store cannot eat the allowance of the ones after it. Truncation is reported through `unparsed.source`, the same channel the hard-coded skip used, so a capped store is as visible in the run's totals as an unreadable one. Five tests, three failing on main.
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
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 #340.
The literal-ID match on the Ghostery store is gone. Iteration carries a per-store record budget instead, and a store that exhausts it is truncated and reported through
unparsed.source.The three decisions
The issue names them explicitly, so here is what I chose and why. All three are cheap to reverse if you would rather have it another way.
1. Metric — record count during iteration, not directory size on disk.
Record volume is the failure mode the issue describes, and it is the thing that actually costs time and memory. Counting during the walk needs no second pass, and no estimate that a freshly-compacted store (small on disk, many records) or a blob-heavy one (large on disk, few records) would throw off in opposite directions. The cap is checked before the record is built, so it bounds the work and not merely the rows kept — iterating a million-record store is the hang, and
resolve_indexeddb_blob_refson each record is the memory.2. Behaviour — truncate with a note, not a hard skip.
This is the one I feel strongest about, because it makes the threshold much less load-bearing. It is strictly more than the rule it replaces: the Ghostery store currently yields zero records and would now yield the first 100,000. A threshold set too low costs completeness — which is reported — rather than correctness. A hard skip has no such margin.
3. Configurable — module constant, read into an instance attribute.
INDEXEDDB_MAX_RECORDS_PER_STORE = 100_000, alongsideSECONDARY_CACHE_DIRS. It is read once intoself.indexeddb_max_records_per_store, so a caller with a known-good corpus can raise it, or set it toNoneto disable the cap entirely, without patching the module. I did not wire it to a CLI flag — that felt like it wanted your call on the surface, and the attribute is enough for a programmatic caller.On the default of 100,000: this is the number I am least able to defend from measurement, and I want to be straight about that — I do not have the corpus datasets, so I have not measured the record-count distribution of ordinary stores against it. The reasoning is only that it is an order of magnitude below the 1M+ that motivated the original skip and far above any store I would expect to be ordinary. If you have the distribution to hand, it should probably be set from that rather than from my estimate. The truncate-not-skip choice above is what makes an imperfect default survivable.
The budget is per store directory, not per run, so one huge store cannot eat the allowance of the ones after it.
Verification
Five tests driving
get_indexeddbagainst a stubbedWrappedIndexDB. Three fail onmain:The other two pin that a store under the cap is still read whole and that a
Nonecap disables truncation. The iteration-bound test asserts against the stub's owniteratedcounter, so it fails if the cap only filters rows rather than stopping the walk.Full suite on this branch: 244 passed, 2 skipped, 80 subtests passed. Python 3.12.4.
Not done here
The issue also floats a streamed partial parse. What this does is the simple form of that — first N records plus a truncation note — rather than anything resumable. If you want a store to be readable in pages across runs, that wants a cursor in the output and is a bigger change than this issue.