classification
Title: asyncore stores unnecessary object references
Type: performance Stage: patch review
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: eric.araujo, giampaolo.rodola, pitrou, socketpair
Priority: normal Keywords:

Created on 2011-02-20 15:44 by socketpair, last changed 2011-03-15 19:17 by giampaolo.rodola. This issue is now closed.

Messages (11)
msg128909 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-02-20 15:44
asyncore.py:
-----------
    def add_channel(self, map=None):
        #self.log_info('adding channel %s' % self)
        if map is None:
            map = self._map
        map[self._fileno] = self
---------

As we see, it add itself to a map, creating unnecessary refences to 'self'.

Such code should be rewritten via weakref. Now it's unknown when object will garbage collected. For example, if someone forget to call close(), object will stuck, eating file descriptor and memory... 

When using weakref, we may guarantee (via callback fcuntion), that we call close() when last reference to object has been lost. Also, such behaviour guarantee, that object will be garbage collected when last user's reference has gone.

To approve my thoughts, see code:
------------------------------
class MyServer(asyncore.dispatcher):
    def __init__(self, listenaddr):
        asyncore.dispatcher.__init__(self)
        self.create_socket(socket.AF_INET, socket.SOCK_STREAM)
        self.set_reuse_addr()
        self.bind(listenaddr)
        self.listen(5)
    def handle_accept(self):
        while 1:
            r=self.accept()
            if r is None:
                break
            my_conn_handler(r[0])
------------
As we see, we create a new instance via my_conn_handler(), and we do not store reference to it, but object will not be garbage collected. It's unexpected behaviour.
msg128920 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-02-20 20:47
> Such code should be rewritten via weakref.

Can you write a patch?
msg128936 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-02-21 04:44
--- asyncore.py	2010-09-15 22:18:21.000000000 +0600
+++ asyncore.py	2011-02-21 09:43:15.033839614 +0500
@@ -58,7 +58,7 @@
 try:
     socket_map
 except NameError:
-    socket_map = {}
+    socket_map = weakref.WeakValueDictionary()
 
 def _strerror(err):
     try:
msg128937 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-02-21 04:46
sorry, forgot 
"import weakref"
msg128969 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-02-21 16:27
I'd be fine with this. My only concern are performances.
I've tried this:
http://code.google.com/p/pyftpdlib/issues/attachmentText?id=152&aid=-7106494857544071944&name=bench.py&token=bd350bbd6909c7c2a70da55db15d24ed

Results:

plain dict: 722.26 Mb/sec
WeakValueDictionary: 659.76 Mb/sec
msg129458 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-25 23:02
Well, isn’t WeakRefDictionary faster than a dict here?
msg129459 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-25 23:02
WeakValue*
msg129462 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-25 23:06
Éric, the weak dicts are implemented in pure Python while built-in dicts are in C. That can make quite a difference.
msg129476 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-26 00:19
Oh, I guess I didn’t understand at all what the numbers meant.  I shouldn’t have compared the values and assumed that less was better, my bad.
msg129544 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-02-26 13:41
Unless there's a way to automatically call close() when a dispatcher instance is no longer referenced (and I can't think of anything to do that) I'd say we better close this as rejected.
msg131023 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-03-15 19:17
Closing out as per msg128969.
History
Date User Action Args
2011-03-15 19:17:24giampaolo.rodolasetstatus: open -> closed
nosy: pitrou, giampaolo.rodola, eric.araujo, socketpair
resolution: wont fix
2011-03-15 19:17:08giampaolo.rodolasetnosy: pitrou, giampaolo.rodola, eric.araujo, socketpair
messages: + msg131023
2011-02-26 13:41:51giampaolo.rodolasetnosy: pitrou, giampaolo.rodola, eric.araujo, socketpair
messages: + msg129544
2011-02-26 00:19:55eric.araujosetnosy: pitrou, giampaolo.rodola, eric.araujo, socketpair
messages: + msg129476
2011-02-25 23:06:44pitrousetnosy: + pitrou
messages: + msg129462
2011-02-25 23:02:53eric.araujosetnosy: giampaolo.rodola, eric.araujo, socketpair
messages: + msg129459
2011-02-25 23:02:31eric.araujosetversions: - Python 2.6, Python 3.1, Python 2.7, Python 3.2
nosy: + eric.araujo

messages: + msg129458

type: behavior -> performance
stage: patch review
2011-02-21 16:27:17giampaolo.rodolasetmessages: + msg128969
2011-02-21 04:46:19socketpairsetmessages: + msg128937
2011-02-21 04:44:57socketpairsetmessages: + msg128936
2011-02-20 20:47:22giampaolo.rodolasetmessages: + msg128920
2011-02-20 20:32:47pitrousetassignee: giampaolo.rodola

nosy: + giampaolo.rodola
2011-02-20 15:44:05socketpaircreate