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

Introduce Benchmarks #43

Merged
merged 15 commits into from Oct 31, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cabal.project
Expand Up @@ -8,3 +8,4 @@ packages: containers-sig-readonly
containers-contrib
containers-example
containers-sig-laws
containers-sig-benchmarks
4 changes: 4 additions & 0 deletions containers-contrib-readonly/containers-contrib.cabal
Expand Up @@ -20,9 +20,13 @@ source-repository head
library
hs-source-dirs: src
exposed-modules: Map.Contrib.Laws
Map.Contrib.Bench

build-depends: base
, QuickCheck
, containers-sig-readonly
, gauge
, text
, relude
ghc-options: -Wall -fhide-source-paths
default-language: Haskell2010
39 changes: 39 additions & 0 deletions containers-contrib-readonly/src/Map/Contrib/Bench.hs
@@ -0,0 +1,39 @@
{-# LANGUAGE BangPatterns #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE NoImplicitPrelude #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE TypeApplications #-}

module Map.Contrib.Bench where
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you, please, explicit export list to this module so it will become more clear what are the internals and what is the exposed interface?


import Map
import Gauge.Main (Benchmark, bench, defaultMain, whnf)
import Relude hiding (fromList, Map)

benchmark
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this benchmark in a way that it tells about what it does, like benchmarkSequentialLookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this function as if it runs "all" the benchmarks a module will have. In this case, we only have lookupBench and this function runs it only. Do you think we should expose multiple benchmark functions and run them from the containers-sig-benchmarkss Main module?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yigitozkavci Hmm, if this is the function that runs all benchmarks than okay

:: forall k. (Enum k, Bounded k, Key k, NFData (Map k Int))
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of requiring NFData constraint in this benchmark, you can require maps to have NFData instance by adding it in .hsig file here:

instance (Show k, Show v) => Show (Map k v)
instance ( Eq k, Eq v) => Eq (Map k v)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Map.Prim won't (to my knowledge) have a NFData instance, we cannot show the constraint in the signature file. I will wait for the clarification before I move on on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see the problem now. The fact that Map.Prim doesn't have NFData instance really makes things more difficult.

=> String
-> Proxy k
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing Proxy k here you can just not pass it and enable AllowAmbiguousTypes language extension. So instead of passing:

benchmark "some label" (Proxy @T)

you can call this function just as:

benchmark @T "some label"

-XTypeApplications FTW 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I chose not to enable the AllowAmbiguousTypes extension but in this case it might be okay 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Despite the name, AllowAmbiguousTypes is actually a completely safe extension 👌 You only can use it with TypeApplications but TypeApplications is also safe.

-> IO ()
benchmark label _ = do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that structureName is better name for the label variable

let benchMaxElems = min 1024 (fromEnum (maxBound @k))
let upBound = foldl' (.) id (replicate benchMaxElems succ) minBound
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify benchmarks a lot if you pass lower and upper bounds to this function. So then in main we can just create variables:

let lookupRangeLower = 0
let lookupRangeUpper = 1024

and pass them to this function. So the approach is more flexible because we can easily change the range and test with other range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm great, this is the kinda-same thing we've done with the type alias K. Will do 👍

let map_keys :: [k] = [minBound..upBound]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The complexity here exists because I wanted to be able to write benchmarks for any k such that (Enum k, Bounded k, Key k, NFData (Map k Int)) holds. I think it's a good choice but the choice is arguable, obviously.

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra complexity is okay if it allows to have polymoprhic implementation. I see lookup benchmarks from the haskell-perf/dictionaries and I'm horrified by their code...

https://github.com/haskell-perf/dictionaries/blob/2bbc950e9597eea74130a714662b9b2c173ecd02/Time.hs#L307-L321

So I think that you're doing great job here 👍


let mapEntries = zip map_keys [0..]

let m = fromList mapEntries

evaluateNF_ m

defaultMain
[ lookupBench (label <> "-lookup/all") m (map fst mapEntries)
]

lookupBench :: forall k. Key k => String -> Map k Int -> [k] -> Benchmark
lookupBench label m = bench label . whnf (go 0)
where
go :: Int -> [k] -> Int
go = foldl' (\a -> (a +) . fromMaybe 0 . flip lookup m)
2 changes: 1 addition & 1 deletion containers-int-strict/containers-int-strict.cabal
Expand Up @@ -25,7 +25,7 @@ library
exposed-modules: Map.Int
reexported-modules: Map.Int as Map

build-depends: base, containers
build-depends: base, containers, deepseq
ghc-options: -Wall -fhide-source-paths
-fexpose-all-unfoldings
default-language: Haskell2010
7 changes: 5 additions & 2 deletions containers-int-strict/src/Map/Int.hs
Expand Up @@ -34,10 +34,10 @@ module Map.Int
, alter
) where

import Prelude hiding (lookup, null)
import Control.DeepSeq (NFData (..))
import Data.Coerce (coerce)

import qualified Data.IntMap.Strict as M
Copy link
Contributor

Choose a reason for hiding this comment

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

According to our style guide, qualified imports should be in separate section at the end of imports separated by empty line. This helps readability in general.

import Prelude hiding (lookup, null)

newtype Map k v = IM (M.IntMap v)
deriving newtype (Show, Eq)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of writing NFData instance manually you can add it to this list in deriving newtype

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, never used DerivingStrategies before, beautiful!

Expand Down Expand Up @@ -111,3 +111,6 @@ delete = coerce @(k -> M.IntMap v -> M.IntMap v) M.delete
alter :: forall k v. Key k => (Maybe v -> Maybe v) -> k -> Map k v -> Map k v
alter = coerce @((Maybe v -> Maybe v) -> k -> M.IntMap v -> M.IntMap v) M.alter
{-# INLINE alter #-}

instance NFData v => NFData (Map k v) where
rnf (IM m) = rnf m
2 changes: 1 addition & 1 deletion containers-primitive/containers-primitive.cabal
Expand Up @@ -23,7 +23,7 @@ library
exposed-modules: Map.Prim
reexported-modules: Map.Prim as Map

build-depends: base, primitive-containers >= 0.3.0
build-depends: base, primitive-containers >= 0.3.0, deepseq
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
build-depends: base, primitive-containers >= 0.3.0, deepseq
build-depends: base
, deepseq ^>= 1.4
, primitive-containers ^>= 0.3.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not know about cabal 2.0's caret operator, great, I applied the diff :)

ghc-options: -Wall -fhide-source-paths
-fexpose-all-unfoldings
default-language: Haskell2010
2 changes: 2 additions & 0 deletions containers-primitive/src/Map/Prim.hs
Expand Up @@ -20,6 +20,8 @@ module Map.Prim
, elems
) where

import Control.DeepSeq (NFData (..))
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this import is not used here. Probably it's not needed. As well as deepseq dependency for the containers-primitive package.


import Prelude hiding (lookup, null)

import Data.Maybe (fromMaybe, isJust)
Expand Down
13 changes: 13 additions & 0 deletions containers-sig-benchmarks/CHANGELOG.md
@@ -0,0 +1,13 @@
Change log
==========

`containers-benchmark` uses [PVP Versioning][1].
The change log is available [on GitHub][2].

0.0.0
=====
* Initially created.

[1]: https://pvp.haskell.org
[2]: https://github.com/kowainik/containers-backpack/releases