refactor(metatable): replace parallel cache arrays with containers.Map#112
Merged
Conversation
Test Results265 tests 265 ✅ 27s ⏱️ Results for commit c6d6a14. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors MetaTable meta-object caching from parallel arrays to a single containers.Map, removing sync/listener machinery and switching to lazy eviction of stale handles.
Changes:
- Replace
MetaObjectCache/MetaObjectCacheMemberswith acontainers.Mapkeyed by normalized IDs. - Remove cache-member syncing and eviction listeners; detect & evict invalid cached handles on next access.
- Update the invalidation test to reflect lazy-eviction behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
code/+nansen/+metadata/@MetaTable/MetaTable.m |
Replaces cache storage with containers.Map, rewrites cache refresh/invalidation logic, and implements lazy eviction in getMetaObjects. |
tests/+nansen/+unittest/+metadata/MetaTableCacheInvalidationTest.m |
Updates a test name/doc to match lazy eviction and removes a warning-free delete step. |
.github/badges/tests.svg |
Updates the “tests passed” badge text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ehennestad
force-pushed
the
metatable-cache-map
branch
from
fa4b7c4 to
41a3ab1
Compare
MetaObjectCache and MetaObjectCacheMembers were a parallel-array pair requiring an explicit sync step (updateMetaObjectCacheMembers) after every mutation. Replace with a single containers.Map keyed by normalised ID so the ID→object relationship is the sole source of truth. Key changes: - MetaObjectCache is now a containers.Map (char → any), initialised in the constructor; MetaObjectCacheMembers property removed entirely. - updateMetaObjectCacheMembers, findCachedMetaObjectIndex removed — Map lookups (isKey/Map(id)/remove) replace the old linear-scan + rebuild. - Eviction listener (attachCacheEvictionListener / onMetaObjectDestroyed) removed in favour of lazy eviction: an invalid cached handle is evicted on the next getMetaObjects call rather than via an ObjectBeingDestroyed callback. Behaviour observed by callers is identical. - invalidateMetaObjectCache, all refreshMetaObjectCache* methods, and resetMetaObjectCache rewritten to use Map operations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename testDestroyingCachedObjectEvictsItFromCache to testDestroyingCachedObjectRebuildsCacheOnNextAccess and add a comment describing the lazy eviction mechanism: an invalidated cached handle is detected and evicted on the next getMetaObjects call, not at delete time. - Remove the delete(cachedItem) assertion from testRoutineCacheFlowsAreWarningFree. That line guarded against the ObjectBeingDestroyed listener emitting CacheMemberMissing; the listener was removed in the containers.Map refactor so the assertion was vacuous. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous wording said status indicates "if an object was created", which was inaccurate — cached objects also get status = true. The semantics are: true = valid object obtained (from cache or newly constructed); false = construction failed for that row. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- assignTableVariableAdded → applyAddTableVariable (clearer verb phrase) - assignTableVariableRemoved inlined into removeTableVariable (single call site, no reason for the indirection) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pattern Consistent event-responder naming across all four private helpers: refreshMetaObjectCacheProperty → refreshCacheOnPropertyChanged refreshMetaObjectCacheRows → refreshCacheOnRowsChanged refreshMetaObjectCacheVariableAdded → refreshCacheOnVariableAdded refreshMetaObjectCacheVariableRemoved → refreshCacheOnVariableRemoved Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…helpers - removeTableVariable now delegates to master when called on a dummy, fixing a bug where removal was never propagated (unlike addTableVariable which already did this correctly) - Extract applyAddTableVariableToMaster and applyRemoveTableVariableFromMaster as private methods; addTableVariable and removeTableVariable now hold the full local logic directly - Remove applyAddTableVariable (logic inlined into addTableVariable) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds testEditCellColumnRefreshesCachedProperty to guard against the bug
fixed in the previous commit: refreshCacheOnPropertyChanged was passing
the raw cell wrapper ({value}) to refreshProperty instead of the
unwrapped value.
- Add Tags = {} property to RefreshableTestItem (untyped; holds whatever
the table produces after unwrapping the cell-in-cell convention)
- Add Tags column to createRefreshableItemMetaTable using the scalar-cell
storage convention ({{'tag1'}; ...})
- Test verifies cachedItem.Tags equals the char 'newtag' (not {'newtag'})
after editEntries
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CellUtilTest covers isWrapped and unWrap directly — flat cells, nested scalar cells, multi-level nesting, and non-cell inputs. MetaTableCellColumnTest covers the assignEntries cell-column path: plain char input gets wrapped, already-wrapped input passes through, and doubly-wrapped input is unwrapped before storage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ehennestad
force-pushed
the
metatable-cache-map
branch
from
3392c15 to
9aa2dd6
Compare
Rename variables
Suppress output from remove method of containers.Map
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
MetaObjectCachearray +MetaObjectCacheMemberscell-array pair with a singlecontainers.Mapkeyed by normalised ID, making the ID→object relationship the sole source of truthupdateMetaObjectCacheMembers(the sync step that was required after every mutation) andfindCachedMetaObjectIndex(linear ID scan) — both made redundant by Map operationsattachCacheEvictionListener/onMetaObjectDestroyedmachinery in favour of lazy eviction: an invalid cached handle is detected and evicted on the nextgetMetaObjectscall rather than via anObjectBeingDestroyedcallbackWhy
The parallel-array design required an explicit sync step (
updateMetaObjectCacheMembers) after every cache mutation. This was called redundantly — once per cached object — inside therefreshMetaObjectCacheVariableAddedloop. With a Map the cache is self-consistent by construction, and all lookups are O(1) instead of O(N) linear scans.Test plan
runtests('nansen.unittest.metadata.MetaTableCacheInvalidationTest')— 18 tests, all passing with no changes to test code