Fix deref

[YruamaLairba]

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]

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]

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]

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]

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]

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 :)