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

making Synthdef work in many threads #6073

Conversation

JordanHendersonMusic
Copy link
Contributor

@JordanHendersonMusic JordanHendersonMusic commented Aug 19, 2023

Purpose and Motivation

Go off the conversation around threads on scsynth....
If many things are promises or otherwise capable of waiting, it becomes necessary for synthdefs to work in multiple threads.
Since synthdef is the most 'basic' and only way for a user or even Quark library writer to make a 'synth' (as it is tied into UGen.buildSynthDef) it ought to do the write thing.

Further, in my own work I often want to make owned resource as a part of the synthdef and synth creation process.
Since I have rewritten most of my code to make use of promises, not being able to define multiple synthdefs at a time is a big issue. This fixed that, and as far as I can tell, is the only way to fix it.

Types of changes

  • A couple of new classes to deal with thread local and thread hierarchy singletons
  • Bug fix - synthdef doesn't work in threads
  • Breaking change (small) - buildSynthDef -> UGen.buildSynthDef

There is one breaking change.
Code that once called the classvar buildSynthDef must now call UGen.buildSynthDef. Many classes already do this so there was only a few changes to make.
This will require Quarks to change their code, but it is minimal and once done, works as before.
I think this is the smallest change that is needed to achieve the result.

To-do list

I have not done the documentation and the code could be cleaner, I'm just curious to know if it might be accepted.

  • Code is tested
  • [?] All tests are passing
  • Updated documentation
  • This PR is ready for review

Examples

This code works, clearly alternating between each synthdef whilst still producing the correct controls.

(
s.waitForBoot {
	fork {
		SynthDef(\a, {
			'doingA'.postln;
			\arg1.kr(1);
			1.0.wait;
			'doingA'.postln;
			\arg2.kr(2);
			Out.ar(0, SinOsc.ar(223, mul: 0.1) )
		}).add;
	};

	fork {
		SynthDef(\b, {
			0.5.wait;
			'doingB'.postln;
			\argm1.kr(-1);
			1.0.wait;
			'doingB'.postln;
			\argm2.kr(-2);
			Out.ar(1, SinOsc.ar(432, mul: 0.1))
		}).add;
	};
}
)


SynthDescLib.at(\a)
SynthDescLib.at(\b)

To understand the changes look at these three files in order.

  1. Core/ThreadSingleton.sc - new class
  2. SynthDef - private changes
  3. UGen - one public change, rest is private

Following this BasicOpsUGen.sc, FFT.sc and FFTUnpacking have the UGen prefix added to their calls to buildSynthDef.

Finally, SynthDesc.sc is fixed.

@telephon
Copy link
Member

Interesting. In terms of implementation, don't you think this may be done with composition rather than inheritance? It seems a strong constraing that a class must inherit from the "manager" to use the manager. Imagine a pattern that needs it – you'd have to make every pattern a manager.

@jamshark70
Copy link
Contributor

I think it's worth considering very carefully how much more sophisticated SynthDef needs to be.

SynthDef is fundamentally the language-side representation of a GraphDef in the server. Making a GraphDef is an extremely complicated process, and I think because of this we sometimes want SynthDef to "do it all" -- but SC needs to have a low-level server-object abstraction for GraphDef. If SynthDef gloms on more features, then it would be necessary to move at least some of what is now SynthDef into a lower-level class.

With that in mind, what are the resources which both 1/ require blocking and 2/ properly belong in the middle of a SynthDef function?

