-
Notifications
You must be signed in to change notification settings - Fork 22
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
Benchmarking Streamly backend #54
Conversation
ping @ocramz |
Also the break in the tests seem unrelated to the PR. It looks like dh-core/dh-core has a dependency conflict, which is surprising since everything was fine at the last commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good, but I'd rather have a couple minor changes before merging. Thanks!
analyze/analyze.cabal
Outdated
text , | ||
unordered-containers , | ||
vector , | ||
weigh , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
criterion and weigh only need to be in the "library" section only if you want to run benchmarks in GHCi, this is why I put some imports under "DEBUG". They will all be removed from the "library" section when we release this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! I didn't think it was actually going to be merged, I thought it was just to treat the benchmark as a separate issue!
analyze/test/benchStreamly.hs
Outdated
{-# LANGUAGE GADTs #-} | ||
|
||
|
||
module Main where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you move this into a new bench
folder? test
should be just for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed
analyze/test/benchStreamly.hs
Outdated
import qualified Weigh as W | ||
|
||
|
||
-- implementation details required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that below this line are the back-end details required to use Streamly
analyze/test/benchStreamly.hs
Outdated
n = 1500 | ||
|
||
testKeys :: Vector Text | ||
testKeys = unsafePerformIO $ V.replicateM n $ liftM (T.pack . take 10 . randomRs ('a','z')) newStdGen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is unsafePerformIO
for, here and in the following?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ocramz Using unsafePerformIO
the value of testKeys
and testData
is determined only once (and so we avoid the overhead of creating a new set of data for each function to test. Although a nicer way to do this would be to have said functions accept the data as input arguments, and then bind the data once in speed
and in weight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the only thing I'm concerned about is that Travis is red now. Could you fix that?
I can't think of why this is broken now. Need some sleep first
analyze -> criterion -> statistics-15.0.0.0 (from lts) -> DLA-0.1.0.0, while package locally is 0.2.0.0 so, we use statistics 0.14.0.2, which is allowed by criterion, and doesn't import DLA
Also I removed the import of |
And it works! |
Very nice, thank you! |
Hi @Magalame , I just noticed that you forgot a few things from this PR. Please see these two commits I made to fix it : I'm glad to include your contributions, but please be more careful and submit fully-working code next time. |
@ocramz I'm very sorry to hear that, I'm fairly inexperienced with stack/Haskell tooling in general so I might miss some things, but I'll be extra careful |
See #35
I'm pretty sure my modifications of
analyze.cabal
are overkill at some point, but it should get the benchmark running.Using the recommended flags by
Streamly
, I have for benchmarking runtime:stack ghc -- -package streamly-0.6.0 -package vector-0.12.0.3 --make benchStreamly.hs -main-is speed -O2 -fspec-constr-recursive=10
Giving:
For memory use, we just change the target:
stack ghc -- -package streamly-0.6.0 -package vector-0.12.0.3 --make benchStreamly.hs -main-is weight -O2 -fspec-constr-recursive=10
yielding
It's very possible I've let a foolish mistake somewhere. However
Streamly
does seem promising in terms of speed, at the cost of a slight increase in memory consumption.