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

Are we sure the memoryview atomics need aligning #5509

Closed
da-woods opened this issue Jul 3, 2023 · 0 comments · Fixed by #5510
Closed

Are we sure the memoryview atomics need aligning #5509

da-woods opened this issue Jul 3, 2023 · 0 comments · Fixed by #5510

Comments

@da-woods
Copy link
Contributor

da-woods commented Jul 3, 2023

Describe your issue

We have a slightly complicated scheme to make sure memoryview atomics are aligned:

# the following array will contain a single __pyx_atomic int with
# suitable alignment
cdef __pyx_atomic_int acquisition_count[2]
cdef __pyx_atomic_int *acquisition_count_aligned_p

self.acquisition_count_aligned_p = <__pyx_atomic_int *> align_pointer(
<void *> &self.acquisition_count[0], sizeof(__pyx_atomic_int))

I've had a quick try at removing it, and the indirection from this scheme seems to cause something like a 20% performance loss, for a toy example doing memoryview slicing in a loop.

My view is that:

  • __pyx_atomic_int should already be aligned appropriately within the __pyx_memoryview struct (because I'm pretty sure the C compiler guarantees this).
  • The allocated __pyx_memoryview should be aligned appropriately because
    1. malloc guarantees this (by allocating to the maximum alignment the system requires)
    2. Although Python itself will be responsible for the positioning of the struct within allocated memory,
      it's pretty much got to be right, otherwise it'd break a lot of code (especially on platforms that require alignment)

Therefore, I think this scheme is an unnecessary pessimization. I've tried putting in an assert to check the alignment and can't get it to trip locally (which doesn't prove it's right, but suggests it might be at least on some platforms).

Am I missing anything?

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

Successfully merging a pull request may close this issue.

1 participant