Skip to content
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

Fix deref #106

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Fix deref #106

wants to merge 5 commits into from

Conversation

YruamaLairba
Copy link
Contributor

@YruamaLairba YruamaLairba commented Apr 3, 2022

I already mentioned it elsewhere, few time ago i discovered that the port system and especially the dereferentiation mechanism was wrongly implemented, so i started to fix it. This fix would solve #95 and make ports safer. Every reference gotten from ports inside run context couldn't outlive this context. However, ports themself may moved out from run context, but i actually didn't find a way.

Current state:

  • port system and ports are fixed in lv2_core.
  • atom ports are broken, i just did a hack to be able to compile anyway. I will try to fix soon but i may need help for that, i'm unable to understand how the whole thing work internally.

EDIT: forgot to mention, a new issue appear with this fix, "inplace output" ports can produce mutable reference, which is bad. This is not addressed here because this require to modify some design choice about ports.

@PieterPenninckx
Copy link
Contributor

PieterPenninckx commented Apr 22, 2022

Cool, thanks for the effort!

Edit: I didn't see it's already marked as "Draft".

I'm starting to try to understand your PR and I will probably have to come back to this later. As a reminder for myself and to help other reviewers, I summarize the PR here:

  • The input_from_raw method from the PortType trait now returns *const Self::InputPortType rather than Self::InputPortType. As a consequence, Self::InputPortType is now ?Sized instead of sized.
  • Similar change to output_from_raw.
  • In the implementation of e.g. the PortType trait for Audio, the InputType associated parameter is defined as [f32] instead of &'static [f32].

It's not clear to me yet how this solves the issue with the 'static lifetime. I'll have to think about it more another time.

@YruamaLairba
Copy link
Contributor Author

It's not clear to me yet how this solves the issue with the 'static lifetime. I'll have to think about it more another time.

This is actually not so easy to understand, this require to well understand how works lifetimes inference/elision/binding, and how dereferentiations works and when it occur implicitly. Chapters 3.3 to 3.6 of the nomicon explain the lifetime part. Solving this issue at this step was even unintentional, I realized afterwards it was solved.

For an explanations with an example:

struct Amp {
    foo: Option<&'static [f32]>
}

struct Ports { input: InputPort<Audio> }

impl Plugin for Amp {
    fn run(&mut self, ports: &mut Ports, _features: &mut (), _: u32) {
        self.foo.replace(&*ports.input); // Should fail to compile now because lifetime issue
    }
}
  • Doing &*ports.input is equivalent to call deref(&ports.inputs) -> &[f32]. If you expand this give deref<'a>(&'a ports.inputs) -> &'a[f32]. You now see that your "&[f32]" is valid as long ports is valid.
  • ports is a &mut _ passed to the run function. This reference doesn't have lifetime annotation, so Rust only knows it's valid for run, in other word, may not valid outside the run context. Because of that rust won't let you move your &mut Ports or &[f32] outside the run context.

Before the fix &*ports.input was actually equivalent to call deref(&ports.inputs) -> &&'static[f32], the result was implicitly dereferenced to &'static[f32] when calling Option::replace()

And keep in mind lifetimes issue aren't fully solved. Ports are just difficult to move away from run context, A safe legal way may exist. And this difficulty exist because i didn't implement Sync or Send for any ports. In Theory, it's may legal to implement those trait for some ports.

@PieterPenninckx
Copy link
Contributor

Thanks for explaining. If I get it correctly, the important part is in the implementation of the Deref trait:

impl<T: PortType> Deref for OutputPort<T> {
    type Target = T::OutputPortType;

    fn deref(&self) -> &Self::Target {
      todo!();
    }
}

Because of the other changes, now T::OutputPortType is typically [f32] rather than &'static [f32] and the type of the return value of the derefmethod is &'a [f32] (where 'a is the lifetime of the OutputPort) rather than &'a &'static [f32].

I think &[f32] makes much more sense than &&'static [f32]. So you just removed the unneeded &'static. Cool!

And keep in mind lifetimes issue aren't fully solved. Ports are just difficult to move away from run context, A safe legal way may exist. And this difficulty exist because i didn't implement Sync or Send for any ports. In Theory, it's may legal to implement those trait for some ports.

The only thing I could think about is that the run method could spawn a separate thread and share the port with that thread, as long as the thread is stopped before the run method returns ("scoped threads"). I can't think of anything else.

Looks nice! I'm enthusiastic about this PR!

@YruamaLairba
Copy link
Contributor Author

The only thing I could think about is that the run method could spawn a separate thread and share the port with that thread, as long as the thread is stopped before the run method returns ("scoped threads"). I can't think of anything else.

Sharing port data across "scoped threads" is probably less efficient than copying port data across to a threads pool. Spawning/Joining threads have probably a much higher overhead than copying port data.

And because of Multi-threading overheads in general, i don't think there is so many situation where multi-threading actually improve performance. I think If someone want to optimize by increasing parallelism, he should look first how to rewrite it's algorithm to increase auto-vectorisation.

@YruamaLairba
Copy link
Contributor Author

YruamaLairba commented Apr 28, 2022

forgot to say, i'm not currently working on this PR. It's really difficult to understand how the Atom crate work internally. Things seems excessively complex. the "Deref missunderstanding" persisted here resulting to have Port that can't be easily fixed. I also found some suspicious code like a a lifetime transmutation.

Maybe i shouldn't wait atom to be fixed to propose a PR reworking port system to support Atom EDIT: to properly support "Inplace" processing ? Anyway, i'm busy on another project :)

@YruamaLairba YruamaLairba mentioned this pull request Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants