tustvold commented on PR #6336: URL: https://github.com/apache/arrow-rs/pull/6336#issuecomment-2376366898
I've not had time to look at this PR in detail, but my initial response is to echo the concern Andrew raised regarding the general utility of this abstraction. As I understand it this mechanism would require making every codepath that allocates arrays be generic on an Allocator and take an Allocator as a parameter. Even putting aside the fact that this would only be possible on nightly, this is a monumental amount of work effectively doubling the API surface. Taking a step back, memory allocation in arrow should rarely be on a hot path. As such having monomorphised access to the allocator implementation is perhaps not as important as for say a standard library collection type, we can afford to have a moderate additional overhead per allocation. Additionally the proposed use-cases appear to largely center around memory tracking, as opposed to altering the behaviour of the system allocator. As such we could potentially just store a `std::sync::Weak<AtomicUsize>` in thread local storage, with allocations decrementing / incrementing this as appropriate. Implementations can then override to capture allocations performed in that context. Some care would be needed in async contexts, e.g. by wrapping spawned futures, but nothing overly intrusive. -- 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]
