Hah! I worked this up last night, and my version was exactly the same, modulo function names. One thing that I think really needs to be solved though, is that the compiler will insert something like this in magic do blocks (if you use case/if):

main = function __do () {
  foo = bar();
  (function () {
    var $1 = ...;
    if ($1 ...) {
      return someEff;
    }
    return otherEff;
  })()();
  return unit;
}

This is a problem because with EffFn inlining, you lose a significant optimization opportunity:

(function () {
  if (...) {
    return function () {
      return someEffFn(1, 2, 3);
    }
  }
  return function () {
    return someEffFn(4, 5, 6);
  }
})()()

If we floated that call into the leaves (I think applyReturns in MagicDo.hs), then the IIFE code should take care of it.

FWIW, I'm working converting halogen-vdom to EffFn, so we can compare output and performance.

Oops.

So I verified that the simple binding in magic-do just becomes a straight call:

function __do() {
...
  $foreign.log2("Hello", "World");
...
}

But yeah this is just the case for the straightforward discard or binding cases of the magic-do optimisation.

I added this case to undo in MagicDo.purs, which eliminates a lot of redundant applications. Otherwise, if you have any cases that use EffFn, you'll probably undo the optimization due to redundant applications.

  undo (App _ (App s1 (Function s2 Nothing [] (Block ss body)) []) []) =
    App s1 (Function s2 Nothing [] (Block ss (applyReturns `fmap` body))) []

Yup, I can see that working in my example. Feel free to PR any other changes to this branch if that would be easiest?

Another change I made was to do untilFixedPoint (return . tidyUp . tco . magicDo) js', which removes a lot of leftover things like pure applications. I don't know what the implications overall for this are.

Just using untilFixedPoint (return . tidyUp) . tco =<< untilFixedPoint (return . magicDo) js' works well. Applying tco to a fixed point seems a bit dodgy.

Given the new untilFixedPoint call, we should try to measure any performance impact here before this gets merged.

Also, since this code is quite fragile, we should try to build the package set, and actually run some package test suites to make sure nothing gets broken.

Maybe the fixed point isn't necessary? It should stabilize after another run, So maybe you'd just need to split up convert into two functions, and inline the repetition.

Removed the case that was firing on local definitions rather than the particular module name, now there is a test failure for AppendInReverse.purs which I hope is unrelated

If you merge master, the failure should go away.

We can try this out extensively once the release candidate is done, just to make sure there isn't a significant performance regression.

Thanks!