Buffer.read should... well, "never say never," but I feel pretty confident in saying this should never be done inside a SynthDef. The user should get an error if they try to do this (perhaps even especially if they're new users: encouraging bad habits at the beginning seems to me like an anti-pattern).

The only other thing I can think of is to get something from a file or UI, or OSC resource... but a strong argument could be made that the user should collect all the data first, and only when everything is ready, then build the SynthDef.

So my general suggestion for extensions to SynthDef is not to change SynthDef, but to build on top of SynthDef. There could be a SynthDef-builder-helper that manages the traffic flow, and finishes up with the SynthDef construction.

@jamshark70
Copy link
Contributor

With all of that said, if the interpreter ever supports pre-emptive multitasking, then SynthDef will have to be made thread safe anyway... so it may be worth fixing that up in advance.

@smrg-lm
Copy link
Contributor

smrg-lm commented Sep 8, 2023

Why couldn't I hold the estate per instance? Was it because of SynthDef.wrap? My point is that if it was just for that reason these changes are too complicated while the problem is elsewhere.

@JordanHendersonMusic
Copy link
Contributor Author

JordanHendersonMusic commented Sep 8, 2023

@telephon composition > inheritance
That was the wrong choice, changed.

@jamshark70

we sometimes want SynthDef to "do it all" -- but SC needs to have a low-level server-object abstraction for GraphDef.
[...]
So my general suggestion for extensions to SynthDef is not to change SynthDef, but to build on top of SynthDef. There could be a SynthDef-builder-helper that manages the traffic flow, and finishes up with the SynthDef construction.

Completely agree, unfortunately Ugen and similar classes use many of SynthDef's methods to reorder the graph and do other stuff - and Quarks too may do this. In other words, the interface of SynthDef is integral to the interface of UGen and all unit generators. This is this the only way I could think of doing this without breaking compatibility.

Buffer.read should... well, "never say never," but I feel pretty confident in saying this should never be done inside a SynthDef.

Completely disagree. I don't want to mess around with creating and managing resources separate from where they are used, particularly if they are used once and exist from beginning, to the end of the piece. What is more, I've written a library thing that wraps buffer and synthdef so that you can create resources inside the function - two different classes for buffers that exist across synths, and that are local to the synth instance. When this is run in multiple threads, as expected, it all breaks.

@smrg-lm

My point is that if it was just for that reason these changes are too complicated while the problem is elsewhere.

I don't actually understand your point.
The change here is very simple, it is just that a bunch of other classes needed to be updated to call a method rather than access a classvar.

@@ -8,6 +7,8 @@ UGen : AbstractFunction {

var <>antecedents, <>descendants, <>widthFirstAntecedents; // topo sorting

*buildSynthDef { ^SynthDef.getActiveSingleton }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actually change to the public interface.

@smrg-lm
Copy link
Contributor

smrg-lm commented Sep 8, 2023

Just checked, I don't have the brain to follow right now, but the ugen graph is actually built from ugen classes, it's just an AST based on the AbstractFunction interface, for some reason it depends on the state of the SynthDef class (it's the graph processor/compiler), the thing that is it uses a global (library wise) state. My hypothesis was it could be simpler to pass the AST to an instance object for processing, with some simple and known compilers techniques, instead of using a global state in the library and the ping-pong methods between SynthDef and UGen. Well, that said, it's out of scope. It was on my todo list.

I don't have a position about this change, for me the problem is elsewhere, maybe there are too much diff changes in SynthDesc.

@jamshark70
Copy link
Contributor

Buffer.read should... well, "never say never," but I feel pretty confident in saying this should never be done inside a SynthDef.

Completely disagree. I don't want to mess around with creating and managing resources separate from where they are used, particularly if they are used once and exist from beginning, to the end of the piece.

Will take this question to the forum... not quite directly related to the PR itself.

@JordanHendersonMusic
Copy link
Contributor Author

JordanHendersonMusic commented Feb 2, 2024

My apologise for leaving this so long — life.

In the mean time I've thought about this some more and there are many quarks that this would break, here is a quick list of quarks that touch the member Ugen.buildSynthDef based off a quick search on https://github.com/supercollider-quarks/downloaded-quarks.

  1. Feedback
  2. Bending
  3. autogui
  4. cruciallib Instr
  5. Sequencer
  6. wslib
  7. Steno
  8. cruciallib Players
  9. UgenStructure

About half of these can be easily fixed by turning the member access into a method call (Ugen.buildSynthDef()), but the other half would require a large amount of work — most would need making thread safe, while Bending would be the biggest challenge.

While Synthdef should probably be made thread safe, without a way to tie quark versions to sclang versions I don't think it is currently worth the cost of breaking other's code.

Should I close this pr?

@JordanHendersonMusic
Copy link
Contributor Author

Yes I'm closing this because I no longer think it is the correct approach and would break quarks.

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.

None yet

4 participants