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

Classlib: Correct 'a+b.neg' ugen optimization, and fix descendants #3437

Merged
merged 2 commits into from Feb 2, 2018

Conversation

Projects
None yet
7 participants
@jamshark70
Copy link
Contributor

jamshark70 commented Jan 16, 2018

This PR addresses the issues raised in #972.

  • a + b.neg should optimize to a - b only if b.neg has only one descendant. But the prior logic was to perform some sort of optimization, no matter how many descendants needed the b.neg result. This caused required UGens to be lost to dead code optimization.

  • Scott C. correctly pointed out that the "optimize" methods use descendants to decide whether the optimization is legitimate, but they do not update the descendants sets belonging to inputs of the replaced or deleted units. This PR introduces a method to update the descendants of all relevant input UGens.

  • It also adds some preliminary unit tests for SynthDef optimizations. Currently testing each binary-op optimization method for the correct types of remaining operators. The tests don't currently check inputs or descendant sets.

Test case from #972.

(
d = SynthDef(\spreaderBroke, {
	var a, b, c, d, e;
	a = DC.kr(0.3);
	b = a + 1.1;
	c = b.neg;
	Poll.kr(1, c);
	
	d = 1.2 - c;
	e = d.abs;
}).add;

d.children
)

Without the fix, Poll gets lost because its source (b.neg) got folded into optimizeSub, and then the optimized UGen got removed as dead code.

-> [ a DC, a Sum3, an Impulse ]

With the fix, d and e are correctly dropped (dead code elimination) but the use of b.neg in Poll prevents optimization (correctly), and Poll remains intact.

-> [ a DC, a BinaryOpUGen, an UnaryOpUGen, an Impulse, a Poll ]

Suggesting 3.9.1 as it is a bugfix (of a subtle issue that is hard to explain), not a new feature.

jamshark70 added some commits Jan 1, 2018

Testsuite: Preliminary tests for binary operator optimization
Limitations of the current tests:

- They check only for the correct types of math operators remaining
  after optimization. They do *not* check the inputs of the optimized
  UGens.

- They do not check ancestors and descendants.

@jamshark70 jamshark70 requested a review from scztt Jan 16, 2018

@scztt

This comment has been minimized.

Copy link
Contributor

scztt commented Jan 17, 2018

@jamshark70 I can look at this in the next few days. You are a superstar for addressing this :)

@scztt

This comment has been minimized.

Copy link
Contributor

scztt commented Jan 17, 2018

@mtmccrea Want to look at this also, since you originally found it? You should be able to drop in the .sc's to test.

@mtmccrea

This comment has been minimized.

Copy link
Contributor

mtmccrea commented Jan 18, 2018

@scztt I ran it on the original case (2013!) and it looks like we're getting the right answer now.
Thanks to you and @jamshark70 for taking this on, it's been a long time coming and I know this one was a deep dive. Much appreciated!

@snappizz snappizz added this to the 3.9.1 milestone Jan 21, 2018

@snappizz snappizz self-assigned this Jan 21, 2018

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Feb 1, 2018

This builds, the reproducer is fixed, and the UnitTest passes. It's clear that @jamshark70 thought this through carefully.

@scztt have you had a chance to look at this?

@joshpar joshpar self-assigned this Feb 1, 2018

@joshpar

joshpar approved these changes Feb 1, 2018

@jamshark70

This comment has been minimized.

Copy link
Contributor Author

jamshark70 commented Feb 2, 2018

FWIW, just now, I wanted to check myself on the descendants logic. After some false starts, I'm satisfied that it's working correctly.

Something to ponder for the future -- It's incredibly difficult to test UGen states in the middle of optimization, for a few reasons:

  • No external access. We initTopoSort at the beginning of optimization (populating descendants), and re-init for the topological sort itself. You can't put debugging statements into the SynthDef function because optimization happens after the function returns, and you can't unit test the def after building because the antecedents and descendants variables have been cleared (and even if they weren't cleared, they would reflect the state after sorting, not the state after optimization). I had to resort to extensive and hard-to-read debugging posts.

  • OutputProxies. Debugging was harder because MultiOutUGens pass OutputProxies as inputs to their descendants, and we don't set descendants the proxies. That's not wrong in itself -- we optimize and sort UGens, not output channels -- but it means, for instance, when I checked a.tryPerform(\descendants).do(_.dump), if a was an OutputProxy, nothing would print.

I don't think we have to worry about it for this PR. I checked optimizeAddNeg and optimizeToMulAdd and, after optimizeUpdateDescendants, upstream units' descendants are right. Since I'm using a method for this update, I'm sure it's consistent across the other optimizations. But it's something to be aware of for future work on graph optimizing -- it's easy to check the result, but hard/impossible to unit test the logic to get there.

@scztt
Copy link
Contributor

scztt left a comment

Finally had some time to look through this more closely. This looks good, at least judging from my memory of this problem :). I keep feeling like these optimizations could be further generalized into just a couple functions, but given that this high-severity bug stuck around for years without a solution, the risk of further change far outweighs the benefit to clarity.

@scztt

scztt approved these changes Feb 2, 2018

@brianlheim brianlheim merged commit 9a12ad2 into supercollider:3.9 Feb 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment