Conversation
There was a problem hiding this comment.
Pull request overview
该 PR 将 njson 的句柄池与缓存从“进程级全局状态”重构为“按 s7 VM 隔离的状态”，并在 C++ glue 层建立清晰的线程归属约束，以降低多 VM/并发场景下的串扰与竞态风险。
Changes:
- 引入
NjsonState与基于unordered_map<s7_scheme*, ...>的 per-VM 状态 registry，句柄池/generation/free-list/keys 缓存按 VM 隔离 - 在各
g_njson-*入口新增 owner-thread 检查（跨线程调用同一 VM 的 njson API 抛thread-error） - keys 缓存从
s7_pointer + gc_protect/unprotect改为vector<string>，命中时按需重建 Scheme list
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/goldfish.hpp | njson 状态隔离、线程归属校验、keys 缓存结构与相关 helper 签名调整 |
| devel/214_12.md | 记录重构动机、实现要点与测试/bench 验证方式 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static std::mutex njson_state_registry_mutex; | ||
| static std::unordered_map<s7_scheme*, std::unique_ptr<NjsonState>> njson_state_registry; | ||
|
|
||
| static NjsonState& | ||
| njson_get_or_create_state (s7_scheme* sc) { | ||
| std::lock_guard<std::mutex> lock (njson_state_registry_mutex); | ||
| auto it = njson_state_registry.find (sc); | ||
| if (it == njson_state_registry.end ()) { | ||
| auto inserted = njson_state_registry.emplace (sc, std::make_unique<NjsonState> ()); | ||
| return *(inserted.first->second); | ||
| } | ||
| return *(it->second); | ||
| } |
There was a problem hiding this comment.
njson_state_registry is keyed by raw s7_scheme* and entries are never removed. If a VM is ever torn down (or s7_scheme* addresses are reused), this can leak memory and potentially associate a new VM with stale handle/key-cache state. Consider adding an explicit njson_unregister_state(sc) (or a VM-destruction hook) to erase the registry entry when the VM is disposed, and ensure any teardown path calls it.
| NjsonState& state = njson_get_or_create_state (sc); | ||
| njson_clear_keys_cache_slot (sc, id); | ||
| njson_handle_store[static_cast<size_t> (id)].reset (); | ||
| njson_handle_free_ids.push_back (id); | ||
| state.handle_store[static_cast<size_t> (id)].reset (); | ||
| state.handle_free_ids.push_back (id); |
There was a problem hiding this comment.
f_njson_free fetches state via njson_get_or_create_state(sc) and then calls njson_clear_keys_cache_slot(sc, id), which also does its own njson_get_or_create_state(sc) lookup/lock. This redundant registry lookup adds overhead on a hot path. Consider refactoring helpers like njson_clear_keys_cache_slot (and similar) to take a NjsonState& (or otherwise reuse the already-fetched state) so each API call does a single registry lookup.
No description provided.