LakshSingla commented on code in PR #15987:
URL: https://github.com/apache/druid/pull/15987#discussion_r1512928070
##########
processing/src/main/java/org/apache/druid/frame/allocation/AppendableMemory.java:
##########
@@ -136,7 +136,7 @@ public boolean reserveAdditional(final int bytes)
// Allocation needed.
// Math.max(allocationSize, bytes) in case "bytes" is greater than
SOFT_MAXIMUM_ALLOCATION_SIZE.
final Optional<ResourceHolder<WritableMemory>> newMemory =
- allocator.allocate(Math.max(nextAllocationSize, bytes));
+ allocator.allocate(Math.min(allocator.available(),
Math.max(nextAllocationSize, bytes)));
Review Comment:
I'll explain with an example here, consider the following scenario:
* `nextAllocationSize` will be in multiples of 2, let's say for a particular
run of the dependable memory, it is 1024.
* Someone wants to allocate 600 bytes, i.e. `bytes = 600`
* Remaining memory in the allocator is 700, therefore `allocator.available()
= 700`
Since someone wants to allocate 600 bytes, and the available bytes in the
allocator are 700, the allocation should succeed, and we should be able to give
them 600 bytes (This is what Line 129 also checks)
However, if you look at the original code, it will do
`allocator.allocate(Math.max(1024, 600)` i.e. `allocator.allocate(1024)`
which would fail.
The new code will do
`allocator.allocate(Math.min(700, Math.max(1024, 600))`, i.e.
`allocator.allocate(700)`, which should pass (which is the required behavior).
`nextAllocationSize` is more like a "2x buffer allocator limit" - every time
we hit the limit of allocation, we multiply the next block to allocate by 2. So
we do allocations like 1, 2, 4, 8, 16, ...., to minimize the number of
allocations we need to do (and amortize the cost, as per my understanding). I
think a similar principle is applied when dictionaries do dynamic hashing.
Therefore even though the caller requests `bytes`, we give the caller a
larger block, so that the next time the caller requests bytes, we don't
reallocate again. However, this fails to take into account the edge case, that
even though the caller requests x bytes, and the allocator can satisfy that
condition, but not the condition for `nextAllocationSize`, we fail, even though
we should pass. Hence a cap of the allocation, and the available memory. In
normal cases, allocator.available >>>> Math.max(nextAllocationSize, bytes),
therefore most of the time the code should be doing what it's supposed to.
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
