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

Benchmark and optimise reward calculation #1851

Merged
merged 6 commits into from Sep 15, 2020
Merged

Benchmark and optimise reward calculation #1851

merged 6 commits into from Sep 15, 2020

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented Sep 14, 2020

This PR adds a micro-benchmark for the reward update creation and subsequently optimises that operation.

Along the way, we do a few things:

  • Make some formatting changes (mostly splitting very long lines)
  • Add an ability to plug in a custom TX generator to the block generator (we use this to generate empty blocks).
  • Add a function in the benchmarks package to generate a long chain of empty blocks, with delegation initiated at genesis. This moves one function from ouroboros-consensus into shelley-spec-ledger-test.
  • Adds two benchmarks:
    • One for createRUpd, built upon a generated chain with some praos blocks issued.
    • One for likelihood, which was the majority of the reward calculation
  • Optimise the likelihood calculation. This brings the micro-benchmark down from 50-60ms to around 1.

@nc6
Copy link
Contributor Author

nc6 commented Sep 14, 2020

Note that after this PR the createRUpd function is dominated by this epression:

 Stake $ eval (dom (delegs ▷ Set.singleton hk) ◁ stake)

This already has an optimisation in SetAlgebraInternal:

 -- This case inspired by set expression in EpochBoundary.hs
compute (DRestrict (Dom (RRestrict (Base MapR delegs) (SetSingleton hk))) (Base MapR state)) =
   materialize MapR (do { (x,y,z) <- delegs `domEq` state; when (not (y==hk)); one(x,z) })

So unless @TimSheard spots anything that could be improved there, this is probably in a decent state.

Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

looks good to me, thank you @nc6 !

@TimSheard does have some ideas for how to make it even faster.

Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

This looks good, but Nicks comment about the bottleneck, reminded me about another optimization for a similiar SetAlgebra expression found in LedgerState.hs. This was made considerably faster by writing a function over Data.Maps directy. This function was called intersecDomP, I wrote a simliar function intersectDomPLeft, that applie the same kind of improvements.

I wrote some property tests that test that both functions meet their specifcation.
Benchmarking lead to another improvement where we normailize all singleton sets into one kind of Exp. This allows fewer pattern matches.

Nicks Benchmark rewards/createRUpd varys greatly from run to run (lowest 24 ms greatest 928 ms), Swapping in interesectdomPLeft, also varies greatly but across 10 runs, consistently averages 2-3 times faster.

Mostly breaking some very long lines.
This currently exists in the library code of
ouroboros-consensus-shelley. But it would be good to replace that
function with this one, and ensure it's used only in testing.
This is the main component of the reward calculation.
Per the micro-benchmark for likelihood, this reduces the time from
around 50-60ms to around 1. Since this accounts for about 60-70% of the
time spend in `createRUpd`, we should hope to see at least a doubling of
performance there.

The optimisation is achieved by calculating `Set.fromList` only once,
rather than for each reward calculation.
@nc6
Copy link
Contributor Author

nc6 commented Sep 15, 2020

@TimSheard That last commit seems to cause the tests to break; since we wanted to have this in the release today, I've split it into a separate PR (#1857)

@nc6 nc6 merged commit f7e6c7c into master Sep 15, 2020
@iohk-bors iohk-bors bot deleted the nc/benchRewards branch September 15, 2020 11:02
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM

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

4 participants