-
-
Notifications
You must be signed in to change notification settings - Fork 100
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
WIP - Use ArcArray to remove copy #89
Conversation
@@ -5,13 +5,13 @@ use std::{ | |||
|
|||
use crate::Value; | |||
|
|||
pub struct SessionOutputs<'s> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the lifetime here means SessionOutputs
is not bound to the lifetime of the session, meaning it could be accessed after the session is dropped, causing a segfault. Maybe we could add something like session_bind: PhantomData<&'session ()>
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Value
all have an Arc<Session>
then the session will be kept alive until all the Values
are dropped?
Updated the WIP - still don't totally like the ergonomics. And like you said, the I'll see if I can find some time to tackle them soon. |
So, I'm fixing things as I'm going through. But it's getting to the point where I think it's better to just split this in to a larger refactoring where we split out the crates at least in to an Lots of unnecessary allocations, memory leaks and incorrect FFI. It would be easier to maintain to shift that to a smaller crate that is easier to do properly. e.g. Line 67 in ba9df19
This allocates a vec and extracts the pointer. But GetBoundOutputValues overwrites that pointer anyway, then calling to_vec on the slice makes a copy of everything, but then the original FFI pointer is never freed. So 2 extra heap allocations and a memory leak.
Let me know your thoughts? |
This isn't that well thought through - but it worked in my testing. Still missing the
.as_standard_layout
call - I assume calling.to_shared
on theCowArray
resolves that, but I haven't checked.