ARROW-5386: [Java] Making the rounding behavior of the buffer capacity configurable

[liyafan82]

In our scenario, the following code snippet is frequent in our code base:

int requestSize = ...;
if (requestSize <= allocator.getLimit() - allocator.getAllocatedMemory()){
ArrowBuf buffer = allocator.buffer(requestSize);
}
However, it often causes OutOfMemoryException, due to Arrow's rounding behavior.

For example, we have only 12 MB memory left, and we request a buffer with size 10 MB. Appearantly, there is sufficient memory to meet the request. However, the rounding behavior rounds the request size from 10 MB to 16 MB, and there is no 16 MB memory, so an OutOfMemoryException will be thrown.

We propose two ways to solve this problem:

1. We provide a rounding option as an argument to the BaseAllocator#buffer method. There are two possible values for the rounding option: rounding up and rounding down. In the above scenario, the rounding down option can solve the problem.

2. We add a method to the allocator:

int getRoundedSize(final int size, BaseAllocator.AllocationRoundingOption roundingOption)

This method will give the rounded buffer size, given the initial request size. With this method, the user can freely adjust their request size to avoid OOM.

To make it more convenient to use Arrow, we implement both solutions in this PR.

[praveenbingo]

@liyafan82 some minor checkstyle fixes are needed, before we can merge.

[liyafan82]

@liyafan82 some minor checkstyle fixes are needed, before we can merge.

@praveenbingo Thank you so much for your review.
I have fixed the style issues. Hope it will pass the Travis CI build.

[codecov-io]

Codecov Report

Merging #4358 into master will increase coverage by 1.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4358      +/-   ##
==========================================
+ Coverage    88.3%   89.38%   +1.08%     
==========================================
  Files         780      639     -141     
  Lines       98400    88864    -9536     
  Branches     1251        0    -1251     
==========================================
- Hits        86889    79432    -7457     
+ Misses      11275     9432    -1843     
+ Partials      236        0     -236

Impacted Files	Coverage Δ
cpp/src/parquet/schema-test.cc	90.63% <0%> (-8.86%)	⬇️
cpp/src/arrow/flight/client.cc	92.27% <0%> (-2.03%)	⬇️
cpp/src/parquet/column_scanner.h	95.6% <0%> (-0.87%)	⬇️
cpp/src/arrow/flight/server.cc	90.06% <0%> (-0.78%)	⬇️
python/pyarrow/_parquet.pyx	90.85% <0%> (-0.35%)	⬇️
cpp/src/arrow/flight/internal.cc	91.3% <0%> (-0.23%)	⬇️
cpp/src/plasma/test/external_store_tests.cc	100% <0%> (ø)	⬆️
cpp/src/parquet/reader-test.cc	100% <0%> (ø)	⬆️
python/pyarrow/cuda.py	100% <0%> (ø)	⬆️
python/pyarrow/ipc.py	100% <0%> (ø)	⬆️
... and 207 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81e4013...5b3179a. Read the comment docs.

[emkornfield]

@liyafan82 thanks for cleaning up the other javadoc. I don't think its strictly necessary to fixup the methods you aren't touching (and might be better for a separate review).

[liyafan82]

@liyafan82 thanks for cleaning up the other javadoc. I don't think its strictly necessary to fixup the methods you aren't touching (and might be better for a separate review).

@emkornfield Sounds reasonable. I have reverted them back.

[emkornfield]

I think it is this part that doesn't make sense to me. Won't clients get an uexpected indexoutofboundsexception if they are expecting a request a buffer or a larger size but get one of the smaller size?

[liyafan82]

@emkornfield Thanks for the good point. I think I understand what you are talking about.

I think the fundamental reason here, is not that the user gets a smaller buffer unexpectedly. To get a smaller buffer, the user must have specified the rounding down option explicitly.

I think the fundamental reason is that, the actual buffer size may differ from the requested buffer size, and neglecting this fact may cause problems.

One of the problems is what you have mentioned in the comment (the problem of throwing IndexOutOfBoundException). Another problem is what I have discussed in the JIRA description (the problem of throwing OutOfMemoryException).

Yet another problem is resource waste: For example, I am requesting a buffer of 10 MB, and by the default rounding up option, I get a buffer of 16 MB. Then I go ahead using the buffer, as if it has only 10 MB. This will waste the extra 6 MB memory space.

So if the user does not respect the fact that the requested and actual buffer sizes can be different, I guess there is no perfect solution for all these problems.

However, if the user respects this fact, the perfect solution cannot be achieved either, without the options provide by this PR. So this PR makes the perfect solution possible.

[emkornfield]

