10000 [mypyc] Add bytes concat op by 97littleleaf11 · Pull Request #10926 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

[mypyc] Add bytes concat op #10926

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Aug 6, 2021
Merged

[mypyc] Add bytes concat op #10926

merged 12 commits into from
Aug 6, 2021

Conversation

97littleleaf11
Copy link
Collaborator
@97littleleaf11 97littleleaf11 commented Aug 4, 2021 •
edited
Loading

Description

Related issue: mypyc/mypyc#880

Use PyByteArray_Concat and PyBytes_Concat in a c helper function for bytes concat.

Test Plan

Add several run tests for

  • bytes + bytearray
  • bytearray + bytes
  • bytes + bytes
  • bytearray + bytearray

This PR speeds up the bytes_concat microbenchmark from 1.67x to 2.01x

JukkaL
JukkaL reviewed Aug 4, 2021
View reviewed changes
@JukkaL
Copy link
Collaborator
JukkaL commented Aug 4, 2021

Also, can you post a microbenchmark result with this change?

@97littleleaf11
Copy link
Collaborator Author

Master:

running bytes_concat
..........
interpreted: 0.289666s (avg of 5 iterations; stdev 0.86%)
compiled:    0.174355s (avg of 5 iterations; stdev 1.3%)

compiled is 1.661x faster

This PR:

running bytes_concat
..........
interpreted: 0.292826s (avg of 5 iterations; stdev 0.31%)
compiled:    0.167645s (avg of 5 iterations; stdev 0.47%)

compiled is 1.747x faster

It seems that we can hardly benefit from this op.

@JukkaL
Copy link
Collaborator
JukkaL commented Aug 4, 2021

I think that it's worth trying to implement the bytes concat from first principles in C instead of using PyBytes_Concat. The Python C API function may have some overhead that we can avoid. The implementation seems quite easy. Also I'd suggest specializing for bytes + bytes only, since it may improve performance, and have a slow path for non-bytes values.

@r3m0t
Copy link
Contributor
r3m0t commented Aug 4, 2021

PyBytes_Concat has a fast path if the left operand has eaxctly one reference-is it being hit?

JukkaL
JukkaL approved these changes Aug 6, 2021
View reviewed changes
Copy link
Collaborator
@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good, just a few style nits. Thanks for carefully testing interactions between bytes and bytearray.

@JukkaL JukkaL merged commit e7161ac into python:master Aug 6, 2021
@97littleleaf11 97littleleaf11 deleted the add-concat-op branch August 7, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JukkaL JukkaL JukkaL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@97littleleaf11 @JukkaL @r3m0t
0