pyld icon indicating copy to clipboard operation
pyld copied to clipboard

Ensure that ActiveContextCache only enters a given combination of (activeCtx, localCtx) once in its internal FIFO

Open kacarstensen-shift opened this issue 9 years ago • 1 comments

If ActiveContextCache.set is called multiple times during processing with a particular combination of activeCtx and localCtx, ActiveContextCache.order gets out of sync with ActiveContextCache.cache (i.e., multiple copies of a particular key combination can exist in order even though only one copy can be cached at any given time). This causes issues later in the cache's lifecycle when it begins to evict previously set elements. The first instance of a given (activeCtx, localCtx) found in ActiveContextCache.order will be evicted from ActiveContextCache.cache successfully, but subsequent copies will cause in a KeyError. To resolve this, I made inclusion in ActiveContextCache.order conditional on the (activeCtx, localCtx) combination not already existing in ActiveContextCache.cache. This keeps the two data structures are in sync and prevents the KeyErrors.

kacarstensen-shift avatar Nov 16 '15 23:11 kacarstensen-shift

I'm surprised this PR has been open for this long.

I ran into the same issue, though I fixed it by changing set to:

def set(self, active_ctx, local_ctx, result):
        if len(self.order) == self.size:
            entry = self.order.popleft()
            if sum(
                e['activeCtx'] == entry['activeCtx'] and
                e['localCtx'] == entry['localCtx'] for e in self.order
            ) == 0:
                # only delete from cache if it doesn't exist in context deque
                del self.cache[entry['activeCtx']][entry['localCtx']]
        key1 = json.dumps(active_ctx)
        key2 = json.dumps(local_ctx)
        self.order.append({'activeCtx': key1, 'localCtx': key2})
        self.cache.setdefault(key1, {})[key2] = json.loads(json.dumps(result))

So only remove an entry from cache if it's not anywhere in the deque, this way contexts don't get evicted from cache if they're still used somewhere else, but the deque itself is still limited to self.size number of entries.

Panaetius avatar Nov 14 '19 07:11 Panaetius