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

asyncore stores unnecessary object references #55466

Closed
socketpair mannequin opened this issue Feb 20, 2011 · 11 comments
Closed

asyncore stores unnecessary object references #55466

socketpair mannequin opened this issue Feb 20, 2011 · 11 comments
Assignees
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@socketpair
Copy link
Mannequin

socketpair mannequin commented Feb 20, 2011

BPO 11257
Nosy @pitrou, @giampaolo, @merwok, @socketpair

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = 'https://github.com/giampaolo'
closed_at = <Date 2011-03-15.19:17:24.166>
created_at = <Date 2011-02-20.15:44:05.958>
labels = ['library', 'performance']
title = 'asyncore stores unnecessary object references'
updated_at = <Date 2011-03-15.19:17:24.165>
user = 'https://github.com/socketpair'

bugs.python.org fields:

activity = <Date 2011-03-15.19:17:24.165>
actor = 'giampaolo.rodola'
assignee = 'giampaolo.rodola'
closed = True
closed_date = <Date 2011-03-15.19:17:24.166>
closer = 'giampaolo.rodola'
components = ['Library (Lib)']
creation = <Date 2011-02-20.15:44:05.958>
creator = 'socketpair'
dependencies = []
files = []
hgrepos = []
issue_num = 11257
keywords = []
message_count = 11.0
messages = ['128909', '128920', '128936', '128937', '128969', '129458', '129459', '129462', '129476', '129544', '131023']
nosy_count = 4.0
nosy_names = ['pitrou', 'giampaolo.rodola', 'eric.araujo', 'socketpair']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue11257'
versions = ['Python 3.3']

@socketpair
Copy link
Mannequin Author

socketpair mannequin commented Feb 20, 2011

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.

@socketpair socketpair mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Feb 20, 2011
@giampaolo
Copy link
Contributor

Such code should be rewritten via weakref.

Can you write a patch?

@socketpair
Copy link
Mannequin Author

socketpair mannequin commented Feb 21, 2011

--- 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:

@socketpair
Copy link
Mannequin Author

socketpair mannequin commented Feb 21, 2011

sorry, forgot
"import weakref"

@giampaolo
Copy link
Contributor

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

@merwok
Copy link
Member

merwok commented Feb 25, 2011

Well, isn’t WeakRefDictionary faster than a dict here?

@merwok merwok added performance Performance or resource usage and removed type-bug An unexpected behavior, bug, or error labels Feb 25, 2011
@merwok
Copy link
Member

merwok commented Feb 25, 2011

WeakValue*

@pitrou
Copy link
Member

pitrou commented Feb 25, 2011

Éric, the weak dicts are implemented in pure Python while built-in dicts are in C. That can make quite a difference.

@merwok
Copy link
Member

merwok commented Feb 26, 2011

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.

@giampaolo
Copy link
Contributor

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.

@giampaolo
Copy link
Contributor

Closing out as per msg128969.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir
Projects
None yet
Development

No branches or pull requests

3 participants