A DESCRIPTION OF THE PROBLEM :
The intent in the implementation is to keep the largest buffers in the cache by sorting the array in descending order. However the comparator does not do this and the cache always ends up empty.
```
return Integer.compare(getCapacity(br1), getCapacity(br2));
```
Should be:
```
return Integer.compare(getCapacity(br2), getCapacity(br1));
```
STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Copy-paste the class and add the code.
---------- BEGIN SOURCE ----------
static void main() {
var buffers = List.of(
getBuffer(4096L),
getBuffer(8192L),
getBuffer(16384L)
);
buffers.forEach(ImageBufferCache::releaseBuffer);
System.out.println(Arrays.toString(CACHE.get()));
}
---------- END SOURCE ----------
By inspection of the cache code, having the ordering reversed means that the cache insertion code always fails.
Since the CACHE array starts off with only nulls in:
---------- BEGIN SOURCE ----------
// insert buffer back with new BufferRef wrapping it
cache[MAX_CACHED_BUFFERS] = newCacheEntry(buffer); // <-- Add new element (with non zero capacity) to end of array.
Arrays.sort(cache, DECREASING_CAPACITY_NULLS_LAST); // <-- Incorrect sort leaves element where it is (all other elements are null).
// squeeze the smallest one out
cache[MAX_CACHED_BUFFERS] = null; // <-- Element is immediately removed again.
---------- END SOURCE ----------
So, because this bug guarantees the CACHE array never contains non-null elements, the cache has had no effect since the bug was introduced in May 2021.
As such, it has been decided that the cache class can be removed (since fixing it would entail re-activating otherwise dead code that has potentially subtle garbage collection semantics and would need careful testing).
The intent in the implementation is to keep the largest buffers in the cache by sorting the array in descending order. However the comparator does not do this and the cache always ends up empty.
```
return Integer.compare(getCapacity(br1), getCapacity(br2));
```
Should be:
```
return Integer.compare(getCapacity(br2), getCapacity(br1));
```
STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Copy-paste the class and add the code.
---------- BEGIN SOURCE ----------
static void main() {
var buffers = List.of(
getBuffer(4096L),
getBuffer(8192L),
getBuffer(16384L)
);
buffers.forEach(ImageBufferCache::releaseBuffer);
System.out.println(Arrays.toString(CACHE.get()));
}
---------- END SOURCE ----------
By inspection of the cache code, having the ordering reversed means that the cache insertion code always fails.
Since the CACHE array starts off with only nulls in:
---------- BEGIN SOURCE ----------
// insert buffer back with new BufferRef wrapping it
cache[MAX_CACHED_BUFFERS] = newCacheEntry(buffer); // <-- Add new element (with non zero capacity) to end of array.
Arrays.sort(cache, DECREASING_CAPACITY_NULLS_LAST); // <-- Incorrect sort leaves element where it is (all other elements are null).
// squeeze the smallest one out
cache[MAX_CACHED_BUFFERS] = null; // <-- Element is immediately removed again.
---------- END SOURCE ----------
So, because this bug guarantees the CACHE array never contains non-null elements, the cache has had no effect since the bug was introduced in May 2021.
As such, it has been decided that the cache class can be removed (since fixing it would entail re-activating otherwise dead code that has potentially subtle garbage collection semantics and would need careful testing).
- links to
-
Commit(master)
openjdk/jdk/7c979c14
-
Review(master)
openjdk/jdk/29043