@liyafan82 thanks for the explanation. The thing that seems strange to me is that this is different then the typical memory allocator abstraction. For instance with malloc, at least as far as I'm aware you, don't have this level of control, it is up to the caller to make the decision on how much memory to ask for and the allocator can either satisfy it or it can't. And different memory managers are better at worse and handling internal fragmentation which it seem like the problem here.

I agree it is surprising given the code that you shared, if you request a small amount of memory that appears to be within bounds to get an OOM unexpectedly.

As an alternative proposal, off the top of my head, what do you think of something like this API?

/**
 Determines if the allocation is likely to succeed (it accounts for 
 * for implementation details like padding buffers to certain lengths).
 */ 
bool canProbablyAllocate(int byteCount);

Then you can write the code in the JIRA as something like:

if (!allocator.canProbablyAllocate(requestSize)) {
   // Try making a smaller request size to avoid O
   requestSize = PreviousPowerOf2(requestSize);
}
buffer = allocator.allocate(requestSize);

The if statement could be placed into a static helper method someplace if it is a very common pattern.

It seems like we can then have different allocators that can have different internal fragmentation levels that make it more likely to succeed as long as there the request is within the different of memory allocated and their memory limit.

@praveenbingo thoughts?

[emkornfield]

P.S. Even canProbablyAllocate seems like it might be superfluous, is there a reason you can't catch the OOM and make a smaller request size? Performance?

[liyafan82]

@emkornfield thanks a lot for your good proposal and good questions.

You proposal aims at solving the rounding problem when the request size is smaller than the chunk size. However, we do not know the chunk size. AllocationManager#CHUNK_SIZE has package access level, and its value is platform dependent. This is the first problem we are facing.

Even if we can get the chunk size, say, by reflection. We are facing the second, maybe more important problem. The retry logic above depends on the buffer allocation policy of Arrow. To comply with the principle of encapsulation and reduce the cohesion between Arrow and external code, I don't think the internal logic of Arrow should be exposed to external users, and the external code should not rely on the internal details of Arrow. The only assumption it may have is that, the requested and actual buffer sizes can be different.

This is because if one day, the allocation policy changes, all the user code would break (if the code depends on the allocation details). In fact, we are thinking about a proposal for alternative memory allocation policies, like the buffer size must be a multiple of some unit (e.g. 32 KB memory segment). Powers of 2 can cause too much waste for some scenarios. In this proposal, there is not a threshold like the chunk size, and the rounded size may not be a power of 2.

So we think the retry logic above can be a good work around, but it may not be reliable in the long run, and this is why we open this issue.

BTW, I am curious if it is possible for malloc to allocate a buffer with size larger than the requested size? Is there a rule regulating the requested and actual buffer size for malloc?

Thank you in advance.

[liyafan82]

@emkornfield , thanks for your effort in understanding our proposal.

I agree with you that we should be careful when adding each new API. In addition, I believe in your expertise in memory management. So I do not want this PR to be merged, before we can reach an agreement.

How about we solve this issue like this?

1. We remove the option of rounding up and rounding down from this PR, because we need to spend more time thinking about it. Maybe we can start a discussion in the community, and maybe we can find a better solution in the future.

2. We only preserve the option of providing the rounded buffer size. This is a relatively conservative solution, which does not affect the internal behavior of the allocator, and it will help users avoid OOM in their code. Given a buffer request, the users will know how much memory he/she is really asking for.

What do you think? If you think it is OK, I will revise the code accordingly.

Thank you in advance.

[praveenbingo]

@emkornfield @liyafan82 sorry for the delay.

I agree with @emkornfield suggestion, it makes for a cleaner API and has no surprises since the client is asking for exactly the amount of memory that is required and Arrow is not reducing it in anyway.
We can abstract the retry loop in an utility method if needed.

[liyafan82]

@praveenbingo thank you for your comment. It seems rounding down the requested buffer size is not a good idea.

@praveenbingo and @emkornfield I have updated the PR and only preserve the logic for getting the rounded buffer size. Would you please help to see if it looks good to you?

[emkornfield]

@liyafan82 I'm still concerned that the API might not be deterministic given other implementations of the API. Again it might be OK, I just don't feel comfortable merging the change. Could you please discuss on the ML?

Thank you.

[liyafan82]

@emkornfield Sure. Thanks for the suggestion.
I will start a discussion on the ML.

[emkornfield]

@liyafan82 would you be OK closing the PR given #4400 is merged and there wasn't much interest one way or another on the mailing list?

[liyafan82]

@liyafan82 would you be OK closing the PR given #4400 is merged and there wasn't much interest one way or another on the mailing list?

Agreed, #4400 provides a solution for this issue. So this JIRA can also be closed.