[FLINK-32896] Incorrect `Map.computeIfAbsent(..., ...::new)` usage which misinterprets key as initial capacity

• Type: Bug
• Status: Resolved
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 1.18.0, 1.17.1
• Fix Version/s: 1.19.0
• Component/s: Runtime / Coordination
• Labels: pull-request-available starter
• Assignee: ZY tang
• Reporter: Marcono1234
• Created: 19/Aug/23 21:55
• Updated: 25/Oct/23 07:36
• Resolved: 24/Oct/23 09:34

Description

The are multiple cases in the code which look like this:

map.computeIfAbsent(..., ArrayList::new)

Not only does this create a new collection (here an ArrayList), but computeIfAbsent also passes the map key as argument to the mapping function, so instead of calling the no-args constructor such as new ArrayList<>(), this actually calls the constructor with int initial capacity parameter, such as new ArrayList<>(initialCapacity).

For some cases where this occurs in the Flink code I am not sure if it is intended, but in same cases this does not seem to be intended. Here are the cases I found:

• HsFileDataIndexImpl:163
• HsSpillingStrategy:128
• HsSpillingStrategy:134
• HsSpillingStrategy:140
• HsSpillingStrategy:145
• HsSpillingStrategy:151
• HsSpillingStrategy:157
• HsSpillingStrategyUtils:76
• ProducerMergedPartitionFileIndex:171
• TestingSpillingInfoProvider:201
• TestingSpillingInfoProvider:208
• TestingSpillingInfoProvider:216
• SourceCoordinatorConcurrentAttemptsTest:269

This can lead either to runtime exceptions in case the map key is negative, since the collection constructors reject negative initial capacity values, or it can lead to bad performance if the key (which is misinterpreted as initial capacity) is pretty low, such as 0, or is pretty large and therefore pre-allocates a lot of memory for the collection.

Regardless of whether or not these cases are intended it might be good to replace them with lambda expressions to make this more explicit:

map.computeIfAbsent(..., capacity -> new ArrayList<>(capacity))

or

map.computeIfAbsent(..., k -> new ArrayList<>())

Issue Links

• links to: GitHub Pull Request #23518