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

Fix py3 socket slots #1724

Closed
wants to merge 1 commit into from
Closed

Conversation

carsonip
Copy link
Contributor

Was casually reading gevent source code, found something interesting: python3 socket is not using slots properly. _read_event and _write_event instance attrs are added to __dict__ instead of __slots__. The problematic class attrs mask the slots, and that change was made in 2018 and the SocketMixin with slots was added in 2020. I think it should be just a careless mistake (and it didn't do much harm indeed).

Before change (gevent master, py38):

In [1]: from gevent import socket                                                                                                                     

In [2]: s = socket.socket()                                                                                                                           

In [3]: s.__dict__                                                                                                                                    
Out[3]: 
{'timeout': None,
 'hub': <Hub '' at 0x7f1c51043ac0 epoll default pending=0 ref=0 fileno=18 thread_ident=0x7f1c53d64740>,
 '_read_event': <io at 0x7f1c509d1740 native=0x7f1c509d1780 fd=17 events=READ|_IOFDSET>,
 '_write_event': <io at 0x7f1c509d1840 native=0x7f1c509d1880 fd=17 events=WRITE|_IOFDSET>}

In [12]: %timeit -r 12 s._read_event                                                                                                                  
36.1 ns ± 0.272 ns per loop (mean ± std. dev. of 12 runs, 10000000 loops each)

In [17]: sys.getsizeof(s.__dict__)                                                                                                                    
Out[17]: 104

After change:

In [1]: from gevent import socket                                                                                                                     

In [2]: s = socket.socket()                                                                                                                           

In [3]: s.__dict__                                                                                                                                    
Out[3]: 
{'timeout': None,
 'hub': <Hub '' at 0x7f587b6a1f40 epoll default pending=0 ref=0 fileno=18 thread_ident=0x7f587e4db740>}

In [4]: s._read_event                                                                                                                                 
Out[4]: <io at 0x7f587b7dcb40 native=0x7f587b7dcb80 fd=17 events=READ|_IOFDSET>                                                                                                                               

In [6]: sys.getsizeof(s.__dict__)                                                                                                                     
Out[6]: 104

In [15]: %timeit -r 12 s._read_event                                                                                                                  
33.3 ns ± 0.173 ns per loop (mean ± std. dev. of 12 runs, 10000000 loops each)

No memory usage change. Marginal performance gain (<10%) from using slots for attr access. My laptop produces unstable microbenchmarks so you may get a different number.

Fix read and write event instance attrs added to __dict__ instead of
__slots__.
@carsonip
Copy link
Contributor Author

carsonip commented Dec 20, 2020

Or maybe we can once and for all ditch the instance dict, add timeout and hub to the mixin slots and add empty slots to socket2 and socket3. But there are self.__dict__['timeout'] below that makes it not so straightforward.

jamadden added a commit that referenced this pull request Dec 21, 2020
@jamadden
Copy link
Member

That's a good idea, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants