mirkoalicastro commented on code in PR #22122:
URL: https://github.com/apache/kafka/pull/22122#discussion_r3247237530
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -189,8 +185,6 @@ public static Uuid[] toArray(List<Uuid> list) {
*/
public static List<Uuid> toList(Uuid[] array) {
if (array == null) return null;
- List<Uuid> list = new ArrayList<>(array.length);
- list.addAll(Arrays.asList(array));
- return list;
+ return new ArrayList<>(Arrays.asList(array));
Review Comment:
@smjn The problem that I see is that Uuid is a publicly-exposed class so
users can be using is in a mutable way in their projects. So, it would be a
**breaking change** making it immutable. I think better to avoid breaking
changes, especially for minor optimizations.
Regarding:
(1)
```
List<Uuid> list = new ArrayList<>(array.length);
list.addAll(Arrays.asList(array));
return list;
```
vs
(2)
```
return new ArrayList<>(Arrays.asList(array));
```
the (1) approach:
1. new ArrayList<>(array.length): allocates an array of array.length
2. Arrays.asList(array): creates a thin wrapper
3. list.addAll(array): internally calls `array.toArray()`, then copies it
with `System.arraycopy`
instead the (2) approach:
1. Arrays.asList(array): creates a thin wrapper (no element copy)
2. new ArrayList<>(array): internally calls `array.toArray()` and stores
the array
The (1) approach is a bit more expensive (it allocates the array upfront,
then overwrites it via addAll). Instead, (2) let the ArrayList constructor
directly use the toArray() result as its backing array.
--
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]
