Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added oneShot to Codensity continuation #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Icelandjack
Copy link
Contributor

Solving issue #78. Rewrote functions to add oneShot to the continuation:

instance Functor (Codensity k) where
  fmap f (Codensity m) = Codensity $ oneShot $ \k -> m (k . f)

@Icelandjack
Copy link
Contributor Author

Is it worth adding a oneShot to each continuation, like in ghc

  Codensity f <*> Codensity g =
    Codensity $ oneShot (\bfr -> f $ oneShot (\ab -> g $ oneShot (\x -> bfr (ab x))))

Or the one from IOSim which only uses it for the first continuation.

  (<*>) = \df dx -> IOSim $ oneShot $ \k ->unIOSim df (\f -> unIOSim dx (\x -> k (f x)))

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started, @Icelandjack.

I find it difficult to review this at the moment, as this PR appears to have introduced some unrelated changes, which I don't believe were intended.

src/Control/Monad/Codensity.hs Outdated Show resolved Hide resolved
src/Control/Monad/Codensity.hs Outdated Show resolved Hide resolved
@RyanGlScott
Copy link
Collaborator

Is it worth adding a oneShot to each continuation, like in ghc

  Codensity f <*> Codensity g =
    Codensity $ oneShot (\bfr -> f $ oneShot (\ab -> g $ oneShot (\x -> bfr (ab x))))

Or the one from IOSim which only uses it for the first continuation.

  (<*>) = \df dx -> IOSim $ oneShot $ \k ->unIOSim df (\f -> unIOSim dx (\x -> k (f x)))

An interesting question. Considering that GHC invented the oneShot trick (as far as I am aware), I'm inclined to use GHC's implementation. That being said, it would be worth opening an issue on the io-sim side to see if there is a deeper reason for this difference (or if this is simply an oversight).

@Icelandjack
Copy link
Contributor Author

I've fixed it @RyanGlScott, as to your questions I asked Marcin Szamotulski about it and he did the following benchmark: input-output-hk/io-sim#149. One test drops from 6.7ns to 5.955ns but he says he's going to run a larger simulation to see if it makes a more significant difference.

@RyanGlScott
Copy link
Collaborator

RyanGlScott commented Feb 26, 2024

Thanks, this is looking better. Two thoughts:

  1. We should definitely cite GHC's Note about the one-shot trick somewhere in the comments, as it's not obvious why you'd want to use oneShot here unless you're familiar with the trick.
  2. Reading that Note, one part of the trick that is not implemented here is using a pattern synonym to ensure that all applications of Codensity make use of oneShot. Shouldn't we do that here as well?

@Icelandjack
Copy link
Contributor Author

Marcin had mentioned that idea previously

I run an experiment where I added them using a pattern synonym for IOSim, and it was a regression with respect to just adding them manually. It was surprising to me, but I haven't investigated why it was so.

I can write a blurb and link as well, but what is the best format of linking to a note from ghc?

@RyanGlScott
Copy link
Collaborator

RyanGlScott commented Feb 26, 2024

Wow, the fact that it regressed performance is indeed surprising. I suppose this means that we really should be benchmarking these Codensity changes to see what kind of performance numbers we get—or, if nothing else, we should cite this io-sim discussion (is it online somewhere?) as justification.

I can write a blurb and link as well, but what is the best format of linking to a note from ghc?

I think it would be helpful to write our own Note in the module which defines Codensity, and to include a reference to the Note from the first use of oneShot. Something like:

-- See Note [oneShot] for an explanantion of why we use oneShot below

instance Functor (Codensity (k :: j -> TYPE rep)) where
  fmap f (Codensity m) = Codensity (\k -> m (\x -> k (f x)))
  {-# INLINE fmap #-}

instance Apply (Codensity (f :: k -> TYPE rep)) where
  (<.>) = (<*>)
  {-# INLINE (<.>) #-}

-- ...

{-
Note [oneShot]
~~~~~~~~~~~~~~
The code in this module uses oneShot because ...

The inspiration for this trick comes from ... (cite the GHC Note here)

We deviate slightly from the presentation shown in that Note because
of performance reasons, which are explained in ...
-}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants