Skip to content

Conversation

@JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Nov 6, 2025

This includes the following optimizations:

  • Split Buffer into ReadBuffer and WriteBuffer which are more specialized
  • Only check buffer types in wrapper functions, not in C primitives
  • Use pointers instead of integer indexes in the buffer objects

This improves the performance of a micro-benchmark that reads integers in a loop by 5%, but this could help more if we'd inline some of the smaller functions (in the future). By making the functions simpler, inlining is more feasible.

I'm not sure what's the best way to merge this -- maybe we'll need to have broken master for a while, and then we ca publish a new version of librt, and then update mypy to work using the new librt version.

@JukkaL JukkaL requested a review from ilevkivskyi November 6, 2025 17:02
@JukkaL
Copy link
Collaborator Author

JukkaL commented Nov 6, 2025

Compiled mypy is expected to fail in CI, since we need to publish a new version of librt with these changes first, and update mypy to use the new API (which is easy, but the new API is not backward compatible with the original one).

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

LG, thanks! Just few minor things. I think it is fine to break CI for short time. It would be good to fix it before weekend (there may be some contributors who may be confused by the failure).

@@ -1,19 +1,27 @@
from mypy_extensions import u8

# TODO: Remove Buffer -- right now we have hacky support for BOTH the old and new APIs
Copy link
Member

Choose a reason for hiding this comment

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

When will we be able to remove it? Right after the new version of librt is published, right?

with assertRaises(ValueError):
read_int(r)

def test_buffer_primitive_types() -> None:
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to add calls to this tests, and to the two tests below (you only added test_buffer_grow()).

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.

3 participants