Hoisting isn't done early enough, resulting in duplicated work during compilation

[cjay]

newtype ClosedTerm' (a :: PType) = ClosedTerm' (ClosedTerm a)

pbranch :: forall (b :: PType). ClosedTerm' b -> ClosedTerm' b
pbranch (ClosedTerm' cont) = ClosedTerm' $
  phoistAcyclic $
    pif (pcon PFalse) cont cont

pexplode :: forall (s :: S). Term s PInteger
pexplode = term
  where
    escalating = iterate pbranch (ClosedTerm' 0)
    -- call hierarchy depth / compile time of pexplode:
    -- 10: 0.5s
    -- 13: 1.1s
    -- 15: 3.7s
    -- 17: 16.5s
    -- 19: 79.5s
    ClosedTerm' term = escalating !! 19

A call hierarchy depth over 15 might be rare, but maybe this points to some deeper issue. I suppose hoists don't recognize hoisted sub-terms, instead hashing a fully inlined AST, or something like that? Note that sharing the sub-terms on the Haskell-level does not help.

[L-as]

I'm not sure what you want to happen instead? You're generating an AST with roughly 2^19 nodes.

[cjay]

I expected use-sites of hoisted terms to behave like function calls do in other languages, so they would just be a named reference in the AST, with the name being the hash. Looks like my intuition was wrong there. Thinking a bit more about this, I realize the implementation of a hoisted thing would still have to be carried along with the hash, if the compile function should stay pure. So to prevent memory explosion, terms would need to deduplicate their hoisted subterms while being constructed. Maybe it would be possible, but not sure if it would speed up the compiler in normal use cases at all.

[L-as]

This isn't the issue here. There is no way to do what you're asking, because pbranch fundamentally duplicates the AST. If you make pbranch use a plet, there probably will be no issue. What you're essentially asking for, is for an implicit plet to be inserted. This is antithetical to the design of Plutarch. Rather than assuming it is Haskell, it is useful to understand the underlying model. It is beneficial to allow the programmer to have precise control over what is generated, rather than implicitly inserting plets.

[blamario]

I'd consider it a low priority issue, but we could add a pre-compile fold of the AST to check its size and report an error if it exceeds some ridiculous bound.

[L-as]

Now that I'm looking at the code again, I'm not sure why it's not hoisted.

[L-as]

Ah, the issue here is different I think. Things are being hoisted, but to do the hoisting, the huge AST has to be traversed. Maybe if we do the hoisting while generating the terms, it can me made more efficient.

[L-as]

@cjay Try compiling with -O2.

[L-as]

Maybe also compile Plutarch with -O2?

[cjay]

-O2 on the program was already in use, with the times from the comments. -O2 on Plutarch also seems to make no difference. Assuming I did this correctly, having the following in cabal.project (and reloading nix develop):

package plutarch
  ghc-options:   -O2

[kozross]

@L-as - what's the status of this issue? Is it possible to fix this?

[L-as]

@kozross Haven't worked on this, but should be possible to fix. This is affecting you?

[kozross]

@L-as This is a problem we are running into with @cjay's work, so a fix would be great!

[L-as]

@kozross have the phoistAcyclic always be at the top level

[cjay]

Having 19 top-level definitions that call each other would change this? How? Because of CAF? If I remember correctly I tried with local let-bindings, at least that didn't improve compile time.

[L-as]

GHC will cache evaluations of top-level definitions.