Fix reference leaks in Automaton pickling (leaked whole dump per pickle.dumps)#220
Open
vmax wants to merge 1 commit into
Open
Fix reference leaks in Automaton pickling (leaked whole dump per pickle.dumps)#220vmax wants to merge 1 commit into
vmax wants to merge 1 commit into
Conversation
Two missing Py_DECREFs in the pickling path leaked roughly the full serialized size of the automaton on every successful pickle.dumps(): 1. pickle_data__add_next_buffer(): PyList_Append() takes its own reference to the chunk, but the reference owned after PyBytes_FromStringAndSize() was never released, so every chunk buffer (the entire node dump) stayed alive forever. 2. automaton___reduce__(): Py_BuildValue with the "O" format takes new references to bytes_list and values, but the function returned without releasing its own references; pickle_data__cleanup() was only ever called on the error path. This leaked the list objects and, combined with (1), pinned all chunk buffers. Repro: pickling a 3000-word automaton in a loop leaked ~1.7 MiB per iteration (1 GiB RSS after 600 dumps). With this change RSS stays flat. Unpickling was unaffected. Existing tests pass (149 passed, 8 skipped).
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.
Fixes #219.
Two missing
Py_DECREFs leaked approximately the full serialized size of the automaton on every successfulpickle.dumps()(~1.7 MiB per call for a 3000-word automaton; see the issue for a standalone repro).src/pickle/pickle_data.c—pickle_data__add_next_buffer()PyList_Append()takes its own reference to the chunk; the reference owned afterPyBytes_FromStringAndSize()was only released on the append-failure path. Now it is also released after a successful append — the list keeps the chunk alive. This was the bulk of the leak (the chunk buffers hold the entire node dump).src/Automaton_pickle.c—automaton___reduce__()Py_BuildValuewith the"O"format takes new references tobytes_listandvalues, but the success path returned without releasing the function's own references (pickle_data__cleanup()was only reachable via the error label). Now cleanup runs on the success path too. Thedata.values = NULLspecial-case forPy_Noneis gone: the ownedPy_Nonereference is released by cleanup like any other value.Verification:
dumps/loadsproduces identicaliter()results, including the chunked multi-buffer path (tested with a 235 MiB pickle).pytest tests/: 149 passed, 8 skipped.