loopification #868

shirouto · 2018-08-28T17:25:30Z

Description

This addresses #866 by adding the "-floopification" flag to eta by default. Users can still turn off the optimization through CLI arguments, config, sandbox settings, or cabal file (there is another patch on etlas related to this that insures etlas calls to eta do not turn off the flag by default).

How Has This Been Tested?

Ran the available test harness and additional tests (yet to be submitted --- working on improving the test harness as in #865 and related to #847) to verify that the loopification optimization is applied by default in both eta and etlas.

Types of changes

[ x ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation change
Test suite change

Checklist:

[ x ] I have read the CONTRIBUTING document.

rahulmutt · 2018-08-28T17:31:31Z

@shirouto Thanks for the PR!

I imagined this is the appropriate place to enable loopification in -O0 (and up):

https://github.com/typelead/eta/blob/master/compiler/Eta/Main/DynFlags.hs#L3489-L3526

What do you think?

shirouto · 2018-08-28T17:54:06Z

I thought about it, but since eta does not know where the -O0 flag came from, it would end up always adding -floopification after -O0. This could be confusing for an user that would want to specifically remove all optimizations, including -floopification, by passing in by hand -O0. Of course, we could make it such the only way to turn off loopification is by specifically passing in -fno-loopification, and handle the confusion (we would effectively remove -fno-loopification from the -O0 pack) with documentation. If you prefer the latter, I can commit another PR instead: I aggravated over having to make the change in two separate places.

It was observed that Core lint errors occur if the primops were not declared side-effecting because of thunk splitting in the worker-wrapper code.

rahulmutt · 2018-08-28T18:06:56Z

I agree with the latter - the user should specify -fno-loopification if they want to turn it off. Loopification is fairly cheap (it involves an extra integer comparison on every function call in tail position in the STG code) so I don't see a reason why anyone would want to turn it off unless they want to get SOEs.

shirouto · 2018-08-29T23:10:51Z

Well, I did the change to enable loopification by adding it to -O0, and it seems to work for simple tail recursive functions. However, before I push the new version up I wanted to check in with you about a few things.

While loopification works as expected, it turns out it is not enough to avoid SOE in etlas repl or straight eta without any optimizations. For example, code like forM_ [0..4096] print will still blow the stack. I investigated this further, and it turns out that both -fno-ignore-interface-pragmas and -fenable-rewrite-rules are needed to cover most (all?) SOE cases (I include below the related stack traces). What do you think? I guess at least the latter may be a bit tricky in the repl.
The way the repl works, even if loopification is enabled as part of -O0, :set will not report it in the repl. This may be a bit confusing for the user. Should we do something about it?

Exception in thread "main" java.lang.StackOverflowError
        at java.nio.Buffer.<init>(Buffer.java:197)
        at java.nio.ByteBuffer.<init>(ByteBuffer.java:281)
        at java.nio.ByteBuffer.<init>(ByteBuffer.java:289)
        at java.nio.MappedByteBuffer.<init>(MappedByteBuffer.java:89)
        at java.nio.DirectByteBuffer.<init>(DirectByteBuffer.java:195)
        at java.nio.DirectByteBuffer.duplicate(DirectByteBuffer.java:221)
        at eta.runtime.storage.Block.getBoundedBuffer(Block.java:96)
        at eta.runtime.io.MemoryManager.getBoundedBuffer(MemoryManager.java:135)
        at eta.base.Utils.c_write(Utils.java:146)
        at base.ghc.io.FD$a6.applyV(FD.hs:471)
        at base.ghc.io.FD.$L$wa9$1(FD.hs:527)
        at base.ghc.io.FD.$wa1(FD.hs:468)
        at base.ghc.io.FD.$fBufferedIO_FD_$s$wa(FD.hs:380)
        at base.ghc.io.FD.$fBufferedIO_FD5(FD.hs:92)
        at base.ghc.io.FD$$fBufferedIO_FD5.apply2V(FD.hs)
        at base.ghc.io.handle.Text$sat$147.apply1V(Text.hs:605)
        at base.ghc.io.handle.Internals$sat$56.apply1V(Internals.hs:256)
        at eta.runtime.apply.PAP1_1.applyV(PAP1_1.java:22)
        at eta.runtime.exception.Exception.catch_(Exception.java:132)
        at base.ghc.io.handle.Internals.$wa2(Internals.hs:167)
        at base.ghc.io.handle.Internals$a3.applyV(Internals.hs:133)
        at eta.runtime.exception.Exception.maskAsyncExceptions(Exception.java:45)
        at base.ghc.io.handle.Internals.$wa4(Internals.hs:131)
        at base.ghc.io.handle.Internals.$wa3(Internals.hs:237)
        at base.ghc.io.handle.Internals.wantWritableHandle1(Internals.hs:227)
        at base.ghc.io.handle.Text.$wa8(Text.hs:630)
        at base.ghc.io.handle.Text.hPutStr2(Text.hs:553)
        at base.system.IO.print1(IO.hs:265)
        at base.system.IO.print(IO.hs:279)
        at base.system.IO$print.apply2V(IO.hs:279)
        at eta.runtime.apply.PAPSlow.apply(PAPSlow.java:71)
        at eta.runtime.apply.PAPSlow.applyV(PAPSlow.java:119)
        at eta.runtime.thunk.Thunk.applyV(Thunk.java:121)
        at base.ghc.Base.thenIO1(Base.hs:1335)
        at base.ghc.Base$thenIO1.apply2V(Base.hs)
        at eta.runtime.apply.PAPSlow.apply(PAPSlow.java:71)
        at eta.runtime.apply.PAPSlow.applyV(PAPSlow.java:119)
        at eta.runtime.thunk.Thunk.applyV(Thunk.java:121)

and

Exception in thread "main" java.lang.StackOverflowError
        at base.ghc.io.encoding.UTF8.$wa(UTF8.hs)
        at base.ghc.io.encoding.UTF8.mkUTF1(UTF8.hs)
        at base.ghc.io.encoding.UTF8$mkUTF1.apply2V(UTF8.hs)
        at base.ghc.io.handle.Internals$a2.apply2V(Internals.hs:383)
        at base.ghc.io.handle.Internals.$wa(Internals.hs:376)
        at base.ghc.io.handle.Internals.$wa6(Internals.hs:518)
        at base.ghc.io.handle.Text$sat$147.apply1V(Text.hs:605)
        at base.ghc.io.handle.Internals$sat$56.apply1V(Internals.hs:256)
        at eta.runtime.apply.PAP1_1.applyV(PAP1_1.java:22)
        at eta.runtime.exception.Exception.catch_(Exception.java:132)
        at base.ghc.io.handle.Internals.$wa2(Internals.hs:167)
        at base.ghc.io.handle.Internals$a3.applyV(Internals.hs:133)
        at eta.runtime.exception.Exception.maskAsyncExceptions(Exception.java:45)
        at base.ghc.io.handle.Internals.$wa4(Internals.hs:131)
        at base.ghc.io.handle.Internals.$wa3(Internals.hs:237)
        at base.ghc.io.handle.Internals.wantWritableHandle1(Internals.hs:227)
        at base.ghc.io.handle.Text.$wa8(Text.hs:630)
        at base.ghc.io.handle.Text.hPutStr2(Text.hs:553)
        at base.system.IO.print1(IO.hs:265)
        at base.system.IO.print(IO.hs:279)
        at base.system.IO$print.apply2V(IO.hs:279)
        at eta.runtime.apply.PAPSlow.apply(PAPSlow.java:71)
        at eta.runtime.apply.PAPSlow.applyV(PAPSlow.java:119)
        at eta.runtime.thunk.Thunk.applyV(Thunk.java:121)

are typical stack traces I ran into when running long series of monadic binds compiled with vanilla eta. Also in the repl, something like forM_ [0..4096] print will throw off:

eta: panic! (the 'impossible' happened)                                                                                                                                                                                                        
  (Eta version 0.8.6b2):                                                                                                                                                                                                                       
        java.lang.RuntimeException: Failed during evalIO of Eta REPL expression                                                                                                                                                                
        at eta.serv.REPLClassLoader.evalIO(REPLClassLoader.java:263)                                                                                                                                                                           
        at eta.serv.Utils.evalIO(Utils.java:42)                                                                                                                                                                                                
        at main.eta.serv.Run$sat$33.applyV(Run.hs:84)                                                                                                                                                                                          
        at eta.runtime.exception.Exception.catch_(Exception.java:132)                                                                                                                                                                          
        at main.eta.serv.Run.run1(Run.hs:181)                                                                                                                                                                                                  
        at main.Lib.$wa(Lib.hs:23)
        at main.Main$sat$1.applyV(Main.hs:36)
        at eta.runtime.exception.Exception.maskUninterruptible(Exception.java:69)
        at main.Main.main6(Main.hs:37)
        at main.Main.main4(Main.hs:34)
        at main.Main.main1(Main.hs:32)
        at main.Main$main1.applyV(Main.hs)
        at eta.runtime.exception.Exception.catch_(Exception.java:132)
        at main.Main.main7(Main.hs)
        at main.Main.DZCmain(Main.hs:25)
        at main.Main$DZCmain.applyV(Main.hs:25)
        at eta.runtime.stg.Closures$EvalLazyIO.enter(Closures.java:125)
        at eta.runtime.stg.Capability.schedule(Capability.java:254)
        at eta.runtime.stg.Capability.scheduleClosure(Capability.java:210)
        at eta.runtime.Runtime.evalLazyIO(Runtime.java:372)
        at eta.runtime.Runtime.main(Runtime.java:365)
        at eta.main.main(Unknown Source)
Caused by: java.lang.RuntimeException: Failed during evalIOInternal of Eta REPL expression
        at eta.serv.REPLClassLoader.evalIOInternal(REPLClassLoader.java:381)
        at eta.serv.REPLClassLoader.evalIO(REPLClassLoader.java:261)
        ... 21 more
Caused by: java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at eta.serv.REPLClassLoader.evalIOInternal(REPLClassLoader.java:379)
        ... 22 more
Caused by: eta.runtime.exception.EtaException: thread blocked indefinitely in an MVar operation
        at eta.runtime.stg.Capability.detectMVarDeadlock(Capability.java:711)
        at eta.runtime.stg.Capability.idleLoop(Capability.java:698)
        at eta.runtime.stg.Capability.blockedLoop(Capability.java:736)
        at eta.runtime.stg.Capability.blockedLoop(Capability.java:732)
        at eta.runtime.concurrent.Concurrent.takeMVar(Concurrent.java:79)
        at base.ghc.io.handle.Internals.$wa2(Internals.hs:164)
        at base.ghc.io.handle.Internals$a3.applyV(Internals.hs:133)
        at eta.runtime.exception.Exception.maskAsyncExceptions(Exception.java:45)
        at base.ghc.io.handle.Internals.$wa4(Internals.hs:131)
        at base.ghc.io.handle.Internals.$wa3(Internals.hs:237)
        at base.ghc.io.handle.Internals.wantWritableHandle1(Internals.hs:227)
        at base.ghc.io.Handle.hFlush1(Handle.hs:305)
        at base.ghc.io.Handle.hFlush(Handle.hs:305)
        at base.ghc.io.Handle$hFlush.apply1V(Handle.hs:305)

rahulmutt · 2018-08-30T14:02:47Z

This is a known issue (traverse print [1..] overflows the stack #543). We have several options:

Add a trampolineIO inside of the definition of forM_ for the IO case. The trampoline will cause unnecessary overhead in the case where optimizations take place (i.e. with -fenable-rewrite-rules)
Rewrite it altogether and make it stack-safe in all cases:
http://neilmitchell.blogspot.com/2015/09/making-sequencemapm-for-io-take-o1-stack.html
We actually have some primops that are Eta-specific that can make the mentioned implementation robust to the optimizer.

This will require some benchmarking since they are such commonly used functions.
Can you file a separate issue to track this?

If we make -floopification enabled by default for -O0, then it makes sense why :set won't show it. It won't be confusing if we document it.

You mentioned a weird thread blocked indefinitely on MVar problem - I think that's a side-effect of the SOE. Can you file a separate issue for this as well?

Go ahead and push up your change - I'll merge it. The other issues you brought up, we'll tackle one by one.

CLAassistant · 2018-08-30T16:18:25Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ shirouto
❌ rahulmutt
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

shirouto · 2018-08-30T16:23:44Z

Very well, I reset to master and pushed again with the new approach. I will open a separate ticket for the MVar stack trace.

rahulmutt

Looks good @shirouto! Thanks.

rahulmutt · 2018-08-30T16:33:00Z

Btw, please make sure to do a rebase for your PR when updating with master next time!

rahulmutt and others added 2 commits August 27, 2018 14:59

Bump to 0.8.6b3

c1d1237

loopification

f468fbf

Apply IO Hack for Java monad as well

a8f4f84

Make the fresh*# primops side-effecting

92a8707

It was observed that Core lint errors occur if the primops were not declared side-effecting because of thunk splitting in the worker-wrapper code.

shirouto added 2 commits August 30, 2018 12:13

update to master 'loopification'

bc8c026

add loopification to -O0

9840c69

rahulmutt approved these changes Aug 30, 2018

View reviewed changes

rahulmutt merged commit 2ce44e7 into typelead:master Aug 30, 2018

shirouto deleted the loopification branch August 30, 2018 16:45

shirouto mentioned this pull request Aug 30, 2018

REPL: thread blocked indefinitely in an MVar operation #870

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loopification #868

loopification #868

shirouto commented Aug 28, 2018

rahulmutt commented Aug 28, 2018 •

edited

shirouto commented Aug 28, 2018 •

edited

rahulmutt commented Aug 28, 2018

shirouto commented Aug 29, 2018

rahulmutt commented Aug 30, 2018

CLAassistant commented Aug 30, 2018

shirouto commented Aug 30, 2018

rahulmutt left a comment

rahulmutt commented Aug 30, 2018

loopification #868

loopification #868

Conversation

shirouto commented Aug 28, 2018

Description

How Has This Been Tested?

Types of changes

Checklist:

rahulmutt commented Aug 28, 2018 • edited

shirouto commented Aug 28, 2018 • edited

rahulmutt commented Aug 28, 2018

shirouto commented Aug 29, 2018

rahulmutt commented Aug 30, 2018

CLAassistant commented Aug 30, 2018

shirouto commented Aug 30, 2018

rahulmutt left a comment

Choose a reason for hiding this comment

rahulmutt commented Aug 30, 2018

rahulmutt commented Aug 28, 2018 •

edited

shirouto commented Aug 28, 2018 •

edited