Improve checking for nil Buffers #2960
Conversation
There was a problem hiding this comment.
OK, I've done a speed comparison:
MyTest {
setnMsg { arg ... args;
^["/b_setn", 7] ++ args
}
setn { arg ... args;
this.whatever(this.setnMsg(*args));
}
setn2 { arg ... args;
this.whatever(["/b_setn", 7] ++ args);
}
whatever { arg args;
^args
}
}
bench { 1000.do { a.setn([1, 2, 3]) } };
and
bench { 1000.do { a.setn2([1, 2, 3]) } };
here, the second (direct) call is about 1.5 times faster. Because this is a core class, this is important.
| } | ||
|
|
||
| set { arg index, float ... morePairs; | ||
| if(bufnum.isNil) { Error("Cannot call % on a % that has been freed".format(thisMethod.name, this.class.name)).throw }; | ||
| server.listSendMsg([\b_set, bufnum, index, float] ++ morePairs) | ||
| server.listSendMsg(this.setMsg(index, float, *morePairs)) |
There was a problem hiding this comment.
this will slow down performance of this method, especially if you send many morePairs. Better some duplicate code in this case (I know, it's not beautiful, but it is a bottleneck).
| ^[\b_set, bufnum, index, float] ++ morePairs | ||
| } | ||
|
|
||
| setn { arg ... args; | ||
| if(bufnum.isNil) { Error("Cannot call % on a % that has been freed".format(thisMethod.name, this.class.name)).throw }; | ||
| server.sendMsg(*this.setnMsg(*args)); | ||
| server.listSendMsg(this.setnMsg(*args)); |
| } | ||
|
|
||
| get { arg index, action; | ||
| if(bufnum.isNil) { Error("Cannot call % on a % that has been freed".format(thisMethod.name, this.class.name)).throw }; | ||
| server.listSendMsg(this.getMsg(index, action)); |
| ^[\b_get, bufnum, index] | ||
| } | ||
|
|
||
| getn { arg index, count, action; | ||
| if(bufnum.isNil) { Error("Cannot call % on a % that has been freed".format(thisMethod.name, this.class.name)).throw }; | ||
| server.listSendMsg(this.getnMsg(index, count, action)); |
| ^[\b_getn, bufnum, index, count] | ||
| } | ||
|
|
||
| fill { arg startAt, numFrames, value ... more; | ||
| if(bufnum.isNil) { Error("Cannot call % on a % that has been freed".format(thisMethod.name, this.class.name)).throw }; | ||
| server.listSendMsg(["/b_fill", bufnum, startAt, numFrames.asInt, value] ++ more) | ||
| server.listSendMsg(this.fillMsg(startAt, numFrames, value, *more)) |
| + (asWavetable.binaryValue * 2) | ||
| + (clearFirst.binaryValue * 4)] | ||
| ++ genArgs) | ||
| server.listSendMsg(this.genMsg(genCommand, genArgs, normalize, asWavetable, clearFirst)) |
| + (asWavetable.binaryValue * 2) | ||
| + (clearFirst.binaryValue * 4)] | ||
| ++ amps) | ||
| server.listSendMsg(this.sine1Msg(amps, normalize, asWavetable, clearFirst)) |
| + (asWavetable.binaryValue * 2) | ||
| + (clearFirst.binaryValue * 4)] | ||
| ++ [freqs, amps].lace(freqs.size * 2)) | ||
| server.listSendMsg(this.sine2Msg(freqs, amps, normalize, asWavetable, clearFirst)) |
| + (asWavetable.binaryValue * 2) | ||
| + (clearFirst.binaryValue * 4)] | ||
| ++ [freqs, amps, phases].lace(freqs.size * 3)) | ||
| server.listSendMsg(this.sine3Msg(freqs, amps, phases, normalize, asWavetable, clearFirst)) |
| + (clearFirst.binaryValue * 4)] | ||
| ++ amplitudes) | ||
| cheby { arg amps, normalize=true, asWavetable=true, clearFirst=true; | ||
| server.listSendMsg(this.chebyMsg(amps, normalize, asWavetable, clearFirst)) |
|
I just noticed that so far, the existing code is also not consistently caring for efficiency. So if you feel like improving it, that is good, but if you prefer to just leave this PR as it is, I can do this some other time. |
|
I kind of hate to say this after all the work rejiggering the methods, but... I'm nervous about coupling the OSCFuncs with the message creation. When I started writing this, I was only a little nervous, but after finishing my remarks, actually I'm considerably nervous about it.
TBH I find the Con position more convincing. I can understand the reason for moving the OSCFuncs, but I disagree with it. |
|
Thanks @telephon and @jamshark70 OSCFunc placementI was only 50/50 on this one anyway. James seems to have a good point. I can move them back. I wonder: is there a Performance and code duplicationI made these changes trying to avoid code duplication (both the bufnum.isNil check and Msg creation). It is not acceptable to decrease performance, however. I wonder if I need to go about this in the opposite way. All methods check for |
|
I didn't notice the OSCFuncs in the message creation code. I think it's not good to put it there, because it is supposed to just create the messages, nothing else. maybe the best solution is: make a test method like |
|
This moves the Next up is the server messages. |
|
These are the changed I now propose :
Thank you for reviewing and testing 👍 |
There was a problem hiding this comment.
OK, so now all that seems missing is the test for the "direct" methods. e.g. fill is missing it.
see e.g.
fill { arg startAt, numFrames, value ... more;
server.listSendMsg([\b_fill, bufnum, startAt, numFrames.asInt, value] ++ more)
}
fillMsg { arg startAt, numFrames, value ... more;
if(bufnum.isNil) { Error("Cannot construct a % for a % that has been freed".format(thisMethod.name, this.class.name)).throw };
^[\b_fill, bufnum, startAt, numFrames.asInt, value] ++ more
}
|
Make sure you are looking at the latest changes. |
|
ah good - sorry for the noise. Somehow the webpage showed me the wrong version. |
|
@patrickdupuis is this ready to go? |
|
Maybe @jamshark70 or @snappizz can give a look? The PR might need labelling and whatnot for the 3.9 change log. |
patrickdupuis
changed the title
|
looks good to me |
|
Ok for me too. |
This PR is a redo of my previous work on checking for freed Buffers. The checks now happen inside the
Msgmethods.OSCFuncs have also been moved inside theseMsgmethods. TheMsgmethods are now consistently called in order to construct the Server messages.Thank you for testing and reviewing!