Send safe roc list #4361
Send safe roc list #4361
Conversation
crates/roc_std/src/roc_list.rs
Outdated
| } | ||
|
|
||
| // Marks a list as readonly. This means that it will be leaked. | ||
| // For constants passed in from platform to application, this may be reasonable. |
There was a problem hiding this comment.
isn't this the other way around? It's a bit of memory passed from the application to the platform?
There was a problem hiding this comment.
Oh, so this has a very specific use case in mind. I have a Rust host. It spawns a task/future/thread per request. There is a constant Str or List that I want to pass into my Roc function on every request. I do not want to allocate and deallocate on every single one of these calls. That would just be a waste.
Instead, I create a RocStr. I set it as readonly. I then share it between as many threads as I want, cloning the pointer, capacity, and length, rather than cloning the entire pointed to Str and calling malloc.
I specifically ran into this with a web server. Every request, I want to pass in the base url of my website. As a result, there was an extra malloc and free for every request that I would rather avoid if possible.
If something is passed from application to host, I would generally not expect that this function would get used, but I guess you might have uses.
There was a problem hiding this comment.
oh, interesting! That makes a lot of sense.
crates/roc_std/src/roc_list.rs
Outdated
|
|
||
| // Marks a list as readonly. This means that it will be leaked. | ||
| // For constants passed in from platform to application, this may be reasonable. | ||
| // Marked unsafe because it should not be used lightly. |
There was a problem hiding this comment.
doesn't "unsafe" in Rust mean memory unsafety? How could this lead to that? If anything, it's making things a little safer by avoiding in-place mutation for the rest of this list's life?
There was a problem hiding this comment.
It is memory unsafe, at least in my opinion. It leads to a memory leak, 100% guaranteed. If used wrong, this could lead to a crash due to eating up all memory.
There was a problem hiding this comment.
wait, how does it lead to a leak? Because it needs to be copied to be used? It's still being tracked, isn't it? It's just up to the platform to manage the lifecycle after this?
There was a problem hiding this comment.
Ok so it technically doesn't have to lead to a leak, but it would by default.
In general, read-only can appear in 3 ways:
- The value is stored in read-only memory like a constant in the app
- The value is set read-only by the host.
- Our refcounting maxes out. When that happens, we saturate to read-only and the value is never freed.
In the host case. I would expect most users to set a value to read-only and then keep it around for the lifetime of the app, repeatedly passing the value in. If instead, you set a value to read-only and then drop it, you will get a memory leak. A read only value is never freed (we don't know how it became read-only and are not sure if it is safe to free. might lead to use after free).
Technically a smart platform that knows lifetimes could use it as needed and then set it back to regularly refcounted and free the value. That said, if any roc code still has a reference to it, that would lead to use after free. As such, if you set a value to read-only, you really are labeling it as having a static lifetime that lasts until the end of a program.
There was a problem hiding this comment.
ahhhh, ok. That makes a lot of sense. Maybe this explanation belongs in the doc comment? I think clippy will warn if there isn't a section starting with "unsafety:" anyway, right? (We just don't enforce clippy-cleanliness on roc_std)
There was a problem hiding this comment.
Added a safety comment. Also filed #4366 about adding clippy and tests to CI for these crates.
| T: Clone, | ||
| { | ||
| fn from(l: RocList<T>) -> Self { | ||
| if l.is_unique() || l.is_readonly() { |
There was a problem hiding this comment.
does this also need to check if each item is unique/readonly?
There was a problem hiding this comment.
No, but maybe a better api could be made. Currently, a RocList will only be Send if every element it contains is Send. That comes from the where T: Send a few lines up.
As such, If you make a SendSafeRocList<RocStr>, it will not be Send. You would need to first run a map over the list to create a SendSafeRocList<SendSafeRocStr> and then it would be sendable. So this only needs to make the outer list Send. Rust will enforce that the inner item is also Send before allow it to be sent.
There was a problem hiding this comment.
that seems reasonable for now. It seems like an OK to manage this automatically would be to define some ConvertToSendable trait. I don't know if that's what we want, though!
|
General question: Do you think I should add That way, you can This would be used when you want to send the same data to many threads. So you would clone it for each thread and pass the clone into the thread. |
|
shallow copy meaning copying the pointer/len/capacity? I think that'd probably make sense, but I don't have a use case right now where I'd need it. Maybe you do with your web server? |
This was a request that came up when using Roc types with threads and async. They get quite inconvenient to pass from one thread to the next. These Wrappers should make that simpler.