Bug #11231: FastFindCache clear operations may leak memory - Database - Golden Code Redmine

Bug #11231

« Previous | Next »

FastFindCache clear operations may leak memory

Added by Alexandru Lungu 5 months ago. Updated 5 months ago.

Status:

Test

Priority:

Urgent

Assignee:

Artur Școlnic

Target version:

Start date:

Due date:

% Done:

100%

billable:

vendor_id:

GCD

case_num:

version_reported:

version_resolved:

reviewer:

Alexandru Lungu

production:

env_name:

topics:

History

#1 Updated by Alexandru Lungu 5 months ago

Priority changed from Normal to Urgent
Assignee set to Artur Școlnic

After refactoring FFCache to be driven by the ReverseLookup, so that there is only one unified LRU cache that controls the cached entities and "auto-balances" the FFCache to get more spaces to more intensively used tables, there might be a memory leak.

The map we use in the l3 map of ffcache won't be resized on clear. In other words, I see maps that have 131.072 slots in the l3 cache, but all are null. This is possible because, even if ReverseLookup expires entities and removes them from the L3 maps, the map table is just cleared and not resized.

Worst case scenario is that ReverseLookup is filled with >65536 (so the size is doubled to 131072) records of only one single temp-table. In a large customer application this happens from time to time when running reports that eventually have nested FIND on temp-tables that are constrained by an outer loop.

In a heap dump I see, there are ~100 such tables and there are 40 sessions. Roughly, there are 4GB of null slots reserved at the server side (only temp). The permanent database FFCache is 700MB out of the same considerations, but it has 440 tables active across all sessions.

This is problematic as this calculation that requires almost 5GB of empty memory for 40 sessions is an average case. Worst-case is that more temp-tables are loaded in such fashion, resulting in tens of GB potential memory consumption.

Unfortunately, Java doesn't have any granular access into map capacity, so we are forced so construct new maps instead of doing clear operations. I am afraid that this bug may affect customers with large number of parallel sessions that happen to do heavy work from time-to-time in without restarting their sessions (usually GUI sessions).

#2 Updated by Artur Școlnic 5 months ago

Status changed from New to WIP

#3 Updated by Artur Școlnic 5 months ago

Status changed from WIP to Review
% Done changed from 0 to 50
reviewer Alexandru Lungu added

I used the following heuristics to alleviate the issue:

On L3 clear –
When clearing an L3 map, check its current size against a defined threshold:
If the size is below the threshold, simply call clear() and retain the existing backing array.
If the size exceeds the threshold, recreate the map to release the oversized backing array.

On reverseCache remove –
After removals, evaluate whether the map has shrunk significantly relative to its previously observed peak size.
If the current size falls below a defined fraction of that peak (approximating a low load factor), recreate the map to shrink its backing array.
Since the actual backing array capacity is not directly accessible, the load factor is approximated by tracking the last observed maximum size of the map.

lruCache is bounded so it is more manageable (by reducing the max capacity).

The code is in 11231a.
Alexandru, please review.

#4 Updated by Alexandru Lungu 5 months ago

When clearing an L3 map, check its current size against a defined threshold:

I am concerned that the size represents the number of active elements, not the actual capacity. One could do a FIND (and add a record) and then change the record (invalidating the slot without calling clear). At some point, when clearing, the size may be small, but the capacity may be very big.

Unfortunately, we can't know what is the capacity of the map live, but we can keep track of it. After each put, the tracker would be computed as max(tracker, map.size()).
The logic you have is right IMHO, but it should use a tracker instead of the actual size(). Also, the tracker should be rounded to the lowest power of 2 bigger than tracker.

#5 Updated by Artur Școlnic 5 months ago

Alexandru Lungu wrote:

I am concerned that the size represents the number of active elements, not the actual capacity. One could do a FIND (and add a record) and then change the record (invalidating the slot without calling clear). At some point, when clearing, the size may be small, but the capacity may be very big.

You are right, I missed that. In this case we should overwrite the L3 caches always, for them it is not worth it to track the actual capacity.

#6 Updated by Constantin Asofiei 5 months ago

Please note this change which moved from 'new instance' to 'clear':

** 010 CA  20240924 Allow a L2/L3 cache to be cleared instead of discarding the instace, to help the garbage
**                  collector.

So whatever we do, we should not be very aggressive with creating new instances.

#7 Updated by Artur Școlnic 5 months ago

Alexandru, I added a new implementation for the Map that shrinks if the load factor is low enough and used it for ff caches.
Please review 11231a/16435.

#8 Updated by Alexandru Lungu 5 months ago

Review of 11231a:

implements Map<K, V> clause should be on a new line
MINIMAL_LOAD_FACTOR should be minimalLoadFactor as it is not static, but a class member that happens to be final.
map.putAll(m);, lastMaxSize should update lastMaxSize
clear will eventually do map = new WeakHashMap<>();. Why is it a weak hash map by default? maybe a flag on c'tor should be used to detect if weak or not should be used. Also, drop the possiblity to procive a custom map, because we will support only hashmap an weakhashmap. An arbitrary map will be lost when clearing.
- shrinkIfNeeded should account if it should create HashMap or WeakHashMap