stack-safe closure conversion in speedy compiler

[nickchapman-da]

This change re-codes the speedy closure-conversion phase to be stack-safe, and adds tests that 'prove' this stack-safety. The old version of the code did not pass these tests.

As additional one-time testing, the entire repo was successfully built such that both the old and new closure-conversion code was run, and a diff check performed for every expression transformed. You can see this by looking at the 2nd commit in the PR where this old code and diff check are removed.

During this work, some unrelated quadratic-or-worse complexity issues were discovered. These are not fixed here, but are marked with TODOs, and will be addressed in future work. #11830

This change also includes some minor cleanups to SExpr1, and removes an unused constructor.

This work is part of #11561

benchmark

Using a new benchmark #11852, I checked the impact of this PR on speedy compilation times, runinng on an important .dar -- There is no observable impact.

bazel run daml-lf/interpreter:speedy-compilation-benchmark -- -f 2 -i 10 -wi 10 -bm avgt -p darPath=/path/to/important.dar

before:

SpeedyCompilation.bench  /path/to/important.dar  avgt   20  7.202 ± 0.368   s/op

after:

SpeedyCompilation.bench  /path/to/important.dar  avgt   20  7.073 ± 0.127   s/op

[remyhaemmerle-da]

I like it. Thanks.

[remyhaemmerle-da]

1- Why this is still called SEAppGeneral and not simply SEApp ?
2- Why does it take an Array as second Argument ? Expr0.SEAppGeneral should problably used a list as well.

[remyhaemmerle-da]

I did not look in details, but more generally it guess it would be nicer, if only SExpr uses Array an SomeArrayEquals.

[nickchapman-da]

Change to use List for these two cases in SExpr0
Also renamed SEAppGeneral -> SEApp and removed the SEApp object.

[remyhaemmerle-da]

Suggested change

	Bounce(() => transform(depth, target.SEMakeClo(Array(fvs: _*), arity, body), k))
	Bounce(() => transform(depth, target.SEMakeClo(fvs.toArray, arity, body), k))

[nickchapman-da]

yup

[remyhaemmerle-da]

Why not renaming this to source as you have done elsewhere ?

[nickchapman-da]

ok
but I opened it later with import source._ 😄

[cocreature]

Nice work! A few minor comments and lets make sure we run some benchmarks before merging this.

[cocreature]

Referring to code that we deleted isn’t all that useful. Let’s focus the comment on how the issues and how we want to change it rather than where it originates from.

[nickchapman-da]

ok

[cocreature]

Suggested change

	* recursive expression instances.
	* recursive expression instances (values of type SExpr).

recusive expression instance sounded a bit confusing to me at first.

[nickchapman-da]

done

[cocreature]

Suggested change

	* For the more complex expression forms containinga list of recursive instances
	* For the more complex expression forms containing a list of recursive instances

[nickchapman-da]

fixed

[cocreature]

Why do we embed the further continuation instead of having a stack of continuations and having the empty stack match to Finish? Inlining the stack in the continuations themselves seems a bit weird.

[nickchapman-da]

I will investigate this idea...
As we discussed offline, it probably has a very tiny perf impact (traverse 2 pointers instead of 1), but should be compensated by code clarity: removing the duplication of having every Cont variant containing a cont member.

[cocreature]

feel free to tackle this in a separte PR.

[nickchapman-da]

Thanks. But I think I will do it now. It shouldn't take long. And it would be good to do it before benchmarking.

[nickchapman-da]

I have moved to use a stack of continuations as suggested.

[nickchapman-da]

I have moved to use a stack of continuations as suggested.

[cocreature]

this also doesn’t look stack safe afaict

[remyhaemmerle-da]

This could probably be solved in the same time as the complexity TODO above.
We could simply compute and store the free variables in the source.SExpr objects directly as we are building them in the previous pass.

[nickchapman-da]

You are correct. This is not stack safe!
I added a note to this effect in issue #11830

[remyhaemmerle-da]

Suggested change

	sealed trait Cont
	private[this] abstract class Cont

1. If it does not need to by public, it should be private
2. abstract class are in general slightly more efficient.

[nickchapman-da]

It can't be marked private because we inside an enclosing def. (so we are more private that private!)
I changed from trait to abstract class
But it must still be a sealed abstract class so we get exhaustivity checking of pattern matches

[remyhaemmerle-da]

My Bad.

[remyhaemmerle-da]

Suggested change

	sealed trait Traversal
	object Traversal {
	final case class Down(exp: source.SExpr, env: Env) extends Traversal
	final case class Up(exp: target.SExpr) extends Traversal
	}
	import Traversal._
	private[this] abstract class Traversal
	private[this] object Traversal {
	final case class Down(exp: source.SExpr, env: Env) extends Traversal
	final case class Up(exp: target.SExpr) extends Traversal
	}
	import Traversal._

[nickchapman-da]

same here

[remyhaemmerle-da]

My Bad as well.

[nickchapman-da]

Thanks for your comments.
The above commits address everything except for the continuation-stack idea, which I plan to explore next.