classification
Title: Make memoryview weakrefable
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: jcea, neologix, pitrou, python-dev, sbt, skrah
Priority: normal Keywords: patch

Created on 2012-05-27 21:05 by sbt, last changed 2012-07-25 17:03 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
memoryview-weakref.patch sbt, 2012-05-27 21:05
memoryview-weakref.patch sbt, 2012-05-28 10:30 review
memoryview-weakref.patch sbt, 2012-05-28 14:44 review
Messages (14)
msg161729 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-05-27 21:05
The attached patch makes memoryview objects weakrefable.

The reason I would like them to be weakrefable is so that I can manage the finalization and pickling of memoryview objects which wrap shared mmap segments.

(It would be even better if memoryview were subclassable, but I don't know if naively changing tp_flags would be enough.)
msg161730 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-27 21:07
Looks like an obviously good idea.
The patch needs tests, though.
msg161763 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-05-28 10:30
New patch.
msg161770 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-28 11:44
In the test, you should call gc.collect() so that it works on non-reference counted implementations. Also, I would call PyObject_ClearWeakRefs() after memory_release() and Py_CLEAR(self->mbuf), not before (in case a weakref callback relies on the buffer being released).
msg161776 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-05-28 12:50
> In the test, you should call gc.collect() so that it works on non-
> reference counted implementations.

I did think about using gc.collect(), but I was not sure whether it was guaranteed to collect everything possible if you only call it only once.  (I know nothing about garbage collectors.)

> Also, I would call PyObject_ClearWeakRefs() after memory_release() and 
> Py_CLEAR(self->mbuf), not before (in case a weakref callback relies on 
> the buffer being released).

Doing it after Py_CLEAR(self->mbuf) seems to contradict 

    http://docs.python.org/dev/extending/newtypes.html?highlight=pyobject_clearweakrefs#weak-reference-support

which says

    The only further addition is that the destructor needs to call the weak 
    reference manager to clear any weak references. This should be done *before*
    any other parts of the destruction have occurred, but is only required if the 
    weak reference list is non-NULL:
msg161778 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2012-05-28 13:03
> I did think about using gc.collect(), but I was not sure whether it was
> guaranteed to collect everything possible if you only call it only once.  
> (I know nothing about garbage collectors.)

You could use support.gc_collect() for that.
msg161779 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-28 13:13
> Doing it after Py_CLEAR(self->mbuf) seems to contradict 
> 
>     http://docs.python.org/dev/extending/newtypes.html?highlight=pyobject_clearweakrefs#weak-reference-support
> 
> which says
> 
>     The only further addition is that the destructor needs to call the weak 
>     reference manager to clear any weak references. This should be done *before*
>     any other parts of the destruction have occurred, but is only required if the 
>     weak reference list is non-NULL:

Mmh, this seems to be misled. The original formulation is from SVN
r16381; the clearly erroneous part about resurrecting objects was later
removed in r18223, but the rest is probably unnecessary as well. I'll
open a separate issue.
msg161788 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-05-28 14:44
Updated patch.
msg161797 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-28 18:07
> Updated patch.

Looks good to me!
msg161806 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-28 20:36
New changeset bc0281f85409 by Richard Oudkerk in branch 'default':
Issue #14930: Make memoryview objects weakrefable.
http://hg.python.org/cpython/rev/bc0281f85409
msg166381 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-07-25 10:59
A quick question: Prior to this patch test_memoryview.py exercised
both mbuf_clear() and memory_clear(). Now gcov shows no coverage.

Is this expected? Is it still possible to construct tests that
exercise the code?
msg166406 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-25 15:38
Is it possible that the use of test.support.gc_collect() in test_memoryview made the difference?
msg166412 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-07-25 16:45
Sorry folks, I messed up the revisions when testing. This commit has
nothing to do with the decreased coverage.

Now I properly bisected and in r75481 mbuf_clear() and memory_clear()
are still covered, but in r75484 they are not.

r75484 addresses #1469629. I've got to figure out whether the changed
behavior is expected or not.
msg166416 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-07-25 17:03
r75484 should be b595e1ad5722.
History
Date User Action Args
2012-07-25 17:03:32skrahsetmessages: + msg166416
2012-07-25 16:45:21skrahsetmessages: + msg166412
2012-07-25 15:38:51sbtsetmessages: + msg166406
2012-07-25 10:59:29skrahsetmessages: + msg166381
2012-05-29 08:32:17sbtsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2012-05-28 20:36:41python-devsetnosy: + python-dev
messages: + msg161806
2012-05-28 18:07:29pitrousetmessages: + msg161797
2012-05-28 18:02:29jceasetnosy: + jcea
2012-05-28 14:44:38sbtsetfiles: + memoryview-weakref.patch

messages: + msg161788
2012-05-28 13:13:23pitrousetmessages: + msg161779
2012-05-28 13:03:46neologixsetnosy: + neologix
messages: + msg161778
2012-05-28 12:50:03sbtsetmessages: + msg161776
2012-05-28 11:44:28pitrousetmessages: + msg161770
2012-05-28 10:30:02sbtsetfiles: + memoryview-weakref.patch

messages: + msg161763
2012-05-27 21:07:53pitrousetnosy: + skrah, pitrou
messages: + msg161730
2012-05-27 21:05:59sbtcreate