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

Same memory leaks. Now tested under python 2. #270

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@Adriandorr
Copy link

commented Jun 19, 2017

This contains a fixes for a number of memory leaks, mainly related to dictionary keys and values.

borman and others added some commits Mar 23, 2017

Release saved raw JSON string.
Using ujson.dumps() with objects having __json__() method leaked memory
for object's JSON representation.
Do not discard result of PyObject_CallObject()
PyObject_CallObject() returns a PyObject*; discarding it leaked
memory for the result of output.write().
Merge pull request #1 from borman/master
Merged pull request to fix memory leak.
@qsniyg

This comment has been minimized.

Copy link

commented Nov 2, 2017

I can confirm this works, hope it'll get pulled upstream.

if (GET_TC(tc)->itemName) {
Py_DECREF(GET_TC(tc)->itemName);
GET_TC(tc)->itemName = NULL;
}

This comment has been minimized.

Copy link
@orivej

orivej Dec 26, 2017

Contributor

Accidental extra indentation here.

This comment has been minimized.

Copy link
@jayvdb

jayvdb May 11, 2019

ping @Adriandorr , this looks like the last remaining problem..?

@@ -818,6 +817,19 @@ def test_sortKeys(self):
sortedKeys = ujson.dumps(data, sort_keys=True)
self.assertEqual(sortedKeys, '{"a":1,"b":1,"c":1,"d":1,"e":1,"f":1}')

def test_ujson_has_no_memory_leak(self):
import gc
import objgraph

This comment has been minimized.

Copy link
@orivej

orivej Dec 26, 2017

Contributor

The new dependency should be added to .travis.yml.

@daicang

This comment has been minimized.

Copy link

commented Dec 27, 2017

Nice work. This pr fixes memory leaks when dumps() dict with string/unicode/other container-type key & value. And seems it also fixes memory leak when dump to files.

Some examples I can confirm:
dumps() with:

{string: 1} string key
{unicode: 1} unicode key
{1: [1,]} list value
{1: {1:1}} dict value
{1: string} string value
{1: unicode} unicode value

danmaas added a commit to spinpunch/ultrajson that referenced this pull request Jan 6, 2018

@danmaas

This comment has been minimized.

Copy link

commented Jan 6, 2018

Thank you, this is a helpful fix. Hope it can be merged soon!

@mxr

This comment has been minimized.

Copy link

commented May 10, 2018

@Jahaja @orivej have you had a chance to take a look at this PR? Thanks!

@nav-agarwal

This comment has been minimized.

Copy link

commented Apr 24, 2019

@Adriandorr I am not able to reproduce these memory leaks with ujson(1.35) and python(2.7.14). I have used tests added by you in this fix. Reference count before and after was same.

Here is the script that I used:

import ujson
import sys

def test_does_not_leak_dictionary_values():
    import gc
    gc.collect()
    value = ["abc"]
    data = {"1": value}
    ref_count = sys.getrefcount(value)
    ujson.dumps(data)
    print(ref_count, sys.getrefcount(value))

def test_does_not_leak_dictionary_keys():
    import gc
    gc.collect()
    key1 = "1"
    key2 = "1"
    value1 = ["abc"]
    value2 = [1,2,3]
    data = {key1: value1, key2: value2}
    ref_count1 = sys.getrefcount(key1)
    ref_count2 = sys.getrefcount(key2)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))
    print(ref_count2, sys.getrefcount(key2))

def test_does_not_leak_dictionary_string_key():
    import gc
    gc.collect()
    key1 = "1"
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

def test_does_not_leak_dictionary_tuple_key():
    import gc
    gc.collect()
    key1 = ("a",)
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

def test_does_not_leak_dictionary_bytes_key():
    import gc
    gc.collect()
    key1 = b"1"
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

def test_does_not_leak_dictionary_None_key():
    import gc
    gc.collect()
    key1 = None
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

test_does_not_leak_dictionary_values()
test_does_not_leak_dictionary_keys()
test_does_not_leak_dictionary_string_key()
test_does_not_leak_dictionary_tuple_key()
test_does_not_leak_dictionary_bytes_key()
test_does_not_leak_dictionary_None_key()
@Adriandorr

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

Have you tried adding the tests from the pull requests without the fixes?

@nav-agarwal

This comment has been minimized.

Copy link

commented Apr 24, 2019

@Adriandorr

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

Strange. It’s been a while since I used this. I think it was with python 3, but might well be wrong.

if (GET_TC(tc)->itemName) {
Py_DECREF(GET_TC(tc)->itemName);
GET_TC(tc)->itemName = NULL;
}

This comment has been minimized.

Copy link
@jayvdb

jayvdb May 11, 2019

ping @Adriandorr , this looks like the last remaining problem..?

@sadovnychyi sadovnychyi referenced this pull request Jun 3, 2019

Closed

Memory leaks in ujson #4

sadovnychyi added a commit to lawinsider/srsly that referenced this pull request Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.