Stop re-pickling arguments that can never be pickled - #26
Merged
Conversation
Recording sqlparse spent 5.2s of a 10s run on pickle attempts that raised PicklingError, 38,627 of them, for two functions that take a lambda on every call: TokenList._token_matching and grouping._group. The pickler walks the whole argument graph before reaching the lambda, so the full serialisation cost is paid and then discarded. Duplicates back off and oversized inputs get abandoned, but a failure did neither -- it incremented a counter and returned, so the next call tried again, for the whole run. Failures now abandon the target the way oversized inputs do, with a more generous threshold (16 vs 3): a failure is usually evidence about the signature rather than the input, but a function taking a callback on only some paths deserves rope. Being generous is free, since a signature that cannot be pickled fails thousands of times. sqlparse: 8.66s -> 4.02s, 16,321 distinct inputs unchanged. The four newly abandoned functions had zero recorded inputs before, so nothing is lost -- they were silent gaps and are now declared ones.
The key was digest(["blob", len(blob), _cheap_hash(blob)]): a blake2b pass over every blob, hex-encoded, JSON-serialised, then hashed again. The bytes are already the identity of the input and are already in hand, so they are the key. Exact rather than a digest of a digest, and one fewer stored string per record.
Abandonment used to be rare enough that returning without touching a counter did not show up. It is now the common reason a call is never looked at, and an uncounted early return makes the run summary stop adding up.
_cheap_hash and the digest import became dead when the pickled bytes became the key. (fingerprint was already imported unused.)
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.
Recording
sqlparsetook 16.0s against a 1.1s test suite. Profiling by stubbing out each layer showed the wrapper dispatch costs essentially nothing across 1.42M intercepted calls — the time is pickling, and more than half of it was pickling that failed.38,627
pickle.dumpscalls raisedPicklingErrorand were thrown away. Two functions caused nearly all of them, both taking a lambda on every call:TokenList._token_matchingandengine.grouping:_group. The pickler walks the entire TokenList before reaching the lambda, so it pays almost the full serialisation cost and then raises.Duplicates back off and oversized inputs get abandoned, but a failure did neither — it counted the failure and returned, remembering nothing, so every later call retried for the whole run.
Targets whose arguments repeatedly fail to pickle are now abandoned, mirroring the existing oversized logic but with a much more generous threshold (16 against 3): a failure is usually evidence about the signature rather than the input, but not always, and a signature that genuinely cannot be pickled trips 16 immediately anyway.
Measured on sqlparse, interleaved A/B, three pairs
Coverage is unchanged, not approximately unchanged: 16,321 distinct inputs across 141 targets before and after, with per-target counts identical and no target losing a single input.
abandonedgrows 38 → 42. All four additions had zero recorded inputs before, so they were silent gaps thatcheckimplicitly papered over; they are now declared. That is a net honesty gain, and it is why a threshold of 3 was rejected — it cost 66 real inputs from functions that take a callback on only some paths.nodrift check HEADon unchangedsqlparsestill reports no behaviour change across all 16,321 inputs, so the speedup buys no false positives.sys.monitoringwas deliberately not used. It would replace the interception mechanism, which the profile shows costs about 1% of the overhead — the wrong lever, and 3.12+ only.Also included: the dedup table is keyed on the pickled bytes rather than a digest of a digest, and calls skipped because their target was abandoned are counted so the run summary still adds up.
Closes #5