Fix unit factor usage in Caffeine cache

[ben-manes]

Fix the calculation based on what @aseovic explained to me over slack. I had mistakenly thought this was to change the base unit of the entry's weight, e.g. from bytes to kilobytes, so that larger values could be stored. Instead it is to allow the total capacity to be reported as an int instead of a long, since that fix would be a breaking API change.

A minor cleanup was to allocate a CacheEvent instance only if it the cache will actually fire the event. Since MapListenerSupport#fireEvent is a synchronous call even if no listeners are registered (see collectListeners), the unsynchronized isEmpty() method is called as a guard. It's probably better to prefer avoiding the allocation over the convenience method as not much of a difference in readability.

[aseovic]

Actually, while the existing changes look good, I think some are still missing: getHighUnits and setHighUnits also need to take unit factor into account:

 @Override public int getHighUnits() { return (int) Math.min(f_eviction.getMaximum() / m_nUnitFactor, Integer.MAX_VALUE); } @Override public synchronized void setHighUnits(int units) { f_eviction.setMaximum(units * m_nUnitFactor); }

Without that it would be impossible to set high units to anything larger than 2 GB.

[ben-manes]

CaffeineCacheTest runs against both implementations (unfortunately the current unit factor test passes both with and without this pr). I think adding the high/low tests would help, and we can accumulate more for compatibility checks.

[ben-manes]

Updated the implementation by using the internal / external conversion methods from OldCache and added a unit test. This way the logic stays as similar as possible. It differs slightly because Caffeine avoids long overflow by restricting the maximum size to Long.MAX_VALUE - Integer.MAX_VALUE, rather than checking for that at runtime (whereas OldCache goes negative).

[coherence-bot]

Failed to process pull request
Failed to prepare P4 changelist (possibly a merge conflict).

[aseovic]

@rlubke Looks like we'll have to do a manual patch in P4. Automated PR merge still doesn't work... :-(

@thegridman This may be a good PR to use to sort out why the merge doesn't work any more. It's fairly simple, touches only 3 files that should be exactly the same in main and ce branches in P4.

[ben-manes]

Please remember to merge this.

[rlubke]

We haven't forgotten :)

We're working on it, just in the middle of some refactorings and don't want to muddy the waters at the moment. Will follow up as soon as it has been merged.

[rlubke]

PR has been merged