-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix memory leak with ujson module #49466
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
Conversation
@JelleZijlstra if you have the ability to confirm this patch fixes your issue would be appreciated |
As an aside, has Pandas considered https://github.com/ijl/orjson (a Rust-based JSON (de)serializer)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @WillAyd. Do you think it's worth adding and ASV peakmem_*
benchmarks or are we sufficiently covered?
Its a pretty small memory leak. I don't think it would even register on a peakmem ASV. Let's see if it helps @JelleZijlstra and can go from there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I can confirm this removes the leak. I left a few comments on the code.
@mroeschke this memleak only appears when pandas is unimported from a running process. It's unlikely to affect many current use cases.
Looks good @WillAyd. Would be good to have a whatsnew note for this then LGTM |
Thanks @WillAyd |
* Fix memory leak with ujson module * fixups * Whatsnew
* Fix memory leak with ujson module * fixups * Whatsnew
This patch may have induced a potential regression. Please check the links below. If any ASVs are parameterized, the combinations of parameters that a regression has been detected appear as subbullets. This is a partially automated message.
|
This is mostly vendored from ujson upstream
https://github.com/ultrajson/ultrajson/blob/main/python/ujson.c
Using a static PyObject the way we did before is strongly discouraged. This is more verbose but should be more correct, although interestingly pushes us back to the legacy module initialization instead of PEP 489 multi phase initialization