lwhite1 commented on PR #13248: URL: https://github.com/apache/arrow/pull/13248#issuecomment-1262825876
> My objection here is that the semantics of what is going on here are unclear. We can add code that makes things pass tests but if the _intent_ is already hard to follow then we're just setting up future library users - and ourselves - for more confusion. Agreed, but the argument for moving ahead is not so much 'make tests pass' as 'unblock all uses of a vector or VectorSchemaRoot returned by the C-Data-interface that require a memory transfer'. > In any case I don't have enough time really to look into things deeply. So if another maintainer wants to merge this then I'm not going to object. I appreciate all the time you put into this. I will go hunting for additional eyeballs. > I still think we're too fixated on this being a memory 'allocator' when it seems more like a 'nursery' object. Yes. The name is unfortunate. TBH, I'm not sure what role the Java allocator plays for data managed by native memory. It seems like it's mostly there because the API requires it. > Also I guess going back to this > > > I guess this is reasonable given where we are, but associating with an allocator that isn't doing the allocation seems likely to confuse future developers if the memory appears to be managed in the Java code but isn't. Is the idea that all users of a given bit of memory keep track of their own use, but only the originator (C++ presumably in this case) actually frees it? > > This makes sense to me from the 'nursery' or 'accountant' perspective. Closing a buffer delegates to the specific ReferenceManager implementation, which may use Java code or native code. It also updates the allocation metric in the BufferAllocator, which is separate from how the buffer is actually implemented. Then the application closes the BufferAllocator, which can check at runtime that the application actually closed all buffers it claims were associated with this scope. Transfer, in this scheme, is a way to change what object is responsible for allocated bytes, as opposed to allocating new ones or freeing allocated ones. I agree that the name "allocator" is somewhat confusing when it may not necessarily allocate, but I feel like a general cleanup of this code is probably beyond the scope of this PR. Maybe a ticket could be opened for that and better documentation would help in the short term? -- 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]
