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.

Sign up for GitHub

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

Jump to bottom

Reduce number of calls to toLedgerPParams #4903

Merged
merged 2 commits into from
Mar 11, 2023
Merged

Reduce number of calls to toLedgerPParams #4903

merged 2 commits into from
Mar 11, 2023
+177 −133

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Feb 21, 2023 •
edited
Loading

We call toLedgerPParams multiple times with the same arguments. This function is expensive.

The PR calls toLedgerPParams once then re-uses the result. It is necessary to introduce new functions and change the signature of existing functions to do this.

Resolves #4622

@newhoggy newhoggy force-pushed the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch 2 times, most recently from 1115ae1 to 916f76a Compare February 22, 2023 00:45
@newhoggy newhoggy marked this pull request as ready for review February 22, 2023 00:47
@newhoggy newhoggy changed the title Reduce number of calls to to toLedgerPParams Reduce number of calls totoLedgerPParams Feb 22, 2023
@newhoggy newhoggy changed the title Reduce number of calls totoLedgerPParams Reduce number of calls to toLedgerPParams Feb 22, 2023
@berewt
Copy link

berewt commented Feb 22, 2023

FWIW, from a user perspective (in plutus-apps), I'd be happy with this change. 👍

Jimbo4350
Jimbo4350 reviewed Feb 24, 2023
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice 👍 A few minor comments

cardano-api/src/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Fees.hs Outdated
@@ -236,31 +237,30 @@ estimateTransactionFee _ _ _ (ByronTx _) =
--
evaluateTransactionFee :: forall era.
IsShelleyBasedEra era
=> ProtocolParameters
=> Ledger.PParams (ShelleyLedgerEra era)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably the most sensible/least complicated way to go even if we are breaking the interface. We need to expose Ledger.PParams via Cardano.Api.Shelley

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to update the changelog with the breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change log updated.

cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Convenience/Query.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Eras.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated
-> Ledger.PParams (ShelleyLedgerEra era)
-> BundledProtocolParameters era

bundleProtocolParams :: CardanoEraStyle era -> ProtocolParameters -> BundledProtocolParameters era
Copy link
Contributor

Choose a reason for hiding this comment

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

We can refactor this by changing the type sig to bundleProtocolParams :: CardanoEra era -> ProtocolParameters -> BundledProtocolParameters era and moving the call to cardanoEraStyle into this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@newhoggy newhoggy force-pushed the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch 2 times, most recently from 3ae784b to 9b36afb Compare March 3, 2023 13:50
@lehins
Copy link
Contributor

lehins commented Mar 3, 2023 •
edited
Loading

@newhoggy Now that we can build this repo with ghc-9.2.5, do you check see if this problem was fixed?https://gitlab.haskell.org/ghc/ghc/-/issues/20036

I suspect the reason why toLedgerPParams is so slow is because we are using noInlineMaybeStrictToMaybe, which was introduced to speed up compilation times in #3275

I suspect that it was actually fixed. And if it is then we could switch back to the maybeStrictToMaybe version that is inlined and thus speed up the toLedgerPParams function back again. We could also finally close the GHC ticket too, which would be a nice thing to do.

In case that ghc-8.10.7 is still important we could even do this to not mess up the older version:

#if __GLASGOW_HASKELL__ >= 902
{-# INLINE noInlineMaybeToStrictMaybe #-}
#else
{-# NOINLINE noInlineMaybeToStrictMaybe #-}
#endif

Edit - I attached my ddump-timings results for the module, but it would be better if you could confirm it yourself
20036-timing-results.tar.gz

@lehins lehins mentioned this pull request Mar 5, 2023
Merged
Jimbo4350
Jimbo4350 approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! One minor comment.

cardano-api/ChangeLog.md Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch from 9b36afb to 1ae6005 Compare March 8, 2023 03:05
@newhoggy newhoggy enabled auto-merge March 8, 2023 03:05
@newhoggy newhoggy force-pushed the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch 4 times, most recently from 65c0024 to bc0539f Compare March 10, 2023 13:48
@newhoggy newhoggy added this pull request to the merge queue Mar 10, 2023
@newhoggy newhoggy removed this pull request from the merge queue due to a manual request Mar 10, 2023
@newhoggy newhoggy force-pushed the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch from bc0539f to 668f030 Compare March 10, 2023 14:05
@newhoggy newhoggy enabled auto-merge March 10, 2023 14:06
@newhoggy newhoggy added this pull request to the merge queue Mar 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 10, 2023
@newhoggy
Pass Ledger.PParams (ShelleyLedgerEra era) as arguments so that it ca…
228889f 
…n be computed once as passed around.

Bundle ProtocolParameters and Ledger.PParams (ShelleyLedgerEra era) into BundledProtocolParams era so that both case be passed around together.  The constructor arguments are lazy so that values that are expensive to compute aren't computed unecessarily.
Update changelog
@newhoggy newhoggy force-pushed the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch from 668f030 to 228889f Compare March 10, 2023 23:17
@newhoggy newhoggy enabled auto-merge March 10, 2023 23:20
@newhoggy newhoggy added this pull request to the merge queue Mar 10, 2023
Merged via the queue into master with commit c02b0ab Mar 11, 2023
@iohk-bors iohk-bors bot deleted the newhoggy/reduce-number-of-calls-to-toLedgerPParams branch March 11, 2023 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@deepfire deepfire Awaiting requested review from deepfire

@jutaro jutaro Awaiting requested review from jutaro

@coot coot Awaiting requested review from coot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[PERF] - Performance issues with functions using toLedgerPParams in cardano-api
4 participants
@newhoggy @berewt @lehins @Jimbo4350