Introduce Benchmarks #43

yigitozkavci · 2018-10-27T07:29:51Z

Closes #11

Purpose

Adding benchmarks for assessing runtimes of Map.lookup operation of several packages.

Running the Benchmarks

A new cabal project and executable is added. You can run the benchmarks with:
cabal new-run benchmarks-exe

Questions

There are more operations other than just lookup operation we can do benchmarking for. Shall we add more benchmarks targeting those? If so, shall it be in this PR or in separate PRs?
Shall we add benchmarks to Travis CI to run?
Since Map.Prim module uses Data.Map.Lifted.Lifted behind the scenes and it does not have NFData instance, I could not introduce benchmarks for the module. Can we think of a workaround or do we say "It does make sense that we cannot write a benchmark for it"?

yigitozkavci · 2018-10-27T07:31:49Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

+benchmark label _ = do
+  let benchMaxElems = min 1024 (fromEnum (maxBound @k))
+  let upBound = foldl' (.) id (replicate benchMaxElems succ) minBound
+  let map_keys :: [k] = [minBound..upBound]


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.

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 👍

chshersh

Great work! @yigitozkavci sorry for not returning back to you fast, I was quite busy in last couple days...

I think that existing implementation can be polished a little bit more. But it's really great that you scaffolded initial benchmark suite so later we can just add more benchmarks and improve existing onese 👍

chshersh · 2018-10-29T03:05:00Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

+benchmark
+  :: forall k. (Enum k, Bounded k, Key k, NFData (Map k Int))
+  => String
+  -> Proxy k


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 👍

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

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

chshersh · 2018-10-29T03:05:55Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

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


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

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?

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

chshersh · 2018-10-29T03:13:06Z

containers-int-strict/src/Map/Int.hs

 import qualified Data.IntMap.Strict as M
+import Prelude hiding (lookup, null)

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


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

Oh, never used DerivingStrategies before, beautiful!

chshersh · 2018-10-29T03:15:04Z

containers-primitive/containers-primitive.cabal

@@ -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


Suggested change

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

build-depends: base

, deepseq ^>= 1.4

, primitive-containers ^>= 0.3.0

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

chshersh · 2018-10-29T03:15:55Z

containers-primitive/src/Map/Prim.hs

@@ -20,6 +20,8 @@ module Map.Prim
       , elems
       ) where

+import Control.DeepSeq (NFData (..))


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

chshersh · 2018-10-29T03:23:50Z

containers-sig-benchmarks/src/Main.hs

+main = do
+  BIRO.benchmark "intmap" (Proxy @K)
+
+  BORO.benchmark "ordmap" (Proxy @K)


Looks like benchmarks for primitive-containers are missing here...

Yep, I included this matter in "Questions" section of the PR description, I simply could not figure out how I could implement it since Prim uses Data.Map.Lifted.Lifted inside.

chshersh · 2018-10-29T03:29:11Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

+benchmark label _ = do
+  let benchMaxElems = min 1024 (fromEnum (maxBound @k))
+  let upBound = foldl' (.) id (replicate benchMaxElems succ) minBound
+  let map_keys :: [k] = [minBound..upBound]


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 👍

chshersh · 2018-10-29T03:32:57Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

+  -> IO ()
+benchmark label _ = do
+  let benchMaxElems = min 1024 (fromEnum (maxBound @k))
+  let upBound = foldl' (.) id (replicate benchMaxElems succ) minBound


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.

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

chshersh · 2018-10-29T03:33:51Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

