Remove some uses of buffer() #12
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It looks like the uses of buffer() here were just to do a non-copying slice - i.e. buffer(data, p) is equivalent to data[p:], but it points to the existing memory used for data rather than copying it. I'm proposing this change for discussion: it replaces the buffer calls with standard slicing. If bytes objects are passed in, this will copy data, with some potential performance loss - how much depends on how many times we're slicing each chunk of data. The tests are not measurably slower. If the overhead of copying is significant, the caller of the parsing machinery can pass in a memoryview object, on which slices should be non-copying. I think this should mostly work with no changes to this code, but if that's important, we should obviously test it. This reduces the number of test failures on Python 3 to 251 (26 failures, 225 errors). Tests still pass on Python 2.
Current coverage is
|
Contributor
|
A bit of background here: the extreme uses of buffer and the generated functions for packing/unpacking were necessary to be able to run PLWM on a PentiumII laptop in 1999. It should fine to rewrite it all to use more straight-forward code when that is simpler, even on a Raspberry Pi these days. The unit tests can check that messages are packed and unpacked correctly with the rewritten code. |
Contributor
Author
|
That's good to know, thanks! |
Collaborator
|
This is also fine to me. Merging... |
vasily-v-ryabov
added a commit
that referenced
this pull request
Feb 18, 2016
Remove some uses of buffer()
takluyver
added a commit
to takluyver/python-xlib
that referenced
this pull request
Feb 18, 2016
Struct.to_binary, Struct.parse_value and Struct.parse_binary were doing code generation to produce and store a specialised method for each struct instance. Apparently (see python-xlib#12) this was a necessary optimisation in the distant past, but it's almost certainly no longer needed, and it makes the code considerably harder to follow. On my machine the tests run slightly but noticeably faster with this change, but of course the tests are probably not a realistic sample for performance testing. This doesn't directly impact Python 3 support, but it simplifies things.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It looks like the uses of buffer() here were to do a non-copying slice - i.e.
buffer(data, p)is equivalent todata[p:], but it points to the existing memory used for data rather than copying it.I'm proposing this change for discussion: it replaces the buffer calls with standard slicing. If bytes objects are passed in, this will copy data, with some potential performance loss - how much depends on how many times we're slicing each chunk of data. The tests are not measurably slower.
If the overhead of copying is significant, the caller of the parsing machinery can pass in a memoryview object, on which slices should be non-copying. I think this should mostly work with no changes to this code, but if that's important, we should obviously test it.
This reduces the number of test failures on Python 3 to 251 (26 failures, 225 errors). Tests still pass on Python 2.