Skip to content
This repository was archived by the owner on Jan 13, 2021. It is now read-only.

The _remaining_capacity > _bytes_in_buffer check in bufsocket.py is unnecessary #300

Closed
laike9m opened this issue Jan 12, 2017 · 6 comments
Closed

The _remaining_capacity > _bytes_in_buffer check in bufsocket.py is unnecessary #300

laike9m opened this issue Jan 12, 2017 · 6 comments

Comments

@laike9m
Copy link
Contributor

laike9m commented Jan 12, 2017

This is not an actual issue, but a possible tiny improvement.

TL;DR
I think this check self._remaining_capacity > self._bytes_in_buffer in bufsocket.py#L141 is unnecessary.

Here's why.
From the definitions we know:

  self._remaing_capacity - self._bytes_in_buffer
= (self._buffer_size - self._index) - self._bytes_in_buffer
= self._buffer_size - (self._index + self._bytes_in_buffer)
= self._buffer_size - self._buffer_end

We also know that each time self._bytes_in_buffer increases, the increment is less than
_buffer_size - _buffer_end. An example:

if (self._remaining_capacity > self._bytes_in_buffer and should_read):
    count = self._sck.recv_into(self._buffer_view[self._buffer_end:])
    if not count and amt > self._bytes_in_buffer:
        raise ConnectionResetError()
    self._bytes_in_buffer += count   # count cannot be greater than _buffer_size - _buffer_end

Now we have two formulas:

_buffer_size - _buffer_end = _remaing_capacity - _bytes_in_buffer    ①
count < _buffer_size - _buffer_end                                   ②

Thus _bytes_in_buffer < _remaing_capacity is always True.

  _bytes_in_buffer(new) 
= _bytes_in_buffer(old) + count 
< _bytes_in_buffer + _buffer_size - _buffer_end 
= _remaing_capacity

I intended to remove the check and run tests, but unfortunately I was not able to finish them(I have all the test deps installed and run make test, don't know why it takes so long). So I'm not 100% sure it can be removed.
@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2017

I suspect you're right. ;) Anyone who wants to apply this patch is welcome to.

@laike9m laike9m changed the title The _remaining_capacity > _bytes_in_buffer in bufsocket is unnecessary The _remaining_capacity > _bytes_in_buffer check in bufsocket.py is unnecessary Jan 12, 2017
@laike9m
Copy link
Contributor Author

laike9m commented Jan 12, 2017

I wonder how long do all the tests take to run, normally?

@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2017

For running the tests I recommend using tox, but you may have encountered a test hang. This project has CI, so you can simply open a PR if you'd prefer.

@laike9m
Copy link
Contributor Author

laike9m commented Jan 12, 2017 •
edited
Loading

Could you give me an estimate? So that I know if tests indeed hang. pytest-xdist shows I'm using all 4 cores, I don't know whether using tox can speed it up.

@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2017

On my machine under tox it runs in 6 seconds. I think we encountered problems with coverage measurements and pytest-xdist so the tests as run by tox moved away from that style, but I guess we didn't update the Makefile.

@laike9m
Copy link
Contributor Author

laike9m commented Jan 12, 2017

OK I'll try tox to see if everything works.

laike9m added a commit to laike9m/hyper that referenced this issue Jan 18, 2017
@laike9m
Removed unnecessary size check python-hyper#300
e8d4034
@laike9m laike9m mentioned this issue Jan 18, 2017
Merged
@laike9m laike9m closed this as completed Jan 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Lukasa @laike9m