make MSF a newtype

[sheaf]

Making MSF a newtype fixes some space leaks, in particular with ArrowChoice. This means that if statements and case statements no longer leak space, and as a result functions like throwOnCond which use the ArrowChoice functionality also no longer leak.

[turion]

I wasn't aware that if and case statements leak space. Is there a simple example you can provide for this? How does the space leak arise?

[sheaf]

I'll try to provide a self-contained benchmark later (I'm currently debugging other space leaks in my program), but so far let me say that in my code I had:

anyQuit :: Foldable f => f Action -> Bool
anyQuit = any \case { QuitAction -> True ; _ -> False }

quit :: ( Monad m, Foldable f ) => MSF (ExceptT Quit m) (f Action) ()
quit = throwOnCond anyQuit Quit >>> arr ( const () )

A heap profile revealed that this use of throwOnCond within my program was leaking space at a rate of about 20kB/s.
There were also other functions which were directly using if or case statements within arrow notation and they suffered from the same problem (leaking at the same rate).
All these leaks completely vanished after making MSF a newtype.

I think the leak happened because the method in the ArrowChoice instance causes allocations as a result of wrapping/unwrapping. I don't know how if and case are defined in terms of ArrowChoice so I can't be certain,

[ivanperez-keera]

If this breaks nothing, I am in principle not against.

My suspicion is that the issue with ArrowChoice may be due to a poor implementation (Yampa has a similar issue and, at least there, the mistake was mine).

[ivanperez-keera]

@sheaf I'd be glad to accept this if we can somehow obtain some way to check. Were you able to create a minimal example we can use to quickly verify the leak and the fix?

[ivanperez-keera]

I have just done some basic tests with haskanoid, both deepseqing the main game state and without deepseqing it, and with and without -O2.

I think there is some leak in dunai or bearriver. This seems unrelated to this issue, but it would be nice to use some non-trivial game to understand what's going on both with this PR and generally with dunai.

There is a haskanoid branch called https://github.com/ivanperez-keera/haskanoid/compare/develop-haskelltitan that may help. It enables record-and-replay, which enables create reproducible runs. It would require some work to merge this properly into haskanoid, and I'd be happy to guide that separately there. Otherwise, it can serve as an example of how to record and replay and facilitate profiling in a systematic way.

I've also added a branch: https://github.com/ivanperez-keera/haskanoid/tree/develop-deepseqbangs that allows deepseqing the game state (to make sure the problem is not a leak in haskanoid itself). I don't think we need to merge it, but it may be useful.

In a recent issue, haskanoid started supporting bearriver: ivanperez-keera/haskanoid#95, so compiling with -fbearriver should do.

[sheaf]

I unfortunately haven't managed to come up with a self-contained example that demonstrates the problem.

Here are the original heap allocation graphs that showcase the difference:

Note that in the "datatype" version there are two space leaks which are resolved in the "newtype" version, pertaining to throwOnCond and throwOn. There are other space leaks too, which I have since resolved and are unrelated to dunai.
I don't have any reproducer I can provide, and would also like to note that the current version of my program does not exhibit this space leak (i.e. reverting MSF to a datatype does not reintroduce any leaks).

[ivanperez-keera]

I don't fully understand. Do you mean to say that reintroducing the data still gives you a completely flat profile, or is the MSF/dunai/bearriver leak still there (just not the leaks in your own code)?

[sheaf]

I mean that the leaks due to throwOn/throwOnCond don't resurface in my current program when reverting to the datatype definition, which indicates that the leak only happens under certain conditions. Maybe it's that, with the current iteration of my program, GHC manages to optimise the datatype constructor away, whereas it didn't with the previous iteration of my program.

[ivanperez-keera]

If the leak is not manifesting once you change your side of the code, then I'd rather close this for now and revisit later.

Changing a newtype for a data may have consequences if we are seq-ing stuff somewhere (I don't know), because it eliminates one level of laziness and we rely on laziness for coinduction. This could change the behaviour of dunai. Because we do not have a thorough test suite to make sure everything still works, I cannot be sure that nothing breaks.

We will incorporate a proper way of benchmarking in the future, and then we'll be able to answer these questions properly. When we have proper tests and benchmarks, we can try again, and if nothing breaks then, then I'll re-open and merge this PR.

[ivanperez-keera]

That being said, many, many thanks @sheaf :)