Skip to content
This repository has been archived by the owner on Jun 17, 2020. It is now read-only.

[DEVOPS-121] Cardano sl faster build #49

Closed
wants to merge 7 commits into from

Conversation

deepfire
Copy link
Contributor

@deepfire deepfire commented May 20, 2017

Besides partially addressing https://issues.serokell.io/issue/DEVOPS-36#comment=96-3716, this also sets up NIX_PATH, so that a number of tools needn't have -I specified -- and so this pulls towards unification of nixpkgs specification (don't remember the issue).

cc @domenkozar, @jmitchell

TODO:

  • decide on testing being a part of all Hydra builds
  • decide on whether building all 13 executables is worth it for all Hydra builds

@deepfire deepfire requested a review from domenkozar May 20, 2017 16:37
default.nix Outdated
@@ -11,6 +11,13 @@ let
});
socket-io-src = pkgs.fetchgit (removeAttrs (lib.importJSON ./pkgs/engine-io.json) ["date"]);
cardano-sl-src = pkgs.fetchgit (removeAttrs (lib.importJSON ./pkgs/cardano-sl.json) ["date"]);
cores = 4;
Copy link
Contributor

@domenkozar domenkozar May 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to have threads and cores should be part of the function inputs for default.nix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

default.nix Outdated
buildTarget = subset;
});
buildSpeedupFlagsMarlowScaling = drv : appendConfigureFlag drv
"--ghc-option=-rtsopts --ghc-option=+RTS --ghc-option=-N${toString threads} --ghc-option=-A128m --ghc-option=-n2m --ghc-option=-qb0 --ghc-option=-qn${toString cores} --ghc-option=-RTS";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-N is already set from cabal in cardano-sl

Copy link
Contributor Author

@deepfire deepfire May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The problem here is.. we need to set both -N and -qn, in the 2:1 ratio, as per Simon Marlow's suggestion (see https://ghc.haskell.org/trac/ghc/ticket/9221#comment:72).
  2. Once we accept №1, a need comes to have an authoritative Nix-evaluation-time source of information on the amount of cores on the system.
  3. I couldn't find such a source, hence the hard-coding.
  4. Since we hard-code -qn, I guess it's kinda debatable if leaving -N free is a good thing. It would certainly somewhat obscure the Marlow ratio between cores and threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, @domenkozar, I can't find where cabal sets -N -- at least it doesn't in https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/haskell-modules/generic-builder.nix

Cabal does set -j, but that's ghc --make-level parallelism.

@domenkozar
Copy link
Contributor

For -static we should skip tests and build only targets we need for deployments. cardano-sl should still be a full build with tests etc.

I'd like to see time differences here before/after this PR and with current VM and 16 core VM.

@deepfire-pusher
Copy link

deepfire-pusher commented May 21, 2017

@domenkozar, regarding the testing setup:

  1. the 16-core VM, should it be a new one, or the currently-provisioned i3.2xlarge is sufficient?
  2. Also, your spec implies a 2x2 test matrix with 4 results, right?

@deepfire-pusher
Copy link

deepfire-pusher commented May 21, 2017

@domenkozar, regarding the second question from the PR TODO:

How do we subset the 13 executables -- are we clear on what is needed for deployments?

Should I judge this myself? Perhaps incremental correctness, with an optimistic baseline, can work here : -)

@domenkozar
Copy link
Contributor

So far we need cardano-node and cardano-explorer binaries (I just grepped /bin/)

@domenkozar
Copy link
Contributor

  1. That should be good enough
  2. 4 results: before/after, 2 core/8 core

@deepfire-pusher
Copy link

@domenkozar, thanks, I'll try to do this today

@domenkozar
Copy link
Contributor

@deepfire-pusher I see now that you have updated the issue. Those statistics are good enough.

Let's build only cardano-node and cardano-explorer executables and that should be more than good enough.

@domenkozar
Copy link
Contributor

Note that nix already pases -j$NIX_BUILD_CORES to ghc, so it's not single threaded by default.

@deepfire-pusher
Copy link

deepfire-pusher commented May 21, 2017

If it's already parallel.. well, I that's scary, it means that hydra.iohk.io is even worse, single-threaded, than I thought : -)

@domenkozar
Copy link
Contributor

It's slower because there are multiple packages compiling together. Up to 4.

@deepfire-pusher
Copy link

deepfire-pusher commented May 21, 2017

Ah, I see -- that was the problem I was trying to attack initially, with my Hydra instrumentation -- the direct comparability of measurements.

But I agree, just running the tests manually everywhere is faster, and the result is useful for judgement of effect of changes.

@deepfire deepfire force-pushed the cardano-sl-faster-build branch from 194b5f6 to 2f2e970 Compare May 22, 2017 10:55
@deepfire
Copy link
Contributor Author

@domenkozar, what is our next step here?

@deepfire deepfire changed the title [DEVOPS-36] Cardano sl faster build [DEVOPS-121] Cardano sl faster build May 29, 2017
@domenkozar
Copy link
Contributor

This will be handled as part of DEVOPS-121.

@deepfire deepfire force-pushed the cardano-sl-faster-build branch from 1cecf26 to 09a7bbb Compare June 8, 2017 17:40
@deepfire deepfire force-pushed the cardano-sl-faster-build branch from 09a7bbb to 041f668 Compare June 16, 2017 14:35
@domenkozar
Copy link
Contributor

I'm going to close this, as expressions moved to cardano-sl repo and we can revisit the PR once DEVOPS-121 is picked up.

@domenkozar domenkozar closed this Oct 6, 2017
@deepfire deepfire deleted the cardano-sl-faster-build branch October 30, 2017 14:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants