Fix alignment restrictions #269
Conversation
|
I'm not that familiar with the code in question (I inherited it from elsewhere). Two questions:
|
|
Correction to the previous comment, I am actually the author of this code, I thought I was looking at something else. Still, original concern remains: can we avoid the performance hit of copying on systems that don't require alignment? |
|
Hi,
In the test suite, the pokeByteOff function is used in order to fill the bytestring with a list of SomeStorable. This causes unaligned access in case, for example, the first item is of type Word8 and the second of type Double. This has nothing to do with the alignment of the bytestring itself.
I am not an expert on this, so I am not sure how bad the impact on the performance would be. From what I understand, all architectures impose alignment constraints on memory access, but some of them are able to correct an unaligned one transparently, with a performance cost. Other times, the kernel may handle the exception and fall-back to copying data (see also unaligned-memory-access from Linux kernel). I couldn't find a list of architectures where the above properties are described, so I thought it would be safer to enforce aligned access to all of them. If we could find such a document, then we could enable this feature only for the affected architectures, using the preprocessor. What do you think? |
|
In our performance testing for Store, we found that on x86 architectures at least, unaligned access did not negatively hurt performance. I'd be happy to go with a whitelist approach here and just say that x86 (32- and 64-bit) continue with the code as-is, and all other architectures use the copy/align logic you've implemented here. |
|
Yes, that sounds great! Do you want me to implement the whitelisting and update the PR, or would you like to do it yourself? I also noticed that mapM_BS uses the peek function as well, but since the testsuite now passes, I didn't change that. Do you believe we should change that as well? P.S.: We applied the patch in Debian and the testsuite now passes for all architectures. |
|
If you could update it, that would be great. For the |
The effects of performing an unaligned memory access vary from architecture to architecture. Add the ALLOW_UNALIGNED_ACCESS macro that denotes that unaligned memory access is allowed for the architecture for which we are building. For now, unaligned memory access is only allowed for the x86_64 and i386 architectures.
sinkStorable uses the peek function to read a value from the given memory location but doesn't pay attention to the alignment constraints. This makes the code to fail under the armhf architecture (thus making the code non-portable). Change the sinkStorableHelper function to use a safer version of the peek function which first copies the data byte-for-byte to a new, properly aligned location, and then reads the value. The code reverts to the old implementation in case ALLOW_UNALIGNED_ACCESS is defined.
Modify the someStorables function to take into account the alignment constraints when using the pokeByteOff function.
iliastsi
force-pushed
the
feature-alignment
branch
from
50c2cbe
to
a49b5db
Compare
|
Sorry for the delay, I have updated this PR. Also, I am a little worried about the use of the |
|
Before getting too deep into |
The testsuite for conduit-extra fails on armhf architectures because of unaligned memory access.
According to the docs, the peek and poke functions might require properly aligned addresses to function correctly. This is architecture dependent; thus, portable code should ensure that when peeking or poking values of some type a, the alignment constraint for a, as given by the function alignment is fulfilled.
Here I provide two patches that ensure that alignment constraints are fulfilled. The implementation requires one extra copy, but makes the code portable.
I have patched the code in two steps, first the sinkStorable implementation and then the testsuite. This hopefully demonstrates that the patches introduce no regressions.