feat: remove in-memory SerializedModuleCache #136
Conversation
…rialized modules to filesystem into ModuleCache
| @@ -118,4 +168,56 @@ mod tests { | |||
| assert_eq!(*deserialized_cached_module, *module); | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn cache_get_from_fs() { | |||
There was a problem hiding this comment.
test that:
- builds module, serializes, writes to filesystem at expected path
- gets module from ModuleCache
- asserts that module now exists in deserialized in-memory path
| } | ||
|
|
||
| #[test] | ||
| fn cache_save_to_memory_only() { |
There was a problem hiding this comment.
Cache a module to in-memory deserialized module cache only
|
|
||
| #[test] | ||
| fn cache_test() { | ||
| fn cache_save_to_memory_and_fs() { |
There was a problem hiding this comment.
Cache a module to both filesystem and in-memory deserialized module caches
crates/host/src/module.rs
Outdated
| }; | ||
| self.put_item(key, Arc::new(serialized_module.clone())); | ||
| // Save serialized module to filesystem cache | ||
| self.add_to_filesystem_cache(key, serialized_module)?; |
There was a problem hiding this comment.
Note that we first save to filesystem, then add to in-memory cache. Thus if the process is interrupted after saving to the filesystem, it will still be added to the in-memory cache next time it is fetched.
But this made me realize -- if the process is interrupted while saving to the filesystem and the file is corrupted then it will error every time we try to deserialize it. I made an issue to address this: #137
There was a problem hiding this comment.
Sounds like we should be removing the file from disk if we load it and can't use it?
There was a problem hiding this comment.
Looking good. I like that in new code like the tests you've got just "in memory" and "file system" references. I'd go a little further and try to drop the serialized vs deserialized naming entirely. There is now one way to hold modules in memory and we have to serialize to save to storage. I think that's clear enough without having "deserialized" in the name. I see why it was there and that you've removed quite a few instances, I'm just thinking we can complete that cleanup.
Otherwise, the code is nice and clear :)
crates/host/src/module.rs
Outdated
| }; | ||
| self.put_item(key, Arc::new(serialized_module.clone())); | ||
| // Save serialized module to filesystem cache | ||
| self.add_to_filesystem_cache(key, serialized_module)?; |
There was a problem hiding this comment.
Sounds like we should be removing the file from disk if we load it and can't use it?
crates/host/src/module.rs
Outdated
| deserialized_module_cache: Arc<RwLock<DeserializedModuleCache>>, | ||
|
|
||
| // A function to create a new compiler engine for every module | ||
| make_engine: fn() -> Engine, |
There was a problem hiding this comment.
Having this in the cache is mixing concerns a little. I can't see the scope of this growing so I'm not too bothered. The caching and the production of working modules could be separated I think but if that's done later or never, then that's good with me :)
There was a problem hiding this comment.
Makes sense. I pulled out those fields and the calls that use them into a ModuleBuilder struct. Lmk what you think.
Co-authored-by: ThetaSinner <[email protected]>
Co-authored-by: ThetaSinner <[email protected]>
Co-authored-by: Jost <[email protected]>
Good call, done. Let me know what you think. |
Resolves holochain/holochain#3536
The diff is a hard to read but I think the code is more legible. Let me know if you think of more tests that would be good to add.