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

PoC: INLINE parser combinators #591

Merged
merged 7 commits into from
Sep 19, 2018
Merged

PoC: INLINE parser combinators #591

merged 7 commits into from
Sep 19, 2018

Conversation

phadej
Copy link
Contributor

@phadej phadej commented Sep 18, 2018

On my machine, trying dhall-kubernetes/examples/deployment.dhall the execution time drops significantly:

-15,31s user 0,30s system 47% cpu 33,174 total
+ 8,45s user 0,19s system 72% cpu 11,986 total

The flip side of the coin, is that Token module tortures GHC with all stuff wanting to INLINE. Yet SPJ said about a week ago

I've had a fix pending in my tree for months, but I keep getting pre-empted. Your new ticket will help incentivise me.

in https://ghc.haskell.org/trac/ghc/ticket/15630, so that issue might go away.


I'm not sure inlining on which (set of) functor/applicative/alternative/monad combinator(s) contributes the most, but seems that here as well, inlining composition combinators pays off in run time.


I hope that @f-f or someone else can continue from here. Cheers.

@Gabriella439
Copy link
Collaborator

@phadej: Thank you! 🙂

The test failure is weird and seems like it should not be related to this change at all. I'm digging into it

@Gabriella439
Copy link
Collaborator

@phadej: Oh, I know what the problem is. You also need to implement fail, too

@Gabriella439
Copy link
Collaborator

Thanks again!

@Gabriella439
Copy link
Collaborator

Before I merge, is there any reason not to merge this? This was titled a "Proof of concept" but it looks fine to me. Is it just because there are other things which could be inlined?

@phadej
Copy link
Contributor Author

phadej commented Sep 18, 2018

@Gabriel439 I'm not entirely sure this is optimization.

It seems that this actually doesn't really affect non-profiled build.

And with profiled, both master and this branch have very similar

COST CENTRE            MODULE                     SRC                                                 %time %alloc

noted                  Dhall.Parser.Expression    src/Dhall/Parser/Expression.hs:(31,1)-(38,58)        37.1   18.1
reserved               Dhall.Parser.Token         src/Dhall/Parser/Token.hs:402:1-56                   12.0    9.6
simpleLabel            Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(160,1)-(169,76)           11.6   19.0
takeWhile              Dhall.Parser.Combinators   src/Dhall/Parser/Combinators.hs:134:1-75              5.0    6.4
splitAt                Data.Text                  Data/Text.hs:(1339,1)-(1343,63)                       4.8   15.2
makeOperatorExpression Dhall.Parser.Expression    src/Dhall/Parser/Expression.hs:(169,1)-(173,40)       2.1    1.2
naturalLiteral         Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(68,1)-(70,51)              2.1    3.0
pathComponent          Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(237,1)-(241,34)            1.9    2.8
doubleLiteral          Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(53,1)-(57,42)              1.6    2.2
blockComment           Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(131,1)-(133,24)            1.1    0.5
whitespaceChunk        Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(84,1)-(92,52)              1.1    1.4
localRaw               Dhall.Parser.Expression    src/Dhall/Parser/Expression.hs:(729,1)-(758,36)       1.0    1.4
integerLiteral         Dhall.Parser.Token         src/Dhall/Parser/Token.hs:(60,1)-(65,43)              0.9    1.9
toList                 Data.HashMap.Strict.InsOrd src/Data/HashMap/Strict/InsOrd.hs:(580,1)-(584,22)    0.6    1.7
primitiveExpression    Dhall.Parser.Expression    src/Dhall/Parser/Expression.hs:(256,1)-(465,16)       0.6    1.4

(I'm using GHC-8.2.2 btw, will try 8.4.3 next)

@phadej
Copy link
Contributor Author

phadej commented Sep 18, 2018

The last commit is something one might want to profile in separation. On my machine it halved the time taken by noted. (EDIT: I'm quite sure about that).

@Gabriella439
Copy link
Collaborator

Then I think we should go ahead and merge this whenever you're ready. It can't hurt and it will make it easier to profile if we eliminate profiling false positives caused by the absence of inlining

This way GHC can be smarter, e.g. reserved and keyword disappear
from the profiling output
@phadej
Copy link
Contributor Author

phadej commented Sep 18, 2018

Ok, I run out of low hanging fruits:

master

  INIT    time    0.000s  (  0.000s elapsed)
  MUT     time    6.256s  (  9.445s elapsed)
  GC      time    1.998s  (  1.997s elapsed)
  EXIT    time    0.003s  (  0.003s elapsed)
  Total   time    8.257s  ( 11.445s elapsed)

  %GC     time      24.2%  (17.4% elapsed)

prof

  INIT    time    0.000s  (  0.000s elapsed)
  MUT     time    4.099s  (  6.744s elapsed)
  GC      time    2.077s  (  2.076s elapsed)
  EXIT    time    0.003s  (  0.003s elapsed)
  Total   time    6.180s  (  8.824s elapsed)

  %GC     time      33.6%  (23.5% elapsed)

There is 20-25% speedup now.

@Gabriella439
Copy link
Collaborator

Alright, then I'll go ahead and merge this

@Gabriella439 Gabriella439 merged commit b60f8f6 into dhall-lang:master Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants