westonpace commented on PR #35565: URL: https://github.com/apache/arrow/pull/35565#issuecomment-1552401874
I tried for a while to reproduce some kind of alignment related issue on godbolt and was unsuccessful. I then did some research. Most of the examples I could find either: A. Demonstrated potential performance penalties (which were often cpu-specific) associated with aligned loads B. Were specific to 32-bit ARM processors (which crash on unaligned loads) I think A is mitigated by the fact that any performance benefit is unlikely to outweigh the cost of the allocation / copy required by EnsureAlignment. B is not important as we don't support 32-bit builds (outside of maybe windows 32-bit) that I'm aware of. The root of all this discussion is from some assertions in (what is now) `compare_internal.cc` which would DCHECK that a buffer was aligned. These appear to have been added defensively, and not in response to any actual issue: https://github.com/apache/arrow/pull/10290#discussion_r688021792 So how do we feel about: * Proceed with adding this capability to `CheckAlignment` / `EnsureAlignment` (the utility could still be a useful feature for someone that needs aligned buffers for some other reason) * Remove all defensive alignment assertions (this is a DCHECK to ensure that a buffer is aligned that is sometimes used before type punning) * Change the call in `SourceNode` from "ensure buffers are malloc aligned" to "check if buffers are malloc aligned" and, if not, log a warning along the lines of `One or more buffers are unaligned. This may lead to suboptimal performance. Check to see if it is possible for the data source to generate aligned buffers.` CC @bkietz @pitrou @felipecrv -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