+{-# LANGUAGE FlexibleContexts #-}
+{-# LANGUAGE TypeApplications #-}
+
+module Map.Contrib.Bench where


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?

chshersh · 2018-10-29T03:35:10Z

containers-contrib-readonly/src/Map/Contrib/Bench.hs

+  => String
+  -> Proxy k
+  -> IO ()
+benchmark label _ = do


I think that structureName is better name for the label variable

yigitozkavci · 2018-10-29T13:35:21Z

Hi @chshersh, thank you for the detailed review. After applying your review code seems to work well, but there is a concern:

Because I cannot write a NFData instance for Map.Prim, we cannot have the constraint in Map.hsig file since it breaks the property tests build (as you can see the failures in TravisCI in the past commits). We should either be able to write NFData for Map.Prim or we leave it as it is.

chshersh · 2018-10-31T04:44:04Z

@yigitozkavci Let's solve problem with primitive-containers package under separate issue. Looks like this won't be an easy problem.

chshersh

@yigitozkavci This looks really great! Looks like we can merge this. Thank you again very much!

yigitozkavci added 3 commits October 23, 2018 10:55

Fix merge and introduce benchs

85cb10a

Fix cabalfile for the feature

d2de104

Rename benchmark project

cdc79fc

yigitozkavci commented Oct 27, 2018

View reviewed changes

Replace manually written fold with foldl'

89e6071

chshersh requested review from chshersh and vrom911 October 29, 2018 03:01

chshersh assigned yigitozkavci Oct 29, 2018

chshersh added the bench label Oct 29, 2018

chshersh added this to In progress in #2: Hacktoberfest (October, 2018) via automation Oct 29, 2018

chshersh added the Hacktoberfest https://hacktoberfest.digitalocean.com/ label Oct 29, 2018

chshersh reviewed Oct 29, 2018

View reviewed changes

yigitozkavci added 11 commits October 29, 2018 08:41

Get rid of Proxy by allowing ambiguous types

c3970b0

Utilise DerivingStrategies for NFData instance of Data.IntMap

13ef0cc

Use Cabal 2.0 PvP caret operator for deepseq dependency contstraints

5b25dd9

Remove unused deepseq dependency and NFData import

284b650

Move Map's NFData constraint to signature file

9ff9933

Organise imports according to the design guide

1d07081

Group benchmarks properly

cd097b7

Make bounds parametric in benchmark-exe's Main module

14630a2

Use explicit export in Map.Contrib.Bench module

1f4bc9c

Rename 'label' parameter in Map.Contrib.Bench module

cda3eab

Revert the change regarding having NFData constraint on sig file

335158b

#2: Hacktoberfest (October, 2018) automation moved this from In progress to Reviewer approved Oct 31, 2018

chshersh approved these changes Oct 31, 2018

View reviewed changes

chshersh merged commit 267fcde into kowainik:master Oct 31, 2018

#2: Hacktoberfest (October, 2018) automation moved this from Reviewer approved to Done: PR's Oct 31, 2018

chshersh mentioned this pull request Oct 31, 2018

Figure out way to add primitive-containers to benchmarks #44

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Benchmarks #43

Introduce Benchmarks #43

yigitozkavci commented Oct 27, 2018

yigitozkavci Oct 27, 2018

chshersh Oct 29, 2018

chshersh left a comment

chshersh Oct 29, 2018

yigitozkavci Oct 29, 2018

chshersh Oct 31, 2018

chshersh Oct 29, 2018

yigitozkavci Oct 29, 2018

chshersh Oct 31, 2018

chshersh Oct 29, 2018

yigitozkavci Oct 29, 2018

chshersh Oct 29, 2018

yigitozkavci Oct 29, 2018

chshersh Oct 29, 2018

chshersh Oct 29, 2018

yigitozkavci Oct 29, 2018

chshersh Oct 29, 2018

chshersh Oct 29, 2018

yigitozkavci Oct 29, 2018

chshersh Oct 29, 2018

chshersh Oct 29, 2018

yigitozkavci commented Oct 29, 2018

chshersh commented Oct 31, 2018

chshersh left a comment

Introduce Benchmarks #43

Introduce Benchmarks #43

Conversation

yigitozkavci commented Oct 27, 2018

Purpose

Running the Benchmarks

Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yigitozkavci commented Oct 29, 2018

chshersh commented Oct 31, 2018

chshersh left a comment

Choose a reason for hiding this comment