Skip to content

Implement iforM and iforM_ (#262) - #311

Merged
lehins merged 1 commit into
haskell:masterfrom
fumieval:master
Jun 8, 2020
Merged

Implement iforM and iforM_ (#262)#311
lehins merged 1 commit into
haskell:masterfrom
fumieval:master

Conversation

@fumieval

@fumieval fumieval commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

Added iforM and iforM_, indexed variants of forM and forM_

@Shimuuar

Shimuuar commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

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

fumieval commented Jun 6, 2020

Copy link
Copy Markdown
Contributor Author

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

Shimuuar commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

LGTM

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

@fumieval

fumieval commented Jun 6, 2020

Copy link
Copy Markdown
Contributor Author

Good catch!

@Shimuuar

Shimuuar commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

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)

Bodigrim
Bodigrim reviewed Jun 7, 2020
View reviewed changes

@Bodigrim Bodigrim left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread Data/Vector.hs Outdated
Comment thread Data/Vector.hs Outdated
@lehins

lehins commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

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

@lehins
lehins merged commit 4e20ae0 into haskell:master Jun 8, 2020
@fumieval

fumieval commented Jun 8, 2020

Copy link
Copy Markdown
Contributor Author

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

Reviewers

@Bodigrim Bodigrim Bodigrim approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fumieval @Shimuuar @lehins @Bodigrim