Bindings for co-log

[ambroslins]

addresses #10.

I am not sure whether the use of forkEnv in runLog and withLog is safe and sound. IMHO this does work as expected (see tests), but I don't know if this can be misused.

Feedback would be welcome :)

[ambroslins]

Adding the constraint contiguous == 0.5.2 for ghc-8.8.4 should fix the build issue. But I am not sure how to add it only to 8.8.4.

[mmhat]

Thank you :slightly_smiling_face: I'll have a look at it in the next days.

[ambroslins]

I think I messed up something while implementing the Concurrent Effects. Running the `withBackgroundLogger/logs all messages concurrently" test results in an access violation:

Access violation in generated code when reading 0xffffffffffffffff

I don't know enough about the internals of Effectful to figure out what the exact problem here is. I basically just play type Tetris :).

Maybe it would make sense to mark this PR as a draft, as it is not yet ready right to be merged.

[mmhat]

@ambroslins I made another PR against your branch with a fix of these issues. I also included a more or less elaborate explanation in the PRs description. After all, the problem was unsafeEff and passing around the environment across thread boundaries -- I apologize for the bad advise I gave you when I pointed you to that function.

[ambroslins]

@mmhat Thanks for all the help! I hope I have not cause you too much trouble.

Please let me know if further changes are required. How are maintainers and other fields handled in the .cabal files, should I change these to the repo owner/maintainers?

One more thing: You may note that I accidentally pushed on your branch (Didn't know that this is even possible). This was not my intention and I reverted the changes.

Yes, I checked the Allow edits by maintainers box, so fell free to push to this branch.

[mmhat]

Thanks for all the help! I hope I have not cause you too much trouble.

Your welcome; I am glad I could help!

Please let me know if further changes are required.

I am still thinking about the issue regarding the concurrency module and forkBackgroundLogger and mkBackgroundThread in particular. The workaround with the threadDelay is really just a hack to make the tests pass but I fear there is something more to it. I could not reproduce a similar behavior using co-log alone. Further investigations are needed and this problem is a hard one to debug. That being said, I suggest the following: In order to get this merged in time let's comment out these functions and the tests for those and open another issue once this is merged. IMHO that's the fastest way forward even though it hurts a bit. At least your work is not lost so we can build on top of it later. Would this be okay for you?

How are maintainers and other fields handled in the .cabal files, should I change these to the repo owner/maintainers?

To be honest, there is no policy regarding this (yet). Maybe @Kleidukos has an opinion about that.

[mmhat]

@arybczak Thanks for the additional review!

This seems much more complicated than polysemy-co-log. Have you taken a look at it?

Thanks for the pointer; I'll have a look at their implementation.

Using only LogAction IO msg seems reasonable to me although I'd really like to understand why forkBackgroundLogger and mkBackgroundThread do not work as expected.

[arybczak]

I'm guessing this has something to do with the fact that LogAction keeps reference to Env which is then used from multiple threads, which is a no-no.

I'm not familiar with co-log and have little free time as-is, so won't be able to confirm though.

Generally it's best to stick to dynamic dispatch or basic static dispatch if one does not have good understanding of the library internals. Hopefully recent changes to effectful emphasize that.

[ambroslins]

@arybczak Thanks for the help.

Yes, I don't understand enough about the library internals to evaluate whether this implementation is sound or not. I more or less just tried around until I had something that worked, without ever thinking about how this might work with threads etc.. But I am glad to see all this feedback as I really learned a lot because of it.

@mmhat What's your opinion on the best steps forward? Should I change this to only work with LogAction IO msg? Or implement a dynamic dispatch version, or maybe both? Without running any benchmarks I guess the dynamic dispatch version would be slower than the static one. So I am not sure whether that would be worth it, since the benefits of the dynamic version are probably hardly used.

[arybczak]

Without running any benchmarks I guess the dynamic dispatch version would be slower than the static one.

Did you run benchmarks of effectful? Run them and see what's the difference between static and dynamic state in the countdown benchmark - and that's in a very tight loop. Logging itself is much slower than that.

Also, check out the overhead of effectful over reference in the filesize benchmark - it uses a couple of dynamic effects there.

Static dispatch is to be used where it makes sense, mostly where there is only one reasonable interpretation. The performance gains, while nice, are not crucial in most cases.

[ambroslins]

I know that the difference for logging is probably very small, however if there is no real gain in implementing the dynamic version then why not choose the static version? IMO there is only one reasonable interpretation and that's running the LogAction .

Maybe I will find the time to implement both in the next few days.

[mmhat]

@ambroslins It took me a while to come back to this and I hope you still have interest in this PR. In order to revive it I'd suggest the following:

• I think we should keep both the static effect implementation and LogAction running in the Eff monad. The implementation is working as intended I see no reason why we should move to something different.
• I had another look at the "logs all messages concurrently" test and the "logs all messages concurrently" test. Those were the last tests failing but IMHO they are correct and so are the related forkBackgroundThread and mkBackgroundThread functions. Whil this is good I think we should drop the Effectful.Colog.Concurrent module since it got removed upstream in https://github.com/co-log/co-log/commit/bb329ea73241f048fa5fb82ab1fa26f43d3988a3. We may add it later to a binding of the co-log-concurrent package though.
• I will also create an own GitHub project for the two packages in this PR.

Does this sound reasonable?

[ambroslins]

@mmhat Thanks for the reminder and the suggestions.

Yes this does sound reasonable. I will implement these changes as soon as possible.

[ambroslins]

I think these commits should cover all changes. I am not sure how you would like to handle moving this to the new Project, I made a new branch with only the effectful-co-log and effectful-co-log-core packages, if this does help.

I also played around with a dynamic dispatch implementation and it looks like it would be easy to switch the dispatch type if we would run into problems. However, I think my implementation for withLog is wrong or too complicated.

[arybczak]

@ambroslins thanks for working on this PR.

We'll remove this repo in the future (since we now maintain a repo per project as that makes for much better separation of concerns).

However, the library for logging we use is the log library, so I'm only going to maintain bindings for log.

Hence the question is, are you willing to release and maintain the co-log bindings?

[ambroslins]

@arybczak thanks for the info.

I don't have to time to work on this for the next four weeks, but I will probably try to release and maintain the co-log bindings after that.

Btw. congratulation on the effectful release. I am excited to try it out in a project.

[Mr-Andersen]

Hi people. I have my own effectful bindings for co-log, temporarily hosted here: https://git.poridge.club/aka_dude/effectful-co-log. I am willing to maintain them. Where should I open PR to?

[arybczak]

You don't have to open a PR anywhere, you can maintain the library yourself, publish new releases to hackage etc.

This repo was an attempt to gather all bindings in one place, but it was deemed unnecessary centralization.

[Mr-Andersen]

@arybczak would it be possible to request a review from effectful wizards if I host repository on codeberg.org?

[arybczak]

Does any of you plan to release the bindings eventually? There was also https://discourse.haskell.org/t/feedback-for-potential-first-library-co-log-effectful/6246 so it looks like there are 3 unreleased implementations :stuck_out_tongue:

[ambroslins]

Does any of you plan to release the bindings eventually? There was also https://discourse.haskell.org/t/feedback-for-potential-first-library-co-log-effectful/6246 so it looks like there are 3 unreleased implementations 😛

Unfortunately, I don't have time for that at the moment. Maybe I could do it in about 3 months, if no one else has published it by then.

[Mr-Andersen]

@arybczak I am too busy now to do this :)