viirya commented on code in PR #58731:
URL: https://github.com/apache/spark/pull/58731#discussion_r4054499830
##########
core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java:
##########
@@ -103,9 +105,9 @@ public interface KeyOperationsFactory {
private final TaskMemoryManager taskMemoryManager;
/**
- * A linked list for tracking all allocated data pages so that we can free
all of our memory.
+ * A deque for tracking all allocated data pages so that we can free all of
our memory.
*/
- private final LinkedList<MemoryBlock> dataPages = new LinkedList<>();
+ private final Deque<MemoryBlock> dataPages = new ArrayDeque<>();
Review Comment:
The fail-fast note in the description has the direction backwards. Since
that paragraph was added in response to my earlier comment, I'd rather we get
it right -- it will end up in the commit message.
It currently says concurrent modification "may raise
`ConcurrentModificationException` rather than silently returning wrong pages",
which reads as `ArrayDeque` adding detection. For head removal -- exactly what
the destructive path does -- it actually *removes* detection that `LinkedList`
had (JDK 21):
```
// remove head while a stored iterator is live, 10 elements
LinkedList -> ConcurrentModificationException
ArrayDeque -> completes normally, yields all 10 including the removed
element
```
For tail removal both eventually throw, but `ArrayDeque` only notices at the
end of the walk, after already yielding stale elements, whereas `LinkedList`
throws immediately.
This is **not a correctness problem** -- `spill()` returns 0 for
non-destructive iterators and no production call site modifies the map
mid-walk, so neither shape is reachable. But the description should say this is
a reduction in best-effort detection inside an already-undefined region that
happens to be unreachable, rather than presenting it as an improvement. Could
you reword that bullet and the matching sentence under "Does this PR introduce
any user-facing change?"
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
