ARROW-7538: [Java] Clarify actual and desired size in AllocationManager #6204
Conversation
jacques-n
left a comment
jacques-n
left a comment
There was a problem hiding this comment.
Thanks for the contribution! A few small comments, otherwise looks good.
There was a problem hiding this comment.
This seems like a waste of heap space, no? Since AllocationManager is abstract, let's just make getAllocatedSize() abstract in the base class. If a child class needs to store the value, they can add the field. I didn't realize the waste on the initial patch, just now.
There was a problem hiding this comment.
yeah I was actually wondering about this when I implemented this - "why not just make getSize() abstract" . I will make the adjustment
There was a problem hiding this comment.
Let's remove this method.
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
@jacques-n I updated the PR based on your comment. please kindly take a look and see if this is what you had in mind. --Rong
java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Returns the underlying memory chunk size managed. | ||
| * | ||
| * <p>NettyAllocationManager manages memory chunk by the exact requested size. |
There was a problem hiding this comment.
is this description accurate? I am not sure but I also lean into explain a bit more:
| * <p>NettyAllocationManager manages memory chunk by the exact requested size. | |
| * <p>NettyAllocationManager manages memory chunk by computing the allocatedSize based on the requestedSize. Thus the allocatedSize should be returned instead of the initial requestedSize. |
There was a problem hiding this comment.
Probably should be:
NettyAllocationManager rounds requested size up to the next power of two.
jacques-n
left a comment
jacques-n
left a comment
There was a problem hiding this comment.
Looks good, thanks. Let's just update the one comment to be clearer as described below.
| /** | ||
| * Returns the underlying memory chunk size managed. | ||
| * | ||
| * <p>NettyAllocationManager manages memory chunk by the exact requested size. |
There was a problem hiding this comment.
Probably should be:
NettyAllocationManager rounds requested size up to the next power of two.
| public long getSize() { | ||
| return size; | ||
| } | ||
| protected abstract long getSize(); |
There was a problem hiding this comment.
Oh shoot, I just realized you're reducing visibility her on a public interface. You should keep this public. Sorry I missed this.
There was a problem hiding this comment.
oops. I was actually meaning to check in with you on this. (I think this interface should be protected) but sure we should keep it public for backward compatibility.
jacques-n
left a comment
jacques-n
left a comment
There was a problem hiding this comment.
lgtm +1. Thanks @walterddr!
jacques-n
left a comment
jacques-n
left a comment
There was a problem hiding this comment.
lgtm +1. Thanks @walterddr!
|
going to merge. |
This is a follow up on apache#5973. Closes apache#6204 from walterddr/ARROW-7538 and squashes the following commits: 15878d1 <Rong Rong> revert change and make API still public 10547bf <Rong Rong> address patch comment 97e7d3b <Rong Rong> address patch comments 489c203 <Rong Rong> fix checkstyle 1784f92 <Rong Rong> fix requested and allocated size difference Authored-by: Rong Rong <rongr@apache.org> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
4 participants
This is a follow up on #5973.