-
Notifications
You must be signed in to change notification settings - Fork 61k
feat(core): Replace unbounded expression code cache with LRU #27477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7f097bd
feat(core): Replace unbounded expression code cache with LRU
ivov 9426ba7
refactor: Remove premature optimization
ivov b21fdc1
fix(core): allow configurable vm evaluator timeout for benchmarks
despairblue a4b7f00
fix(core): emit cache size gauge reset on evaluator disposal
despairblue 648aea9
test(core): replace manual mockClear cast with vi.clearAllMocks()
despairblue 2e99c22
test(core): add tests for expression.code_cache.size gauge emissions
despairblue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
112 changes: 112 additions & 0 deletions
112
packages/@n8n/expression-runtime/src/evaluator/__tests__/expression-evaluator-cache.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { ExpressionEvaluator } from '../expression-evaluator'; | ||
| import type { RuntimeBridge, ObservabilityProvider } from '../../types'; | ||
|
|
||
| function createMockBridge(): RuntimeBridge { | ||
| return { | ||
| initialize: vi.fn().mockResolvedValue(undefined), | ||
| execute: vi.fn().mockReturnValue('result'), | ||
| dispose: vi.fn().mockResolvedValue(undefined), | ||
| isDisposed: vi.fn().mockReturnValue(false), | ||
| }; | ||
| } | ||
|
|
||
| function createMockObservability(): ObservabilityProvider { | ||
| return { | ||
| metrics: { | ||
| counter: vi.fn(), | ||
| gauge: vi.fn(), | ||
| histogram: vi.fn(), | ||
| }, | ||
| traces: { | ||
| startSpan: vi.fn().mockReturnValue({ | ||
| setStatus: vi.fn(), | ||
| setAttribute: vi.fn(), | ||
| recordException: vi.fn(), | ||
| end: vi.fn(), | ||
| }), | ||
| }, | ||
| logs: { | ||
| error: vi.fn(), | ||
| warn: vi.fn(), | ||
| info: vi.fn(), | ||
| debug: vi.fn(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| describe('ExpressionEvaluator cache', () => { | ||
| let bridge: RuntimeBridge; | ||
| let observability: ObservabilityProvider; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| bridge = createMockBridge(); | ||
| observability = createMockObservability(); | ||
| }); | ||
|
|
||
| it('should emit cache miss on first evaluation', async () => { | ||
| const evaluator = new ExpressionEvaluator({ bridge, observability, maxCodeCacheSize: 1024 }); | ||
| await evaluator.initialize(); | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| expect(observability.metrics.counter).toHaveBeenCalledWith('expression.code_cache.miss', 1); | ||
| }); | ||
|
|
||
| it('should emit cache hit on repeated evaluation', async () => { | ||
| const evaluator = new ExpressionEvaluator({ bridge, observability, maxCodeCacheSize: 1024 }); | ||
| await evaluator.initialize(); | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| expect(observability.metrics.counter).toHaveBeenCalledWith('expression.code_cache.hit', 1); | ||
| }); | ||
|
|
||
| it('should emit eviction when cache is full', async () => { | ||
| const evaluator = new ExpressionEvaluator({ | ||
| bridge, | ||
| observability, | ||
| maxCodeCacheSize: 2, | ||
| }); | ||
| await evaluator.initialize(); | ||
| evaluator.evaluate('={{ $json.a }}', {}); | ||
| evaluator.evaluate('={{ $json.b }}', {}); | ||
| evaluator.evaluate('={{ $json.c }}', {}); // evicts first | ||
| expect(observability.metrics.counter).toHaveBeenCalledWith('expression.code_cache.eviction', 1); | ||
| }); | ||
|
|
||
| it('should work without observability', async () => { | ||
| const evaluator = new ExpressionEvaluator({ bridge, maxCodeCacheSize: 1024 }); | ||
| await evaluator.initialize(); | ||
| expect(() => { | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| it('should emit cache size gauge on cache miss', async () => { | ||
| const evaluator = new ExpressionEvaluator({ bridge, observability, maxCodeCacheSize: 1024 }); | ||
| await evaluator.initialize(); | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| expect(observability.metrics.gauge).toHaveBeenCalledWith('expression.code_cache.size', 1); | ||
| }); | ||
|
|
||
| it('should emit cache size gauge of 0 on dispose', async () => { | ||
| const evaluator = new ExpressionEvaluator({ bridge, observability, maxCodeCacheSize: 1024 }); | ||
| await evaluator.initialize(); | ||
| evaluator.evaluate('={{ $json.email }}', {}); | ||
| vi.clearAllMocks(); | ||
| await evaluator.dispose(); | ||
| expect(observability.metrics.gauge).toHaveBeenCalledWith('expression.code_cache.size', 0); | ||
| }); | ||
|
|
||
| it('should evict least recently used and report miss on re-access', async () => { | ||
| const evaluator = new ExpressionEvaluator({ bridge, observability, maxCodeCacheSize: 2 }); | ||
| await evaluator.initialize(); | ||
| evaluator.evaluate('={{ $json.a }}', {}); | ||
| evaluator.evaluate('={{ $json.b }}', {}); | ||
| evaluator.evaluate('={{ $json.c }}', {}); | ||
| expect(observability.metrics.counter).toHaveBeenCalledWith('expression.code_cache.eviction', 1); | ||
| vi.clearAllMocks(); | ||
| evaluator.evaluate('={{ $json.a }}', {}); | ||
| expect(observability.metrics.counter).toHaveBeenCalledWith('expression.code_cache.miss', 1); | ||
| }); | ||
| }); |
104 changes: 104 additions & 0 deletions
104
packages/@n8n/expression-runtime/src/evaluator/__tests__/lru-cache.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| import { describe, it, expect, vi } from 'vitest'; | ||
| import { LruCache } from '../lru-cache'; | ||
|
|
||
| describe('LruCache', () => { | ||
| it('should store and retrieve values', () => { | ||
| const cache = new LruCache<string, number>(3); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| expect(cache.get('a')).toBe(1); | ||
| expect(cache.get('b')).toBe(2); | ||
| }); | ||
|
|
||
| it('should return undefined for missing keys', () => { | ||
| const cache = new LruCache<string, number>(3); | ||
| expect(cache.get('missing')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should evict the least recently used entry when over capacity', () => { | ||
| const cache = new LruCache<string, number>(2); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.set('c', 3); // evicts 'a' | ||
| expect(cache.get('a')).toBeUndefined(); | ||
| expect(cache.get('b')).toBe(2); | ||
| expect(cache.get('c')).toBe(3); | ||
| }); | ||
|
|
||
| it('should refresh recency on get', () => { | ||
| const cache = new LruCache<string, number>(2); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.get('a'); // refreshes 'a', now 'b' is oldest | ||
| cache.set('c', 3); // evicts 'b' | ||
| expect(cache.get('a')).toBe(1); | ||
| expect(cache.get('b')).toBeUndefined(); | ||
| expect(cache.get('c')).toBe(3); | ||
| }); | ||
|
|
||
| it('should refresh recency on get even when below capacity', () => { | ||
| const cache = new LruCache<string, number>(3); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.get('a'); // refreshes 'a' while cache is below capacity | ||
| cache.set('c', 3); // fills cache | ||
| cache.set('d', 4); // evicts 'b' (oldest), not 'a' | ||
| expect(cache.get('a')).toBe(1); | ||
| expect(cache.get('b')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should refresh recency on set (update)', () => { | ||
| const cache = new LruCache<string, number>(2); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.set('a', 10); // refreshes 'a', now 'b' is oldest | ||
| cache.set('c', 3); // evicts 'b' | ||
| expect(cache.get('a')).toBe(10); | ||
| expect(cache.get('b')).toBeUndefined(); | ||
| expect(cache.get('c')).toBe(3); | ||
| }); | ||
|
|
||
| it('should call onEvict with evicted key and value', () => { | ||
| const onEvict = vi.fn(); | ||
| const cache = new LruCache<string, number>(2, onEvict); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.set('c', 3); // evicts 'a' | ||
| expect(onEvict).toHaveBeenCalledWith('a', 1); | ||
| }); | ||
|
|
||
| it('should not evict when updating existing keys', () => { | ||
| const onEvict = vi.fn(); | ||
| const cache = new LruCache<string, number>(3, onEvict); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.set('a', 10); // update, not a new entry | ||
| cache.set('c', 3); | ||
| expect(onEvict).not.toHaveBeenCalled(); | ||
| expect(cache.get('a')).toBe(10); | ||
| expect(cache.get('b')).toBe(2); | ||
| expect(cache.get('c')).toBe(3); | ||
| }); | ||
|
|
||
| it('should clear all entries', () => { | ||
| const cache = new LruCache<string, number>(3); | ||
| cache.set('a', 1); | ||
| cache.set('b', 2); | ||
| cache.clear(); | ||
| expect(cache.get('a')).toBeUndefined(); | ||
| expect(cache.get('b')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should work with capacity 1', () => { | ||
| const cache = new LruCache<string, number>(1); | ||
| cache.set('a', 1); | ||
| expect(cache.get('a')).toBe(1); | ||
| cache.set('b', 2); // evicts 'a' | ||
| expect(cache.get('a')).toBeUndefined(); | ||
| expect(cache.get('b')).toBe(2); | ||
| }); | ||
|
|
||
| it('should throw for capacity less than 1', () => { | ||
| expect(() => new LruCache<string, number>(0)).toThrow('capacity must be at least 1'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
38 changes: 38 additions & 0 deletions
38
packages/@n8n/expression-runtime/src/evaluator/lru-cache.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| export class LruCache<K, V> { | ||
| private map = new Map<K, V>(); | ||
|
|
||
| constructor( | ||
| private readonly capacity: number, | ||
| private readonly onEvict?: (key: K, value: V) => void, | ||
| ) { | ||
| if (capacity < 1) { | ||
| throw new Error('LruCache capacity must be at least 1'); | ||
| } | ||
| } | ||
|
|
||
| get(key: K): V | undefined { | ||
| const value = this.map.get(key); | ||
| if (value === undefined) return undefined; | ||
| this.map.delete(key); | ||
| this.map.set(key, value); | ||
| return value; | ||
| } | ||
|
|
||
| set(key: K, value: V): void { | ||
| this.map.delete(key); | ||
| this.map.set(key, value); | ||
| if (this.map.size > this.capacity) { | ||
| const [oldestKey, oldestValue] = this.map.entries().next().value!; | ||
| this.map.delete(oldestKey); | ||
| this.onEvict?.(oldestKey, oldestValue); | ||
| } | ||
| } | ||
|
|
||
| get size(): number { | ||
| return this.map.size; | ||
| } | ||
|
|
||
| clear(): void { | ||
| this.map.clear(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.