Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (some?) recursive objects by disabling memo ref counting #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

landaire
Copy link

@landaire landaire commented Jun 3, 2023

While comparing this implementation to the CPython impl I noticed that CPython does not perform ref counting of objects in its internal memo list, but this crate does. I believe that somewhere there's a bug in the ref counting logic (or maybe it's not even valid to ref count these?) that causes an object to be dropped to early from the memo list and cause errors upon subsequent lookups.

This PR adds an internal flag to just disable ref counting until this can be investigated further. I've tested this PR on data which previously returned a recursion error and it now works with correct results.

Related: #22.

@birkenfeld
Copy link
Owner

Sorry for coming back to this only now...

I'm not surprised that the ad-hoc refcounting I added is imperfect; it is added here purely for performance, since without it we would have to clone basically any object being unpickled, which ends up having a prohibitive cost. CPython doesn't have to do this, since its objects are all intrinsically refcounted.

The alternative is to do it like CPython - wrap all unpickled objects in an Rc like type. It could be restricted to composite types, but even large strings might benefit. But this is a huge API change, and makes handling the returned data much more fiddly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants