gh-155901: Fix exponential time when marshalling nested sets - #157128
Open
iamsharduld wants to merge 1 commit into
Open
gh-155901: Fix exponential time when marshalling nested sets#157128iamsharduld wants to merge 1 commit into
iamsharduld wants to merge 1 commit into
Conversation
marshal.dumps() sorts the elements of a set by their marshalled form, computed by marshalling each element on its own. When an element is itself a set, that repeats the sort of the nested set, so the time doubled with every level of nesting. Cache the order of each set for the duration of the top-level call and share the cache, and the nesting depth, with the nested writer used for the sort keys. The output is unchanged.
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.
marshal.dumps()sorts the elements of a set by their marshalled form, so that the output does not depend on hash randomization (bpo-37596). The sort key of each element was computed with a nested_PyMarshal_WriteObjectToString()call. For an element that is itself a set, that call sorts the nested set again, and then the element is marshalled a second time to write it out. So the time doubled with every level of nesting: depth 20 takes 0.5 s on a debug build here, and each further level doubles it.Fix
The order of each set is now computed once per top-level call and cached in the writer state (
WFILE.set_orders, a pointer-keyed_Py_hashtable_tthat maps a set to a tuple of its elements in marshalling order). The nested writer that computes the sort keys (w_set_key()) shares that cache and the nesting depth with its parent, and still has its own reference table, so a key stays independent of where the element appears in the stream.The cache holds borrowed references to the sets, which are kept alive by the object being marshalled. An owned reference would change the result of the
_PyObject_IsUniquelyReferenced()check inw_ref()for a set that is referenced once, and addFLAG_REFto the output.For a chain of nested sets the time is now quadratic in the depth (the sort key of a nested set is still its full marshalled form), and the depth is bounded by
MAX_MARSHAL_STACK_DEPTH. Depth 1000 takes 47 ms on the debug build; the reproducer from the issue is instant at any depth.Output is unchanged
I compared
marshal.dumps()output between the unfixed and fixed interpreters, with the samePYTHONHASHSEED, for format versions 0 to 6 over 139 objects: plain, nested and shared sets, interned and non-interned strings, a code object with set constants, and the code objects of 120 stdlib modules. All 973 outputs are byte-for-byte identical.Other visible effects
ValueError: object too deeply nested to marshalpromptly instead of never finishing. The write path already enforced the limit.marshal.dumpsaudit event is raised once per call. It was also raised for every set element, because the sort keys went through_PyMarshal_WriteObjectToString().Tests
test_nested_sets: round-trips a chain of 100 nested frozensets, and a shallow structure with sets shared between elements, for every format version. It hangs before the fix.test_recursion_limit: extended with a chain ofMAX_MARSHAL_STACK_DEPTH - 1frozensets that round-trips, plus one more level that raisesValueError.test_deterministic_sets: added a nested-set sample, to check that the cached order does not depend on the hash seed.Ran
test_marshal(also with-R 3:3),test_audit,test_compileall,test_py_compile,test_zipimport,test_importlib,test_code, the full suite with-j0, andmake patchcheckon a macOS debug build.