search

Derecho-Project / derecho

The main code repository for the Derecho project.
BSD 3-Clause "New" or "Revised" License
186 stars 47 forks source link

Create a consistent API for from_bytes_noalloc functions #240

Closed etremel closed 2 years ago

etremel commented 2 years ago

This is one possible fix for the issue identified in #204. I changed all of the from_bytes_noalloc functions defined for STL types to have two versions, one "const" and one "non-const", that match the two versions of from_bytes_noalloc declared for ByteRepresentable types. This required some tinkering with the enable_if templates, and the way existing functions call from_bytes_noalloc, to get everything to compile, because the compiler is very pedantic about the difference between context_ptr<T> and context_ptr<const T>.

I'm not sure if these changes match the original intent of the from_bytes_noalloc API defined for ByteRepresentable types, but at least they solve the problem identified in #204. It is now possible to call deserialize_and_run on a function that accepts STL-container parameters (like std::vector), whereas before these changes deserialize_and_run only worked with ByteRepresentable parameters because of the way it called from_bytes_noalloc.

etremel commented 2 years ago

I actually already thought about point 1 while working on this, but I'm pretty sure it's already taken care of by the definition of context_ptr. If you look at context_ptr.hpp, it's just defining a unique_ptr with a custom deleter, and the custom deleter is defined like this:

https://github.com/Derecho-Project/derecho/blob/0664038a478daaa01991dd0edb0514f3e5ef3315/include/derecho/mutils-serialization/context_ptr.hpp#L7-L11

I think the effect of the std::conditional_t template here is that ContextDeleter will become std::default_delete<T> if the template parameter T is not a POD type. This means for non-POD types, context_ptr becomes exactly the same as unique_ptr, and deletes its managed object when it goes out of scope.

Regarding point 2, I think it doesn't really make sense to attempt to modify an object you got from from_bytes_noalloc, since it's supposed to be a "temporary view" of the object that still resides within the receive buffer and will disappear when the buffer is recycled. I would think that if you plan on modifying the object, you should use the regular from_bytes to get a "permanent" copy of it. So I'm a little confused as to why from_bytes_noalloc needs to support modifying the object in-place anyway, but I would be OK with only supporting the context_ptr<const T> version of the STL containers' from_bytes_noalloc.

songweijia commented 2 years ago

Thank you for the explanation. You are right, the implementation of context_ptr for the current will choose the ContextDeleter only for pod types, which means the context_ptr pointers DOES free the allocated memories for STL containers or other non-pod types. So, there is NO memory leak. My previous suspects were wrong.

For the second point, our current usage in the Derecho applications and in Cascade does not change the object returns from from_bytes_noalloc(). For example, in Cascade, if intensive computation on the object is passed from a remote RPC, we just handle a copy of it to the off-critical path. Therefore the in-place change does not make sense. Edward and I agree to disable the non-const from_bytes_noalloc() in the future. Right now, it will not cause any issue.