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

Use Data.ByteString.Base16.encode. #646

Merged
merged 1 commit into from Oct 28, 2018

Conversation

Projects
None yet
2 participants
@0xd34df00d
Copy link
Contributor

commented Jul 1, 2018

I have a somewhat pages lookup-heavy site (lots of structure with autogenerated TOCs), and profiling shows hash is responsible for 40% of MUT time and roughly 45% of allocations. Replacing concatMap over printf with a bytestring-operating base16 encoding makes hash go away from the first lines of the profile and reduces site build time from ~10 seconds to 5 seconds for me (which looks like a nice improvement!).

I guess the names of the cached data are changed with this modification, but that's a matter of site clean (or site rebuild) anyway.

Hopefully you don't mind an additional dependency on base16-bytestring!

@0xd34df00d

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@jaspervdj kindly pinging — any thoughts on this?

@jaspervdj

This comment has been minimized.

Copy link
Owner

commented Sep 25, 2018

@0xd34df00d Thanks a lot for looking into this! I had no idea this was a problem. Can you try the following?

import Numeric (showHex)

hash = toHex . ...
  where
    toHex :: [Word8] -> String
    toHex [] = ""
    toHex (x : xs)
        | x < 16    = '0' : showHex x (toHex xs)
        | otherwise = showHex x (toHex xs)

I think that should remove a lot of allocations without requiring everyone to rebuild their site. If it doesn't help, I'm happy merging in your patch.

@0xd34df00d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Hey @jaspervdj , sorry for taking so long to try out your suggestion! Indeed, it's improving things too quite a lot! The approach in my patch is just marginally faster (~0.5s on my site), so I'd be happy to go with your suggestion!

Would you do your change to upstream hakyll, or shall I issue another PR/update this one?

@jaspervdj

This comment has been minimized.

Copy link
Owner

commented Oct 26, 2018

@0xd34df00d Can you update this PR or close it and create a new one (whichever is easier for you)? That way you get proper credit. :-)

@0xd34df00d 0xd34df00d force-pushed the 0xd34df00d:master branch from 010e42c to 44e5b09 Oct 27, 2018

@0xd34df00d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

@jaspervdj sure! Done!

@jaspervdj jaspervdj merged commit e2db1b2 into jaspervdj:master Oct 28, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@jaspervdj

This comment has been minimized.

Copy link
Owner

commented Oct 28, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.