search

snowleopard / hadrian

Hadrian: a new build system for the Glasgow Haskell Compiler. Now merged into the GHC tree!
https://gitlab.haskell.org/ghc/ghc/tree/master/hadrian
MIT License
208 stars 37 forks source link

Rework parsing Cabal metadata #692

Closed snowleopard closed 6 years ago

snowleopard commented 6 years ago

The current implementation of parsing Cabal data is a big mess, which affects performance as described in #671 and #550. In this PR I intend to simplify the implementation and avoid unnecessary reparsing.

This is in a very draft state right now and not ready for review. I'm pushing to do a CI run.

alpmestan commented 6 years ago

Since this is what we're after here, do you think you could quantify the improvement that this patch makes, once it's ready for review (i.e: no rush, I understand it's not quite ready yet) ? Perhaps give the timings for no-op runs (e.g building a complete stage2 compiler when you've already built it), with and without your patch ? If you don't mind.

snowleopard commented 6 years ago

do you think you could quantify the improvement that this patch makes,

@alpmestan Yes, absolutely! I'll try to quantify the impact both on the full build and on the zero build. Presumably the impact on the full build will be negligible, and I hope to get back to a few seconds for the zero build (compared to current 30 seconds).

I'll aim to finalise this PR by end of today.

snowleopard commented 6 years ago

@alpmestan Current status: zero build down to 10 seconds. Changes to full build time seem insignificant.

There is still some work to be done though.

alpmestan commented 6 years ago

@snowleopard Ah, that's already much more reasonable, good work! I certainly won't stand in your way if you want to reduce this further. :)

snowleopard commented 6 years ago

@alpmestan Yep, I hope to reduce the overhead further :)

By the way, I'm seeing lots of CI failures related to atomic. I assume it's not related to this PR.

alpmestan commented 6 years ago

We certainly didn't see those on my last few PRs, did we? In which case it'd quite likely be related to commits recently merged to HEAD.

Since you mentionned atomic builtins, any chance https://github.com/ghc/ghc/commit/ce3897ffd6e7c8b8f36b8e920168bac8c7f836ae is causing them?

snowleopard commented 6 years ago

any chance ghc/ghc@ce3897f is causing them?

This was my first guess too, but it doesn't seem to touch any sensitive code -- it essentially fixes a bug in the configure scripts, and in the worst case the build system would fail during the configuration.

alpmestan commented 6 years ago

Any chance this fixes your problem: https://phabricator.haskell.org/D5163 ?

snowleopard commented 6 years ago

@alpmestan Aha, that's it! Thank you.

snowleopard commented 6 years ago

P.S.: Sorry, got distracted from finishing this PR at work. Will be back in action tomorrow.

snowleopard commented 6 years ago

Got zero build down to 8 seconds but at the cost of one extra oracle. Looks a bit messy...

snowleopard commented 6 years ago

@alpmestan This is now ready for review. On my machine the zero build now takes only 5 seconds. The full build seems to be only marginally faster, but we seem to have greener Travis CI!

Note that this PR fixes both #671 and #550.

alpmestan commented 6 years ago

Well then you're apparently saving enough time to get us below the max duration :-) Great work!

I'll quite likely review this on monday. Can't wait to see it in action.

snowleopard commented 6 years ago

@alpmestan Many thanks for the review -- I fully agree about better documentation. Will address all your comments tomorrow!

snowleopard commented 6 years ago

@alpmestan Thank you for the review! Merged.