ARROW-6060: [C++] ChunkedBinaryBuilder should only grow when necessary, address runaway memory use in Parquet binary column read #5016
Conversation
|
Can you add a unit test to verify the desired behavior? |
Codecov Report
@@ Coverage Diff @@
## master #5016 +/- ##
=========================================
Coverage ? 89.17%
=========================================
Files ? 727
Lines ? 102887
Branches ? 0
=========================================
Hits ? 91754
Misses ? 11133
Partials ? 0
Continue to review full report at Codecov.
|
| for (int i = 0; i < 8; ++i) { | ||
| ASSERT_OK(builder_->Reserve(chunksize / 2)); | ||
| } | ||
| // no new memory will be allocated since capacity was sufficient for the loop's |
There was a problem hiding this comment.
How is that? We're reserving 4.5 * chunksize total, no?
There was a problem hiding this comment.
In between Reserve calls, no data is appended to the builder, so there is no need to allocate any additional memory.
If you look at the bug report in ARROW-6060 the issue appears to be that memory was being allocated when there was no need.
There was a problem hiding this comment.
To review the docstring for Reserve
/// \brief Ensure that there is enough space allocated to add the indicated
/// number of elements without any further calls to Resize. Overallocation is
/// used in order to minimize the impact of incremental Reserve() calls.
///
/// \param[in] additional_capacity the number of additional array values
/// \return Status
So if there is already enough memory allocated then Reserve should be a no-op
There was a problem hiding this comment.
(it might be worth stating in the docstring that calls to Reserve are not cumulative)
There was a problem hiding this comment.
(it might be worth stating in the docstring that calls to Reserve are not cumulative)
Really? They are. It's the (confusing) difference between Resize and Reserve.
There was a problem hiding this comment.
This is a good opportunity to clarify the docstring if there is still confusion.
Reserve: pre-allocate space for an absolute, additional amount of data (prior calls to Reserve are not taken into consideration, so Reserve(1000)->Reserve(2000) is the same as Reserve(2000))
Resize: pre-allocate space for an absolute amount of data (regardless of how much has already been appended)
As we've seen here getting the behavior of Reserve wrong has made the 0.14.x release basically unusable for a number of users, so there are consequences to this
There was a problem hiding this comment.
@bkietz is it possible to have a more aggressive test, e.g. one that uses 16mb in the normal case and 16g in the falling one?
There was a problem hiding this comment.
@fsaintjacques in the failing case to balloon to 16G all I'd need to do is increase the loop max; the calls to Reserve would continue increasing capacity by the growth factor (1.5) until we run out of memory. I'm not sure what merit increasing the test's scale has
There was a problem hiding this comment.
Agreed with @bkietz. We don't need to test to crash, just fail, if Reserve() is buggy.
wesm
changed the title
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
+1, LGTM. Will merge if CI green.
|
I checked the memory use script from the JIRA issue and peak memory use is back in line with expectations |
5 participants
@wesm ChunkedBinaryBuilder was increasing capacity by the growth factor on every call to Reserve. Now it will only do so if added capacity is required