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

5) rust: use AsRef/AsMut in compute interface #1087

Closed
wants to merge 1 commit into from

Conversation

crop2000
Copy link
Contributor

@crop2000 crop2000 commented Nov 25, 2024

the current compute function signature looks like this:

	pub fn compute(&mut self, count: usize, inputs: &[&[FaustFloat]], outputs: &mut [&mut [FaustFloat]]){

this prevents us from passing a mutable reference as input buffer slice.
in the case one wants to chain dsps where the outputs of one dsp are used as the input of another it would be good if one can pass pass same slice that is used as the output of one dsp as input of another. but in that case one would pass a mutable reference. which is not possible with the current function signature.
this pr changes the functions to be more general. so that one can pass a mutable reference to the the input.
The new now looks like this:

	pub fn compute<I, O>(
		&mut self,
		count: usize,
		inputs: &[I],
		outputs: &mut [O],
	) where
		I: AsRef<[FaustFloat]>,
		O: AsMut<[FaustFloat]>,
	{

this change should be fully backwards compatible. (as first attemps have shown that needs some review and improvements of testing)

Because I found out that my earlier introduced compute_array interface is not contributing to the performance i removed it here again because it would make this change more complicated.

@crop2000 crop2000 force-pushed the rust_borrow branch 4 times, most recently from f8efbfe to f6071ce Compare December 1, 2024 11:50
@crop2000
Copy link
Contributor Author

crop2000 commented Dec 1, 2024

@sletz this one is also ready

@@ -122,10 +122,7 @@ travis:
#########################################################################
# automatic github action test
github_action:
$(MAKE) -f Make.rust outdir=rust/osec CARGOOPTIONS="" FAUSTOPTIONS="-I ../../libraries/ -double -os -ec -rnt -a archs/rust/architecture_osecrnt.rs"
Copy link
Member

@sletz sletz Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you remove the tests here ? (I think it makes sense to keep all of them)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rational was to only keep tests that relate to a change in ci. I don't have a strong opinion on this.

@sletz
Copy link
Member

sletz commented Dec 2, 2024

You wrote this change is fully backwards compatible but faust2portaudiorust and faust2jackrust are broken after this change. So what are existing projects supposed to do ?

@crop2000
Copy link
Contributor Author

crop2000 commented Dec 2, 2024

I made a mistake before because i thought i already implemented the most general interface.
Now i wrote a test to compare different interfaces and updated the pr accordingly.
here is the test for documentation.
I also updated some architecture files so that the cover differently typed buffers to be passed to the compute function.

@sletz
Copy link
Member

sletz commented Dec 2, 2024

OK, faust2portaudiorust and faust2jackrust. @bluenote10 could I ask your "more expert than me" overview on this PR ?

@crop2000 crop2000 changed the title 5) rust: use borrow in compute interface 5) rust: use AsRef/AsMut in compute interface Dec 2, 2024
@crop2000
Copy link
Contributor Author

crop2000 commented Dec 2, 2024

I think i will update the PR one more time to match the function signature as i updated it in the pr description above.
I updated the function signature above one more time to only use static dispatch.

@bluenote10
Copy link
Contributor

@bluenote10 could I ask your "more expert than me" overview on this PR ?

Sure I can look into this in a bit more detail, but will probably only get to it next weekend.

@crop2000 crop2000 force-pushed the rust_borrow branch 2 times, most recently from ef18cd4 to 2d88c57 Compare December 4, 2024 16:32
@crop2000
Copy link
Contributor Author

crop2000 commented Dec 5, 2024

i just realized the function signature can be simplified with the same meaning:

pub fn compute(
    &mut self, 
    count: usize, 
    inputs: &[impl AsRef<[FaustFloat]>], 
    outputs: &mut [impl  AsMut<[FaustFloat]>]
)

I added a commit with this change.
To me it looks clearer compared to generic type parameters.
but i don't have a strong opinion on which one we choose in the end.

@bluenote10
Copy link
Contributor

I had a closer look now into this, and as far as I can see this should be all fine!

I was re-rerunning all my benchmarks before and after that change to get a better feeling of how the generated code looks, and if it has an impact on performance. For what I've seen it all looks fine to me, and I would assume that it likely produces the same assembly anyway.

@crop2000
Copy link
Contributor Author

crop2000 commented Dec 7, 2024

@sletz @bluenote10 thanks for having a look. I am also preparing benchmarks, but havnt cleaned them up.

@sletz
Copy link
Member

sletz commented Dec 8, 2024

@sletz @bluenote10 thanks for having a look. I am also preparing benchmarks, but havnt cleaned them up.

Should I wait for that before merging ?

@crop2000
Copy link
Contributor Author

crop2000 commented Dec 8, 2024

@sletz no need to wait for publishable benchmarks.

instead of & or &mut
so that &mut can be passed as input
i changed some architecture files to cover
different ways of passing buffers to compute
@sletz
Copy link
Member

sletz commented Dec 8, 2024

Merged, thanks.

@sletz sletz closed this Dec 8, 2024
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.

3 participants