<mattip>
heh, spent a while trying to work out why "a = np.empty([100000
<mattip>
"a = np.empty([100000]); a.fill(0)" leaks memory but "a = np.empty([100000])" does not,
<arigato>
mattip: btw did you see my comments yesterday about PyList_SetItem()?
<arigato>
and hi :-)
<mattip>
it turns out we never call tp_dealloc() on a, but alloc() is smart enough to not actually allocate
<mattip>
hi
<mattip>
yes, I responded "for the logs"
<arigato>
ah :-)
<mattip>
there are subsequent commits that bring us into cpython compatibility
<mattip>
unless I made a mistake, but I added a single test
<mattip>
maybe more are needed
<arigato>
note that I maintain that it used to work like CPython, and no longer does, notably PyList_SET_ITEM
Taggnostr has quit [Quit: No Ping reply in 180 seconds.]
<mattip>
even after 8a55a93fe3df ?
<arigato>
and PyLIst_SetItem too actually, I think
<arigato>
yes
<mattip>
well, at least for PyList_SET_ITEM there is a test, maybe the test is wrong?
<arigato>
does it pass on CPython?
<mattip>
yes
Taggnostr has joined #pypy
<mattip>
I started with -A and then made our implementation match
<arigato>
ah
<arigato>
why are you calling PyLong_FromLong(0)?
<arigato>
in py3 this returns a cached object
<arigato>
the goal of the test is to avoid that
oberstet3 has quit [Ping timeout: 255 seconds]
<mattip>
ok, I will create some random PyStringObject instead
<arigato>
again, I'm not sure, but I *think* the test is not exposing the problem because it calls PyList_SET_ITEM to replace o2 with o2 itself
<arigato>
mattip: what is wrong with PyList_New(0)?
<mattip>
ok
tifo has joined #pypy
<mattip>
I can add a third object and check that the refcount leaks/doesn;t leak like in cpython
<arigato>
overall, I'm skeptical that everything is right because you killed a Py_DECREF inside PyList_SET_ITEM, but the old test used to pass---does it still pass?
<arigato>
maybe it does
<mattip>
see listobject.c in cpython, we now have AFAICT exactly the same logic
<arigato>
no
<mattip>
cpython 2.7, I did not check 3.5
<arigato>
w_list.setitem() does internally an incref, I believe
<mattip>
no, then the test would fail
<arigato>
no, it does
<arigato>
it has to, otherwise the rest of the code would fail
<arigato>
see make_ref() inside sequence.py:setitem
<arigato>
but it also does a decref() on the old item
<arigato>
that's why it passes the test, which replaces o2 with o2
<arigato>
the proper fix is probably to use the cpy_strategy, in order to directly replace the raw item
<arigato>
you copy-pasted the use of cpy_strategy from PyList_GetItem(), but I think it has no effect here
<mattip>
right
<arigato>
inside PyList_GetItem(), the reason it works is (probably too) subtle
<arigato>
I would rewrite both PyList_GetItem() and the SetItem()/SET_ITEM() to use cpy_strategy more directly and store/load a pyobj, never converting it to w_res or w_item
<mattip>
ok
<mattip>
and more tests, with different objects
<arigato>
yes :-)
<arigato>
...yes, GetItem() is slightly strange as written now: it calls sequence.py:getitem(), which takes the PyObject from storage._elems, calls from_ref(), and return that, which is then turned back to a PyObject
<arigato>
it's my fault I guess
<arigato>
these GetItem/SetItem functions should not call w_list.{get,set}item()
<mattip>
I imagine if we poke at most any part of cpyext we will find pain points
<kenaan>
mattip cpyext-obj-stealing 2bf4323abb8e /pypy/module/cpyext/test/test_listobject.py: rework test as per irc discussion, passes with -A and untranslated
<kenaan>
mattip cpyext-obj-stealing ae8d9aacb75e /pypy/module/cpyext/test/test_listobject.py: test shows that w_list.getitem returns a new PyObject with wrong refcount
<mattip>
well, that is not good, AFAICT this test would fail on default as well
<mattip>
bye
mattip has left #pypy ["bye"]
marky1991 has quit [Quit: Saliendo]
lritter_ has quit [Ping timeout: 246 seconds]
marky1991 has joined #pypy
<LarstiQ>
mattip: (for logs) bye!
yuyichao has quit [Ping timeout: 260 seconds]
realitix has joined #pypy
yuyichao has joined #pypy
Taggnostr has quit [Quit: No Ping reply in 180 seconds.]