[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9044: ARROW-11045: [Rust] Fix performance issues of allocator

[GitBox <gi...@apache.org>]

jorgecarleitao commented on a change in pull request #9044:
URL: https://github.com/apache/arrow/pull/9044#discussion_r550908549



##########
File path: rust/arrow/src/buffer.rs
##########
@@ -756,15 +738,15 @@ impl MutableBuffer {
     /// also ensure the new capacity will be a multiple of 64 bytes.
     ///
     /// Returns the new capacity for this buffer.
-    pub fn reserve(&mut self, capacity: usize) -> usize {
-        if capacity > self.capacity {
-            let new_capacity = bit_util::round_upto_multiple_of_64(capacity);
-            let new_capacity = cmp::max(new_capacity, self.capacity * 2);
+    pub fn reserve(&mut self, additional: usize) {

Review comment:
       I agree that it makes it more difficult. The signature is related to performance, though:
   
   It was difficult to reason with `reserve(new_len)` when `Vec::reserve` and `RawVec::reserve` uses `additional` (and I was basing this PR on that). This PR fixes a bug in one `Builder` that was calling `reserve(1)` in `append`, when it should be calling `reserve(self.len + 1)`. I also spent 4hs tracking a bug because of this difference (I was using `reserve(additional)` when its signature was `reserve(new_len)`.
   
   I can try to split that to another PR, though. IMO this PR will still need to be based on that PR.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org