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

Implement iforM and iforM_ (#262) #311

Merged
merged 1 commit into from Jun 8, 2020
Merged

Conversation

fumieval
Copy link
Contributor

@fumieval fumieval commented Jun 6, 2020

Added iforM and iforM_, indexed variants of forM and forM_

@Shimuuar
Copy link
Contributor

Shimuuar commented Jun 6, 2020

LGTM

Also I vaguely remember that GHC required fully saturated application to inline. So iforM xs won't be inlined since only one parameter is supplied. Consequently it won't be specialized and everything will be slow. I'll try to invent test case for that

@fumieval
Copy link
Contributor Author

fumieval commented Jun 6, 2020

Right, I copypasted the definition of forM and forM_, assuming that they are written this way deliberately. I can update them (including existing forM) easily

@Shimuuar
Copy link
Contributor

Shimuuar commented Jun 6, 2020

LGTM

Sorry. I take that back. :) You forgot Data.Vector.Primitive

@fumieval
Copy link
Contributor Author

fumieval commented Jun 6, 2020

Good catch!

@Shimuuar
Copy link
Contributor

Shimuuar commented Jun 6, 2020

Amended PR LGTM. @lehins @Bodigrim could any of you take a look it just in case. I already almost missed Primitive reexport

No it seems GHC is good with current definition. Toy benchmark sees no difference:

import qualified Data.Vector as V
import Criterion.Main

fun1 :: (Int -> IO ()) -> IO ()
{-# NOINLINE fun1 #-}
fun1 = V.forM_ (V.generate 4000 id)

fun2 :: (Int -> IO ()) -> IO ()
{-# NOINLINE fun2 #-}
fun2 = \f -> V.forM_ (V.generate 4000 id) f

main :: IO ()
main = defaultMain
  [ bench "1" $ whnfIO (fun1 (\i -> return ()))
  , bench "2" $ whnfIO (fun2 (\i -> return ()))
  ]

-O0

benchmarking 1
time                 14.95 μs   (14.88 μs .. 15.04 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 14.96 μs   (14.91 μs .. 15.04 μs)
std dev              210.7 ns   (150.5 ns .. 332.3 ns)
variance introduced by outliers: 10% (moderately inflated)

benchmarking 2
time                 12.20 μs   (12.18 μs .. 12.21 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 12.15 μs   (12.13 μs .. 12.17 μs)
std dev              62.67 ns   (54.52 ns .. 73.09 ns)

-O

benchmarking 1
time                 11.84 μs   (11.83 μs .. 11.85 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 11.85 μs   (11.84 μs .. 11.86 μs)
std dev              45.18 ns   (35.97 ns .. 58.34 ns)

benchmarking 2
time                 13.23 μs   (13.21 μs .. 13.25 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 13.21 μs   (13.20 μs .. 13.23 μs)
std dev              52.67 ns   (40.70 ns .. 70.79 ns)

Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Great stuff! Sorry for succumbing to unholy nitpicking.

Data/Vector.hs Outdated Show resolved Hide resolved
Data/Vector.hs Outdated Show resolved Hide resolved
@lehins
Copy link
Contributor

lehins commented Jun 8, 2020

Looks great. All comments are addressed. @fumieval Thank you!

@lehins lehins merged commit 4e20ae0 into haskell:master Jun 8, 2020
@fumieval
Copy link
Contributor Author

fumieval commented Jun 8, 2020

Thank you guys too for swift feedbacks!

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

4 participants