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

Issue #2147: avoid copies on non-small writes #2169

Merged
merged 10 commits into from Nov 16, 2017

Conversation

pitrou
Copy link
Contributor

@pitrou pitrou commented Oct 19, 2017

Improves performance by 35% on the following benchmark: #2147 (comment)
Makes demos/benchmark/benchmark.py about 2% slower.

(split from #2166)

@bdarnell
Copy link
Member

I haven't read the full diff yet, but in #2008 we found that using memoryview without calling release() can be problematic (maybe just for slices of memoryviews?). We need to make sure that we're doing things correctly if we're going to go even further down the path of using memoryview.

@pitrou
Copy link
Contributor Author

pitrou commented Oct 22, 2017

I've checked that this PR is immune to #2008.

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - I'd like to see some basic docstrings on the new methods and I have a few naming nits, but otherwise I think it's ready to go.

elif size > 0:
if self._buffers:
is_large, b = self._buffers[-1]
is_large = is_large or len(b) >= self._large_buf_threshold
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_large is doing a lot of different things here. I think it woudl be more clear if this were

if self._buffers: 
    is_memview, b = self._buffers[-1]
    new_buf = is_memview or len(b) > self._large_buf_threshold
else:
    new_buf = True

def peek(self, size):
assert size > 0
try:
is_large, b = self._buffers[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_memview here as well.

else:
return memoryview(b)[pos:pos + size]

def skip(self, size):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this advance or consume?

@pitrou
Copy link
Contributor Author

pitrou commented Nov 7, 2017

There was a hang in the latest Travis build on 2.7. I'm not able to reproduce locally.

@bdarnell
Copy link
Member

Thanks!

@bdarnell
Copy link
Member

(I reran the travis build and it passed, so the hang was just a fluke)

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