Skip to content

gh-141968: Use take_bytes to simplify and remove copy from pyrepl BaseEventQueue#149852

Open
lgeiger wants to merge 2 commits into
python:mainfrom
lgeiger:pyrepl-eventq-take-bytes
Open

gh-141968: Use take_bytes to simplify and remove copy from pyrepl BaseEventQueue#149852
lgeiger wants to merge 2 commits into
python:mainfrom
lgeiger:pyrepl-eventq-take-bytes

Conversation

@lgeiger
Copy link
Copy Markdown
Contributor

@lgeiger lgeiger commented May 14, 2026

This PR uses bytearray.take_bytes to simplify pyrepl's BaseEventQueue.flush_buf and remove copies from BaseEventQueue.push

import timeit
from statistics import mean, stdev

times = timeit.repeat(
    "[q.push(b) for b in data]",
    setup=(
        "from _pyrepl.base_eventqueue import BaseEventQueue\n"
        "q = BaseEventQueue('utf-8', {})\n"
        "data = b'hello world' * 8\n"
    ),
    number=50_000,
    repeat=5,
)
print(f"{mean(times) * 1e3:.2f} ± {stdev(times) * 1e3:.2f} ms")
main:    1845.39 ± 10.02 ms
this PR: 1371.56 ± 3.85 ms

@maurycy
Copy link
Copy Markdown
Contributor

maurycy commented May 15, 2026

cc @cmaloney

return not self.events

def flush_buf(self) -> bytearray:
def flush_buf(self) -> bytes:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe this method is redundant now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to remove the function. For now I kept it to keep the diff small. But happy to remove it and just use self.buf.take_bytes() instead. Let me know what you prefer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 to removing, this is only used three places and I don't think after this change test_flush_buf is adding a lot of value

Comment thread Misc/NEWS.d/next/Library/2026-05-14-23-03-48.gh-issue-141968.4cQ6Vf.rst Outdated
@lgeiger lgeiger force-pushed the pyrepl-eventq-take-bytes branch from 1b0d030 to bf42a7b Compare May 15, 2026 15:16
Copy link
Copy Markdown
Contributor

@cmaloney cmaloney left a comment

Choose a reason for hiding this comment

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

Happy to see .take_bytes() helping speed up more code!

old = self.buf
self.buf = bytearray()
return old
return self.buf.take_bytes() # type: ignore[attr-defined, no-any-return]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the attr-defined shouldn't be needed here / if the type checker supports 3.15+ the attribute is always defined; not sure about the rest (I'd remove unless the in-repository mypy check requires)

return not self.events

def flush_buf(self) -> bytearray:
def flush_buf(self) -> bytes:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 to removing, this is only used three places and I don't think after this change test_flush_buf is adding a lot of value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants