Selki/ff3396 expiring bandit cache #128

maya-the-mango · 2024-11-12T12:22:33Z

labels: mergeable

Motivation and Context

//: # We want to be able to use a cache for bandits, that will expire after set ttl.

Description

//: # Created a TLRU cache implementation and injected it in EppoClient. Additionaly fixed an old unit test that was failing due to change in test data. Assert expected v123 actual data was 123

How has this been tested?

//: # Unit tests. See tlru-cache.spec.ts, tlru-in-memory-assignment-cache.spec.ts`.

src/cache/lru-cache.ts

src/cache/tlru-cache.ts

src/client/eppo-client.ts

src/cache/tlru-in-memory-assignment-cache.spec.ts

…anged default ttl of TLRU assignment cache.

felipecsl · 2024-11-12T21:25:02Z

src/cache/tlru-cache.ts

+    this.cacheEntriesTimoutIds.delete(key);
+  }
+
+  private setCacheEntryTimout(key: string): void {


typo: setCacheEntryTimeout

felipecsl · 2024-11-12T21:28:35Z

src/cache/tlru-cache.ts

+    // for eviction, because it is now considered most recently
+    // accessed thus the timer should start over. Not doing that
+    // will cause a de-sync that will stop proper eviction
+    this.clearCacheEntryTimeout(key);


I think we should move the statement above into the if scope below, since we should only clear the timeout if it was a cache hit

src/cache/tlru-cache.ts

felipecsl · 2024-11-12T21:30:47Z

src/cache/tlru-cache.ts

+  }
+
+  private setCacheEntryTimout(key: string): void {
+    const timeOutId = setTimeout(() => {


nit: timeoutId for consistentency with spelling of timeout in other places

src/cache/tlru-in-memory-assignment-cache.ts

felipecsl

LGTM! just one question

felipecsl · 2024-11-12T22:10:47Z

src/client/eppo-client.ts


  public useLRUInMemoryBanditAssignmentCache(maxSize: number) {
-    this.banditAssignmentCache = new LRUInMemoryAssignmentCache(maxSize);
+    this.banditAssignmentCache = new TLRUInMemoryAssignmentCache(maxSize);


did you mean to use TLRU in here as well? the method name says LRU

while "LRU" has meaning, I wonder if "TLRU" is too vauge and we should be expliti in our naming calling it `ExpiringLRUInMemoryAssignmentCache" or something

Also we should allow the timeout to be optionally specified here

I was also not conviced by the naming at first. I used ExpiringLRUInMemoryAssignmentCache in the beginning but then while reading about different caching algorithms. stumbled upon this.
https://en.wikipedia.org/wiki/Cache_replacement_policies#Time-aware,_least-recently_used

@aarsilv I will change the name if that doesn't convince you. What do you think?

aarsilv

This is a great start and I love the tests! However, I have performance concerns regarding the approach. Imagine they are handling tens of thousands of users we'd have tens of thousands of timer references out there which will have a larger memory footprint than just numbers, plus could do funky things to the event loop. I'm thinking a better approach would be to store the system time at which an entry expires (but not actually bothering to remove it until "just in time").

If that makes the LRU size inaccurate, we also could have a single garbage collecting timer that runs every minute or so.

src/cache/lru-in-memory-assignment-cache.ts

src/cache/tlru-cache.ts

src/cache/tlru-in-memory-assignment-cache.spec.ts

src/client/eppo-client-with-bandits.spec.ts

aarsilv · 2024-11-13T03:40:45Z

src/client/eppo-client.ts


  public useLRUInMemoryBanditAssignmentCache(maxSize: number) {
-    this.banditAssignmentCache = new LRUInMemoryAssignmentCache(maxSize);
+    this.banditAssignmentCache = new TLRUInMemoryAssignmentCache(maxSize);


while "LRU" has meaning, I wonder if "TLRU" is too vauge and we should be expliti in our naming calling it `ExpiringLRUInMemoryAssignmentCache" or something

aarsilv · 2024-11-13T03:41:09Z

src/constants.ts

 // number of logging events that may be queued while waiting for initialization
 export const MAX_EVENT_QUEUE_SIZE = 100;
 export const BANDIT_ASSIGNMENT_SHARDS = 10000;
+export const DEFAULT_TLRU_TTL_MS = 600_000;


aarsilv · 2024-11-13T03:41:47Z

src/client/eppo-client.ts


  public useLRUInMemoryBanditAssignmentCache(maxSize: number) {
-    this.banditAssignmentCache = new LRUInMemoryAssignmentCache(maxSize);
+    this.banditAssignmentCache = new TLRUInMemoryAssignmentCache(maxSize);


Also we should allow the timeout to be optionally specified here

aarsilv · 2024-11-13T03:42:41Z

src/client/eppo-client.ts

+   * @param {number} maxSize - Maximum cache size
+   * @param {number} timeout - TTL of cache entries
+   */
+  public useTLRUInMemoryAssignmentCache(maxSize: number, timeout?: number) {


This should be axed and timeout added to the previous method. We only want the expiring option for bandits

aarsilv · 2024-11-13T03:47:16Z

Note that for linear to automatically link and follow the ticket you need the hyphen in the branch name FF-3396

src/cache/tlru-cache.ts

aarsilv

Getting closer! Evicting all the expired entries is a lot of work so we should try to do it only when needed. I don't think we need to for get(), which can check just in time, and set(), which does not care. These two methods will make up the majority use case of this cache, so having them be fast is helpful.

src/cache/abstract-assignment-cache.spec.ts

src/cache/tlru-cache.spec.ts

src/cache/tlru-cache.ts

…es behaviour

aarsilv

Appologies we keep going back and forth here, but I have some concerns about the behavior of get(). I think it may not apply the "R" of LRU. I suggested a test to add and a possible change/simplification of get(). Also we need has() to check and delete based on expiration as well.

This task sounds so simple, but actually is quite complicated!

aarsilv · 2024-11-22T04:46:59Z

src/cache/tlru-cache.spec.ts

+      cacheKeys.push(key);
+    }
+
+    expect(cacheKeys.length).toBe(0);


probably good to leave one non-expired key in here to make sure it's still there

aarsilv · 2024-11-22T04:47:14Z

src/cache/tlru-cache.spec.ts

+      cacheValues.push(value);
+    }
+
+    expect(cacheValues.length).toBe(0);


aarsilv · 2024-11-22T04:48:02Z

src/cache/lru-cache.spec.ts

  });
+
+  /**
+   This test case might be an overkill but in case Map() changes,


probably handled by a test like your "should evict oldest entry when capacity limit is reached" but no harm no foul

src/cache/tlru-cache.ts

aarsilv · 2024-11-22T04:54:35Z

src/cache/tlru-cache.ts

+        this.delete(key);
+        return undefined;
+      }
+


otherwise, if valid, do we need to this.cache.set(key, value); to bump it to the bottom for the LRU like we do the parent class?

I wonder if we just check !this.isCacheEntryValid(key) first, if so delete it from the cache, then proceed to just call the parent method's get()

Great catch Aaron 🙏 Such a stupid mistake, my spirit with this cache implementation was crushed at the moment of writing this method 🫠 Your concerns saved my ass here!

I wonder if we just check !this.isCacheEntryValid(key) first, if so delete it from the cache, then proceed to just call the parent method's get()

I've rewrote it to do just that + reset cache TTL timer if we got a cache hit from parent class

aarsilv · 2024-11-22T04:56:56Z

src/cache/tlru-cache.spec.ts

+    const keys = Array.from(cache.keys());
+    expect(keys[0]).toBe('b');
+    expect(keys[1]).toBe('c');
+  });


I think we need another test that does:

cache.set('a', 'apple'); cache.set('b', 'banana'); cache.get('a'); cache.set('c', 'cherry');

Then makes sure its c and a (e.g., a got re-inserted at the end)

aarsilv

Thanks for these final changes! I think this is in a pretty good spot now 💪

Go ahead and bump the major patch number (middle number) in package.json in this PR before you merge. Then you, can make a release in GitHub. We can pair on that, or @typotter or @rasendubi could show you how.

Next step: have the upstream (currently: node) SDK use this for bandits!

aarsilv · 2024-11-23T00:49:13Z

src/cache/tlru-cache.spec.ts

+    expect(cacheKeys.length).toBe(1);
+    expect(cache.get('c')).toBe('cherry');


aarsilv · 2024-11-23T00:49:46Z

src/cache/tlru-cache.spec.ts

+    expect(cacheValues.length).toBe(1);
+    expect(cache.get('c')).toBe('cherry');


aarsilv · 2024-11-23T00:50:31Z

src/cache/tlru-cache.spec.ts

+    keys = Array.from(cache.keys());
+    expect(keys[0]).toBe('a');
+    expect(keys[1]).toBe('c');


# Conflicts: # src/client/eppo-client.ts

maya-the-mango added 5 commits November 8, 2024 16:42

Added Expiring LRU Cache implementation

d0093fb

Added Expiring LRU Cache implementation

ffc0c59

Reorganized caches into their own files

90429c1

fixed older unit tests

e4d1d26

renaming classes and file organizing

001ae6e

maya-the-mango mentioned this pull request Nov 12, 2024

Selki/ff3396 expiring bandit cache Eppo-exp/node-server-sdk#76

Closed

maya-the-mango requested review from felipecsl, rasendubi and dannymatkovsky November 12, 2024 12:26

rasendubi reviewed Nov 12, 2024

View reviewed changes

Fixed cache eviction for TLRU cache entries that were overwritten. Ch…

c386303

…anged default ttl of TLRU assignment cache.

maya-the-mango requested a review from rasendubi November 12, 2024 21:27

felipecsl reviewed Nov 12, 2024

View reviewed changes

minor code cleanliness improvements

e5c85cc

felipecsl approved these changes Nov 12, 2024

View reviewed changes

aarsilv requested changes Nov 13, 2024

View reviewed changes

rasendubi reviewed Nov 13, 2024

View reviewed changes

src/cache/tlru-cache.ts Outdated Show resolved Hide resolved

maya-the-mango added 2 commits November 18, 2024 13:21

memory optimization of tlru cache

4b02b3e

use better name for bandit cache method in EppoClient

4bdfa7f

maya-the-mango requested review from aarsilv and rasendubi November 18, 2024 13:27

fixed tests

d4a14bd

maya-the-mango mentioned this pull request Nov 19, 2024

Selki/ff 3514 bandit parameters polling only when needed #131

Merged

aarsilv requested changes Nov 19, 2024

View reviewed changes

added more test for tlru cache, changed get, set, entries, keys, valu…

1cf7391

…es behaviour

maya-the-mango requested a review from aarsilv November 21, 2024 15:50

aarsilv requested changes Nov 22, 2024

View reviewed changes

aarsilv assigned maya-the-mango Nov 22, 2024

fixed get() behaviour of tlru cache

64c6729

maya-the-mango requested a review from aarsilv November 22, 2024 16:32

aarsilv approved these changes Nov 23, 2024

View reviewed changes

maya-the-mango added 2 commits November 25, 2024 14:04

sdk version bump

1d1f7ee

Merge branch 'main' into selki/FF3396-expiring-bandit-cache

7336c3e

# Conflicts: # src/client/eppo-client.ts

maya-the-mango merged commit 7f838c2 into main Nov 25, 2024
8 checks passed

maya-the-mango deleted the selki/FF3396-expiring-bandit-cache branch November 25, 2024 14:21

maya-the-mango mentioned this pull request Dec 2, 2024

Selki/ff3396 expiring bandit cache Eppo-exp/node-server-sdk#80

Merged

		expect(cacheKeys.length).toBe(1);
		expect(cache.get('c')).toBe('cherry');

		expect(cacheValues.length).toBe(1);
		expect(cache.get('c')).toBe('cherry');

Selki/ff3396 expiring bandit cache #128

Selki/ff3396 expiring bandit cache #128

Uh oh!

Conversation

maya-the-mango commented Nov 12, 2024

labels: mergeable

Motivation and Context

Description

How has this been tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felipecsl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarsilv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarsilv commented Nov 13, 2024

Uh oh!

Uh oh!

aarsilv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aarsilv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment