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

Make foldl strict, like foldl' #2691

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Make foldl strict, like foldl' #2691

merged 1 commit into from
Mar 12, 2024

Conversation

martijnbastiaan
Copy link
Member

hasura/graphql-engine#2933 (comment)

Fixes #2482


I opted to use clashSimulation instead of writing custom black boxes and evaluator rules, because I feel we already have way too many error prone reimplementations of mundane code in clash-lib.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

go z Nil = z
go z (Cons x xs) =
let z1 = f z x
in z1 `seq` go z1 xs
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
in z1 `seq` go z1 xs
in z1 `seqX` go z1 xs

?

Copy link
Member

@DigitalBrains1 DigitalBrains1 Mar 10, 2024

Choose a reason for hiding this comment

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

seqX would allow using a function that is lazy in its first argument to keep working for XException as it did before foldl became strict, but it is also pretty confusing to people comfortable with bottom values that not all bottoms are treated equal in a function explicitly marked as being strict. So I'm inclined to treat all bottoms as producing bottom by definition.

Copy link
Member

Choose a reason for hiding this comment

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

Another option, a seq where exceptions in its first argument are simply ignored, would at least treat all bottoms as equal but still violate the expectance that a strict foldl would in fact return bottom whenever a bottom is encountered while accumulating. So, once again, I'm inclined to use seq.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should offer a variant with the behaviour that bottoms don't propagate up, so people can still actually write a function lazy in its first argument producing results.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm inclined to use seqX, for the reasons you mention:

seqX would allow using a function that is lazy in its first argument to keep working for XException as it did before foldl became strict,

Because hunting XExceptions is a pretty miserable experience a lot of the time.

Copy link
Member

Choose a reason for hiding this comment

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

We would already differ in the respect that foldl is strict at all, so I suppose if we clearly document foldl as being strict with the exception of ignoring XExceptions in its accumulator, that would also work. But we do need the extra documentation because what we're doing is somewhat strange.

Copy link
Member Author

@martijnbastiaan martijnbastiaan Mar 10, 2024

Choose a reason for hiding this comment

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

That's fair, but I think that's only because we specialize its types. As mentioned in the documentation of Data.Foldable, it never makes sense to use foldl on lists (edit: it's, to me, heavily implied, also see lexi-lambda's comment). We're somewhat special because we (ab)use bottoms to represent X.


Also I thought newer bases already make foldl strict for Lists, but it doesn't seem to be so:

foldl k z0 xs =
  foldr (\(v::a) (fn::b->b) -> oneShot (\(z::b) -> fn (k z v))) (id :: b -> b) xs z0

(Also if you think that implementation is unreadable, look at the one in Foldable:

foldl f z t = appEndo (getDual (foldMap (Dual . Endo . flip f) t)) z

)

Copy link
Member

Choose a reason for hiding this comment

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

In what case would we even observe the difference? The function would be really weird... where it ignores its arguments in some cases, and return exceptions in others...

f 0 y = errorX "QQ"
f _ 2 = 1
f x y = x + y

foldl f 1 [(-1),2]

would return 1 if we used seqX, and raise an exception if we used seq.

I would be nice to have some benchmark numbers to determine whether the performance difference is negligible enough to use seqX over seq.

Copy link
Member Author

Choose a reason for hiding this comment

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

With

{-# LANGUAGE BangPatterns, DataKinds, NoImplicitPrelude, GADTs, ScopedTypeVariables, NumericUnderscores #-}
module Main where

import Clash.Prelude
import Clash.Sized.Vector
import Control.DeepSeq

rnfY :: NFData a => Vec n a -> ()
rnfY = foldl (\() -> rnf) ()

main :: IO ()
main = do
  let !_ = rnfY (repeat @100_000_000 (0 :: Int))
  return ()

and

cabal build clash-prelude && ghc -fforce-recomp Test.hs

and

time ./Test && time ./Test && time ./Test && time ./Test && time ./Test && time ./Test

I get:

seq seqX diff
2.932 5.082 57.69%
2.932 5.152 56.91%
2.902 5.152 56.33%
2.942 5.192 56.66%
2.932 5.182 56.58%
2.932 5.252 55.83%

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it's probably unlikely that you'd write a function where it would make a difference. Still I wouldn't feel comfortable backporting it.

Copy link
Member

@christiaanb christiaanb left a comment

Choose a reason for hiding this comment

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

I would like to see some benchmark numbers on seq vs seqX.

go z Nil = z
go z (Cons x xs) =
let z1 = f z x
in z1 `seq` go z1 xs
Copy link
Member

Choose a reason for hiding this comment

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

In what case would we even observe the difference? The function would be really weird... where it ignores its arguments in some cases, and return exceptions in others...

f 0 y = errorX "QQ"
f _ 2 = 1
f x y = x + y

foldl f 1 [(-1),2]

would return 1 if we used seqX, and raise an exception if we used seq.

I would be nice to have some benchmark numbers to determine whether the performance difference is negligible enough to use seqX over seq.

--
-- Also see:
--
-- https://well-typed.com/blog/90/
Copy link
Member

Choose a reason for hiding this comment

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

But it was posted on April 1st....

Anyhow, I feel this one is the stronger reference to make the change, as it actually argues to make a strict foldl the default. i.e. I would reference it first.

--
-- https://well-typed.com/blog/90/
--
| clashSimulation = go z0 xs0
Copy link
Member

Choose a reason for hiding this comment

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

This will make it that there could be a HDL-vs-Simulation difference if we don't use seqX....

Copy link
Member Author

Choose a reason for hiding this comment

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

In an acceptable way right? X in Haskell simulation => HDL might produce an answer (or X too).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's right.

@martijnbastiaan
Copy link
Member Author

@christiaanb Given the benchmark numbers I think we should pick seq. Though I have no evidence, I'm also inclined to say it's unlikely somewhat would write a function where seq/seqX would make a difference.

@martijnbastiaan martijnbastiaan merged commit 9f1942c into master Mar 12, 2024
12 checks passed
@martijnbastiaan martijnbastiaan deleted the fix2482 branch March 12, 2024 08:48
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.

rnf and rnfX for Vec have a spaceleak
3 participants