Hi, I agree with Jorge's assessment, we should allocate buffers with a certain preferred alignment, but not actually require it when creating buffers from raw pointers or in any computation kernels. Kernels currently do not seem to rely on alignment, also simd implementations use unaligned loads. There is another invariant that some kernels might rely on that might get violated when creating buffers though, I think some (simd) kernels assume that buffers are always padded to a size multiple of 64 bytes and would be reading elements out of bounds otherwise. That assumption is however already violated by creating buffer slices and not specific to the create_from_raw_parts function. I've been planning to refactor those simd kernels for some time but did not get around to it yet.
Regards, Jörn On Fri, Sep 18, 2020 at 7:51 PM Jorge Cardoso Leitão < jorgecarlei...@gmail.com> wrote: > Hi, > > Thanks for the quick answers, and for the clarification wrt to the specs, > Antoine. > > I believe that no-one had the goal of making an architecture-depent > requirement of aligned memory. A plausible explanation is that no-one tried > to create a mis-aligned buffer, since, in Rust, they are allocated via the > same architecture-depent alignment and thus always pass the assert. I can > imagine myself placing that assert to make sure that our allocation was > right, and then forgetting to remove it... > > One simple solution is to continue to allocate buffers with a > architecture-depent alignment, as it offers speed benefits, but still allow > mis-aligned buffers to be created via raw pointers. This would make it > follow the spec: follow the recommendation on alloc, but do not require > alignment in general. > > In practice, this requires removing an "assert!". One question is whether > this invariant is being assumed somewhere else in our code. I assume this > to be negative, as that is the only code that uses that alignment constant. > As an extra data-point, I locally removed the assert and manually changed > the alignment of my own architecture, and the tests continue to pass. > > I encapsulated this on ARROW-10039 > <https://issues.apache.org/jira/browse/ARROW-10039> and I plan to > follow-up > with a PR. Since this is about following the specs, I placed it to target > 2.0.0. Feel free to move it around if you think that this requires further > discussion / is not worth targeting for the 2.0.0. > > Best, > Jorge > > > > On Fri, Sep 18, 2020 at 6:08 PM Antoine Pitrou <anto...@python.org> wrote: > > > > > Le 18/09/2020 à 18:03, Jorge Cardoso Leitão a écrit : > > > // panics with "memory not aligned" > > > Buffer::from_raw_parts(address as *const u8, size, size) > > > > > > I get an address such as `4624199872` (i64), but, when converted to > > `*const > > > u8`, our rust implementation does not consider it to be memory aligned > > (at > > > least in x86_64) and panics. Has anyone worked in this problem? Any > hints > > > on what I am doing wrong? > > > > The Rust implementation should not panic on misaligned buffers. 64-byte > > alignment is a recommendation, not a requirement. Buffers allocated by > > foreign systems such as Python / NumPy, or simply sliced on an arbitrary > > offset, will not necessarily be aligned. > > > > > AFAIK, our rust implementation requires different alignments, depending > > on > > > the architecture (see memory.rs > > > <https://github.com/apache/arrow/blob/master/rust/arrow/src/memory.rs > >). > > > > I am not a Rust expert, but I remember noticing the PR where this was > > proposed and explaining that this was wrong. Again: alignment is a > > recommendation, not a requirement. Making it mandatory *and* > > platform-specific even sounds a bit perverse... > > > > Regards > > > > Antoine. > > > -- *Jörn Horstmann* | Senior Backend Engineer www.signavio.com Kurfürstenstraße 111, 10787 Berlin, Germany <https://www.linkedin.com/company/signavio/> <https://www.twitter.com/signavio> <https://www.facebook.com/signavio> <https://www.youtube.com/user/signavio> <https://www.xing.com/companies/signaviogmbh> <https://t-eu.xink.io/Tracking/Index/tBsAALBtAAAnu0MA0> HRB 121584 B Charlottenburg District Court, VAT ID: DE265675123 Managing Directors: Dr. Gero Decker, Daniel Rosenthal
