feat: Added a generic LRUCache interface and a default implementation #482
Conversation
| */ | ||
| package com.optimizely.ab.internal; | ||
|
|
||
| public interface LRUCache<T> { |
There was a problem hiding this comment.
| public interface LRUCache<T> { | |
| public interface Cache<T> { |
This interface can be for generic caches, not only for LRUCache. Not sure about the best name to avoid conflicts., but "LRU" should be dropped. I think this is for internal use (not for public custom segments cache). If this interface is for public, I have a few other suggestions.
There was a problem hiding this comment.
Changing it to Cache for now as per your suggestion. We can discuss this further in subsequent PRs when things become more clear.
| public void setMaxSize(Integer size) { | ||
| Integer sizeToSet = size; | ||
| if (size < 0 ) { | ||
| sizeToSet = DEFAULT_MAX_SIZE; |
There was a problem hiding this comment.
either way is fine, but let's use "sizeToSet = 0" for all SDKs.
There was a problem hiding this comment.
Done! Defaulting to zero now when max size is negative
| if (linkedList.size() > sizeToSet) { | ||
| // If cache is bigger than the new maxSize, remove additional items from the end. | ||
| int itemsToTrim = linkedList.size() - sizeToSet; | ||
| for (int i = 0; i < itemsToTrim; i++) { | ||
| ItemWrapper item = linkedList.removeLast(); | ||
| hashMap.remove(item.key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need this support for dynamic cache size changes? We can allow them cache size setting only when app starts.
There was a problem hiding this comment.
I changed it to only allow when maxSize passed in is less than the current size of the cache.
|
|
||
| if (hashMap.containsKey(key)) { | ||
| ItemWrapper oldItem = hashMap.get(key); | ||
| linkedList.remove(oldItem); |
There was a problem hiding this comment.
Isn't this linear search? It may be a factor in large cache.
There was a problem hiding this comment.
This is a good catch. Because i was passing the item itself and it uses a doubly linked list, i thought the item should have references to its neighbors and should be able to cleanly remove itself in O(1) but it looks like the actual node with references is managed internally and remove actually searched for the item linearly. I then went ahead and redid the whole implementation using LunkedHashMap
| } | ||
|
|
||
| @Test | ||
| public void saveAndLookupMultipleItems() { |
There was a problem hiding this comment.
This test looks good. It will be good to have the same ordering switch tests for save ops as well.
| return; | ||
| } | ||
|
|
||
| if (linkedList.size() == maxSize) { |
There was a problem hiding this comment.
| if (linkedList.size() == maxSize) { | |
| if (linkedList.size() >= maxSize) { |
There was a problem hiding this comment.
Changed the whole implementation. It does not apply anymore.
| } | ||
|
|
||
| @Test | ||
| public void whenMaxSizeIsReducedInBetween() { |
There was a problem hiding this comment.
Modified the test to check the changed implementation
| } | ||
|
|
||
| @Test | ||
| public void saveAndPeekItems() { |
| void setMaxSize(Integer size); | ||
| void setTimeout(Long timeoutSeconds); |
There was a problem hiding this comment.
We can set these in constructor. Can we remove them for consistency with other SDKs unless we have a strong case?
There was a problem hiding this comment.
Done! I also kept a no argument constructor too that will use default values.
| synchronized (lock) { | ||
| linkedHashMap.put(key, new ItemWrapper(value)); | ||
| } |
There was a problem hiding this comment.
Wow, a single-line solution :)
| } | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| public T peek(String key) { |
There was a problem hiding this comment.
It's surprising that they do not have this method in LinkedHashMap. It should be ok without this since it's mostly for testing/debugging.
Summary
Added an interface for a generic LRUCache and a default Implementation.
Test plan
Issues
OASIS-8385