镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

recalculate size when set() is given the same value again - #416

Open
OsamaAnsar wants to merge 1 commit into
isaacs:mainfrom
OsamaAnsar:fix-set-same-value-size
Open

OsamaAnsar wants to merge 1 commit into
isaacs:mainfrom
OsamaAnsar:fix-set-same-value-size

Conversation

@OsamaAnsar

Copy link
Copy Markdown

set(k, v) with the value already stored at k skipped the size update, because removeItemSize/addItemSize only run inside if (v !== oldVal). So after mutating an object and setting it again (or passing a different size for the same value), the stored size stayed stale. calculatedSize drifted from the real total and maxSize could be exceeded with nothing evicted. The maxEntrySize check already uses the new size, so this was also inconsistent.

Before (sizeCalculation: v => v.length, maxSize: 100, a grown from length 3 to 50 and set again, then b of length 60 added):

re-set same ref (len 50): calculatedSize = 3 (expected 50)
add b (len 60): keys = [ 'b', 'a' ] calculatedSize = 63 real total = 110
explicit size 2 -> 9 on same value: calculatedSize = 2 (expected 9)
control (new object instead of same one): calculatedSize = 50

After:

re-set same ref (len 50): calculatedSize = 50
add b (len 60): keys = [ 'b' ] calculatedSize = 60 real total = 60
explicit size 2 -> 9 on same value: calculatedSize = 9
control (new object instead of same one): calculatedSize = 50

The fix calls removeItemSize/addItemSize in the same-value branch too. Dispose, onInsert and TTL handling for that path are untouched. One visible side effect: the status of such a set() now has entrySize and totalCalculatedSize, as it already does for a replacing set(). That changes one existing entry in tap-snapshots/test/tracing.ts.test.cjs, plus new entries for the new tests.

Tests are in test/size-calculation.ts: mutated object with sizeCalculation (growing, shrinking, eviction order), explicit size on the same value (eviction of another entry, and removal when it exceeds maxSize), and no dispose on same-value set(). With only the source change reverted, 12 assertions in those 3 tests fail; with the fix they pass.

Checked on Node v24.19.0 on Windows: full tap -t0 run (coverage enforcement left on) passes with 0 failures, oxlint src test reports 0 warnings and 0 errors, and prettier --check is clean. I did not run Linux or macOS, other Node versions, or a browser; the change is plain arithmetic on the existing size bookkeeping. (On this machine I ran tap with a small preload that falls back to junctions for symlink, and installed @tapjs/clock with --no-save, see #413. Neither is part of this PR.)

If set(k, v) was called with the value already stored at k, the entry's
size was not updated, even if v had been mutated since (so that
sizeCalculation now returns something different), or a different
`size` option was given. calculatedSize then drifted away from the real
total, and maxSize could be exceeded without anything being evicted.

The update path now removes and re-adds the item size whether or not
the value is the same one. This also reports entrySize and
totalCalculatedSize in the status of such a set().

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant