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
MemoryBackedStore cloned on every FileLoad::map #35
Comments
It is probably a good idea yes. While it was indeed primarily intended for tests, we are actually using the memory store in terminusdb right now, and I'm sure there's many use cases. The idea behind the extra clone here is that you get a snapshot view of whatever the contents of the in-memory 'file' is, at the time of resolving this future. There's probably better ways of doing this though. |
Ah, thanks, it's good to know that the clone was intended for a snapshot. Is there an important reason not to replace it with an active view of the data? |
It shouldn't be important. The data should not actually change, since all files are write-once, but this is not properly enforced by the types. |
I don't know the order of operations. Does the |
Consider this snippet of the implementation of the
FileLoad
trait forMemoryBackedStore
(src/storage/memory.rs
):In the expression
vec.read().unwrap().clone()
here, if my understanding is correct, it appears that theVec<u8>
underlying theRwLock
is being cloned.I understand that
MemoryBackedStore
may be primarily intended for testing and, therefore, for relatively small vectors. However, that may not always be the case, and I'd guess that a clone like this could result in excessive memory usage and possibly even a surprise out-of-memory error. (Then again, I could be blowing things out of proportion, pun intended! 💥)Would it be a good idea to avoid the
.clone()
here?The text was updated successfully, but these errors were encountered: