Async lang behaviour - how to this could be made easier for new users

Development
1

This is a repost of this :

Hello, I’m looking for help solving an issue as it involves the lang code and I am struggling to understand it.

Motivation

For some class

Test {
	doesNotUnderstand{|...a|
		a.postln
	}
}

Test().foo(1, 2, bar: 4)

prints

WARNING: keyword arg 'bar' not found in call to ErTest:doesNotUnderstand
[ foo, 1, 2 ]

It would allow the creation of transparent wrapper classes if this would instead return

[foo, 1, 2, bar, 4]

meaning you could call code before and after all methods.

I’ve written a little Buffer wrapper that tests to see if it has loaded on the server (and optional waits) before every method call, meaning the user never has to call s.sync again, it is completely transparent and works every where I have tested… except with named args method calls. This would be a huge simplification for the user.

Plan for Implementation

I think the problem lies here:

and that the issue is simply a matter of copying all the arguments over.
This section…

…copies only the args that match the current argument names.
Is it possible to copy the rest that do not match over, but placed at the end of the list so it doesn’t interfere with existing code?
I cannot seem to quite figure it out, is there any one who still understands this part of supercollider around?

There is one place where this will not work though, which is when the method arg name matches in doesNotUnderstand, e.g.,

Test {
	doesNotUnderstand{|...a|
		a.postln
	}
}

Test().foo(1, 2, a: 4)

prints

4

The other option would be to create a doesNotUnderstandWithKeys in the class library. There is a cpp function doesNotUnderstandWithKeys but no corresponding method in Object - this is probably a bigger change though.

Thanks,
Jordan

1 Like
3

I’ve been nerd-sniped over DM!

If you knew ahead of time the particular keywords you wanted to capture you’re golden:

Test {
	doesNotUnderstand {|bar...a|
		bar.postln;
		a.postln;
	}
}

Test().dnu(1, 2, bar:4); 

4
[ 1, 2 ]

With the regrettable side effect that the first n elements of your array will be bound to those keyword arguments, with no way to disambiguate this:

Test().dnu(1, 2);

dnu
[ 1, 2 ]

Which is the method name symbol, as the first arg in the array, being treated as the bar positional argument. Yikes! I personally wouldn’t want to write a bunch of type-dependent logic trying to determine if that was bar: \dnu in the call, or the method name.

If you want to get all the keyword args, there’s an ambiguity problem here. If we append keyword args as symbol value pairs into the VarArgs array a, these two calls become ambiguous:

Test().foo(1, 2, a: 4) // a: [\foo, 1, 2, \a, 4]

and

Test().foo(1, 2, \a, 4) // a: [\foo, 1, 2, \a, 4]

You could probably resolve this by always providing pairs in a, with the positional arguments providing integer positions followed by the key/value pairs:

Test().foo(-9, 3, a: 4); // a: [\foo, 0, -9, 1, 3, \a, 4]
Test().foo(-9, 3, \a, 4); // a: [\foo, 0, -9, 1, 3, 2, \a, 3, 4]

To limit the collateral damage of this I’d suggest rewriting doesNotUnderstandWithKeys https://github.com/supercollider/supercollider/blob/ef627ce2c564fe323125234e4374c9c4b0fc7f1d/lang/LangSource/PyrMessage.cpp#L698

which is already rewriting the arguments for this method call, to prepend the function symbol to the start of the stack. You’ll want to also rewrite doesNotUnderstand to add the positional argument indices too.

This is a breaking change, with all the baggage that entails. And I’m not actually sure I agree with it, doesNotUnderstand is already too magical in my opinion. Too many options for surprises for users ignorant of all the extra magic in that method name. But, since we already have special-case interpreter code for this method, I can see an argument for that method being able to fully reconstruct the calling arguments, including keywords. Like we’ve already special-cased it, why not get all the value we can out of that special case?

Speaking of special case magic in the interpreter, have you all explored using IdentityDictionaries with know? IdentityDictionary | SuperCollider 3.12.2 Help . Unknown method calls there get a whole dictionary with the args.

4

It occurs to me if you wanted to get even fancier you could propose modifying the VarArgs grammar to accept an optional second argument, something like:

Test {
    doesNotUnderstand {|...args,kwargs|
    }
}

And then the interpreter could copy the keyword args into the symbol/value pairs into that second argument, if provided. This is a less impactful change to the language as I don’t see how it would break existing doesNotUnderstand code (although there might be some parsing ambiguities with generators, haven’t researched that).

But it’s a much more substantial change to the interpreter. requiring modifications “front-to-back,” meaning from the parser all the way down to the run loop.

5

Thanks for the response!

That is exactly what I’m trying to do.

I’m trying to write a wrapper around another class that is transparent for the user, I cant see how this would work.

Ah, yup didn’t think of that…

What about a sc function like the following?

Object {
    doesNotUnderstandWithKeys {|selector, argArray, kwargs|
       this.doesNotUnderstand(selector, *argArray) // default implementation
    }
}

Could the cpp side of things be edited to call this rather that doesNotUnderstand if there are keyword args?
Wrapping the non-named args in an Array, and the named in an IdentityDictionary (or array in pairs)?
Would that require such substantially editing, or could it be done all in executeMethodWithKeys (cpp)?
If so, I don’t think it would break anything, but I might be wrong…

6

Well you certainly don’t need my permission to do anything to sclang :).

But if you want my opinion, AFAICT this change wouldn’t be particularly harmful, although I would be curious about possible library side effects - SuperCollider has a large set of “magic” words that aren’t keywords reserved by the language, but do special/important things, but also aren’t marked in any particular way as special (e.g. while), and this change would be adding doesNotUnderstandWithKeys to that set. Not a particularly likely word to cause surprises but worth considering. I think at minimum you’d want to have a chat with the Class Library folks, to see if they were comfortable to such a change to Object. If I was them I’d be looking to pretty strictly manage Object bloat, a tendency in languages that don’t allow multiple inheritance.

Also, I really dislike the duplication of logic in the interpreter between the WithKeys variants and not, and would not like to see that duplication replicated in the class library. I see it as interpreter technical debt, with two sets of functions doing very similar things.

7

Just came from the dev meeting yesterday actually.

Agreed, but I don’t have the skills (nor the time) to fix it, and it doesn’t appear than anyone does.

I think I have a somewhat clear understanding of what I need to do.

  1. Add method in sc
  2. Store method in the global list and change method executing in c++ doesNotUnderstandWithKeys and replace called method.
  3. Create arrays of args and kwargs using newPyrArray - still unsure who the parent is for the purpose of garbage collection…
  4. Use SetObject(argArray->slots + n, g->sp + n) to fill the arrays, creating a new array from the slots on the stack.
  5. Here I am unsure how objects are removed and created on the stack, as I have to delete/extend the everything above the selector. Also, if I using SetObject to fill the array, then edit the stack, will the object still be garbage collected afterwards? Any hints?
  6. Replace call to executeMethodWithKeys with executeMethod in doesNotUnderstandWithKeys. Since the keyed args where ignored this shouldn’t break anything.

I’d love your opinion on this, as you might be one of the few people who understands this code, or at least has some experience with it!

One thing I am completely unsure on is what blockValueWithKeys does, or any of its counter parts; it is called in a weird-finite-recursive-nested-if thing that just checks the slot isn’t nil and increments a slot pointer by the result of arrayAtIdentityHashInPairs. Not a clue…

8

My opinion is that this isn’t a good change and I wouldn’t do it. IIUC you’re proposing a change to the SuperCollider language in order to support a class that calls s.sync automatically. I don’t see those as remotely comparable in terms of impact vs. benefit. Why not simply subclass Buffer as AutoSyncBuffer, or a more clever name, and call it done? You don’t need anyone’s permission to distribute it in a Quark, and don’t even need to recompile SCLang.

Adding a method to Object means that you are essentially modifying every single SuperCollider class that has ever been written or ever will be written. IMHO that’s pretty huge! And you can’t do it as a class extension because you are also proposing modifying interpreter code, which then assumes the existence of a doesNotUnderstandWithKeys method. So that will have to ship with the Class Library.

This might read a bit harsh but that’s my honest opinion. Language changes are serious business and impact the entire community. I think we should set a very high bar for them. But I’m not the boss around here, this is my (solicited) opinion.

At minimum, I’d recommend getting a green light from someone who can review your code before getting started on this. And you’re right to worry about the garbage collector, although I think you should be pretty safe following the usage pattern in doesNotUnderstand.

\L

9

The problem is that every method in Buffer and its super classes (including almost all of the several hundred that exist in Object) would need to be rewritten in that class to check the buffer had loaded and wait if necessary.
If this was to be applied to other classes I wanted to cut back on typing and bloating the interface with all the stuff from Object.
So I wanted a generic approach and made a object wrapper that looks something like…

ObjectPreCaller {
   var held_object; 
   var func;
   
   foo { |someArg| func.(); held_object.foo(*someArg) }
   ... for every single method in Object

   // for methods defined in the held_object's class
   doesNotUnderstand { |s, *a| func.(); held_object.perform(s, *a) } 
}

This worked really well expect that kwargs get lost. Which is why I thought this feature as wrongfully omitted from the language as its just loosing information for no reason.

Well the hope was this might get added to the class library. If applied to all classes that represent a server resource, it means the user doesn’t have to think about server/client synchronisation again. I also can’t think of a reason why you would need to access the uninitialised buffer, but by wrapping it, it is quite easy to get it.

I think I’ll take your advice and stay clear of this approach.
Instead I might just add this logic into the Buffer class as it stands, even if this means editing every single method, and adding in a lot of the methods from Object too, making it even more cluttered and harder to maintain. I was hoping the generic solution might simplify that.

Thank you for your time and wisdom :smiley:
J

10

You’re welcome!

As an aside, you’re aware that s.sync round-trips to the server, right? And that it locks the interpreter while it waits? I would recommend being extremely … judicious … about adding it to arbitrary function calls. Particularly inside of methods that might be timing-sensitive, like running inside a Pattern or on a TempoClock or some such.

Happy hacking!

11

It doesn’t actually call s.sync, it uses the action callback to set a bool, which CondVar optionally waits on — it only blocks the thread if it hasn’t been set. For some reason this action is only implemented in Buffer.read, but I can’t see a reason why it couldn’t be added to all buffer creation methods.
J

12

To clarify one point – yes, it does a round trip, but I’m quite sure it doesn’t lock the entire interpreter. Under the hood, it’s a normal Condition usage, where receipt of the return message signals the condition. The thread that called sync will block but other threads are free to operate in the interim.

hjh

13

Sure. Blocking the calling “thread” might be a more accurate description. I would argue that the error bars on SCLang’s use of the term “thread” probably are big enough to have at least some overlap with the generic term “the interpreter”.

Quibbles about semantics aside, my point that this is a bad idea still stands.

14

Right, that’s fair enough. I’ve just seen enough times where some folklore gets started – “I thought I read somewhere that…” – that it’s better to clarify early.

I do agree with this. I would argue for an auto-sync wrapper around Buffer etc on the grounds that Buffer is a low-level representation of a server buffer. There are a couple of SC community bad habits here. We commonly use low-level abstractions as if they were high-level interfaces. This leads to the idea that higher-level behavior should be added into the lower-level abstraction, where object-composition design patterns are probably better (such as the AutoSyncBuffer that you suggested). Alternately – if Buffer were to be the high-level interface, it would still be helpful to have another class to handle the messaging.

hjh

1 Like
15

Just to summarise:

This started off trying to make server/client synchronisation automatic as new users will never need to access an uninitialised buffer, synthdef, synth, … or any other server resource, so why should they bare the burden.

The first thing I considered was SmartBuffer, SmartSynthDef, SmartSynth … while this might work for c++, it requires an extra step for new users who want a buffer, know there is a Buffer, but have to write something else. Which might mean they end up using Buffer and run into sync issues anyway. Also all the documentation refers to Buffer — while making dev decisions based on documentation is bad, there would be a lot of documentation to change…

Next I thought about renaiming/delegating Buffer to RawBuffer to creating a new Buffer that is completely independent of it, offering a reduce set of features and a clearer interface (this RawBuffer could even be moved into an [offical] Quark). The issue here is it requires users edit all existing files. This could be done in stages across versions like this:

Version +1. Rename Buffer to RawBuffer and create an alias Buffer : RawBuffer {...} and throw a warning every time the user call a creation method from Buffer instead of RawBuffer, and when a method is called that will not be in the new Buffer interface.

Version +2. change the new Buffer to be the implementation of SmartBuffer and throw specific errors when accessing methods that now only exist in RawBuffer.

Version +X. Remove the errors that mention RawBuffer from the new buffer class.

This would be a painful experience (particularly if someone missed a version!), but I think the reward is worth it.

The issue here is that I do not think it will not work for SynthDef as the actual def creation is split across many classes (all the FFT classes peer inside SynthDef and change the graph I think…) — actually, @jamshark70 you might be more familiar with this than I am, what do you think? But as far as I can tell, this isn’t an easy task…

So my final option was to wrap these classes in a ServerResource class that internally re-implements all of Objects methods but calls maybewait beforehand, and uses doesNotUnderstand when calling any method specific to Buffer. As already said, this is a completely transparent solution, and a new method called .getUnsafe could be added for users who really need it… but, as per this thread’s existence, the only place I found it doesn’t work is for keyword arguments.

Based on what @lnihlen has said regarding editing the interpreter and the dangers there (conversely, I don’t think editing the Object class for such a change is out of the question when it seems like an oversight), the wrapping approach is a bad idea, despite offering the least painful transition (as there would not be one).

So unless someone can offer a way to get SynthDef to work, I’m going to limit this to just Buffer (which I think is a huge shame as trying to create a synth before the def has loaded is a common error) and suggest the many versioned approach.

…unless anyone has a better suggestion?

16

I like the sound of this. But I am curious, is the user responsible for making sure everything happens inside a thread? What happens in this scenario:

// server is already booted
(
b = Buffer.read(s, "...");
b.numFrames.postln;
)

or this?

(
b = Buffer.read(s, "...");
{ PlayBuf.ar(b.numChannels, b }.play;
)
17

In the implementation I have right now, both of these produce the same error (well actually the first does, the second might not).

Error: Tried to access Buffer before the server had finished loading it.
Either wrap this call in a threaded function, like s.waitForBoot {...}, 
or literally wait a few seconds before using the Buffer.
See SOMEDOCUMENTPAGE in the documentation for more help.

So this is probably how the code should be written as it auto waits,

s.waitForBoot {
   b = Buffer.read(s, "...");
   b.numFrames.postln;
}

but this also works without error…

b = Buffer.read(s, "...");  // eval this LINE

// wait half a second or so

b.numFrames.postln; // eval this LINE

… although the precise time you need to wait will vary depending on system and what else the server has to do.

One downside to this entire approach is that sometimes it might not have loaded the buffer, and some times it might have, meaning whether an error is thrown is not predicable. Therefore code like this…

(
var b = Buffer.read(s, "..."); 
.. do some long calcuation...
b.numFrames
)

… might throw or not depending on how long was literally spent doing the long calculation which can change each time its called or across different computers. But since this was already a bug, this way actually tells you where it is, what it is, and how to fix it, whereas current solution doesn’t even recognise this as a bug making it incredibly hard to find and fix.

As an aside — when I was learning supercollider and had a bug like this (and even still today in times of great desperation), I wrote s.sync before every line of code, then gradually removed them until the problem resurfaces. This was/is a horrid experience that I’m trying to remove.

18

One idea that I’ve been trying to get across is that it is useful to have a Buffer class that just handles messaging and state variables. Sync behavior should IMO be composed on top of this, in another class. The wish to collapse both of these requirements into one and only one class is precisely what I was questioning when I raised some doubts about over-extending the base server abstraction classes.

Because we have been using Buffer as the interface for buffers, it’s tempting to think that the Buffer class itself should be the one-stop shop for buffer-op sync as well. I don’t necessarily object to Buffer being the main interface but IMO then we should push messaging and state variables down into another class, say, BufferBase, and compose.

I think there is no strong reason to have a Buffer god object except for the historical fact that we’ve been using Buffer all along – which may be a valid backward-compatibility concern, but that doesn’t mean it’s the best design.

Just an opinion; you’re completely free to ignore it.

hjh

19

I agree, it is useful to have a class that does both those things…
Buffer does a fine job of handling messages, but a terrible one of dealing with state variables because some times the Buffer object is not yet fully constructed leading to very hard to find bugs, particularly for new users unfamiliar with the idea thread synchronisation, which itself is a very complex idea.

This, I think, is where we are disagreeing… I think that the server/client relationship is a fundamental part of supercollider one that cannot be ignored as we need objects that exist both on the client and on the server — this is unavoidable as per the design and the state of the buffer depends upon synchronising first.

Exactly why I suggested renaming it to RawBuffer, and creating a whole new slimmer and safe alternative.
There is no reason for this class to every let you use it in an uninitialised state.

Again, I agree with this, the wrapping of the class was only there to make it backwards compatible. Although, they are probably already overextended…

I am not attached to the solution, just the opinion that this is a pretty serious and unnecessary hurdle for new users and the default implementation should deal with this. Whether default means lowest level is another matter.

1 Like
20

I don’t disagree with this. Actually part of what I’m suggesting is a culture shift (likely for SC4) away from relying on the base classes as the primary interface. Your view seems to be, locate the primary interfaces in the currently-used classes, shift the current functionality downward, and don’t shift the culture. My view is a bit more toward, keep the current functionality where it is, put the juicy new stuff into new classes, and do shift the culture toward the higher level objects. The two are not that much different in the end, and I’m not strongly wedded to either one, though I think this latter approach would preserve a bit more backward compatibility (if someone is already using Buffer successfully, why break that? let them keep using it, and make new functionality available in a new place).

hjh

21

This means forever increasing the size of the class library which is already pretty unmaintained and probably unmaintainable.

New users will have to keep on top of which class is recommended this year. Also, before starting to learn supercollider, they must also learn the history of supercollider as all the examples could use different classes.

This is a lot of extra work for just wanting to play a sound file.

There is a problem of name collision too, we have buffer, and perhaps smartbuffer what should the next iteration be called, verysmartbuffer? … a facetious example, but name collision is a big deal. Also, prefixing all the technical details in the prefix makes the class look more complex than it is.

I get this, but it’s just a find and replace (in some cases, bit different if something returns a buffer). That’s a pretty reasonable thing to expect someone to do - we could even add something in the ide which does this.

I do appreciate not wanting to break things… but I just don’t think it’s sustainable and makes things harder for new users (supercollider is already pretty difficult to learn).