search

IntersectMBO / plutus

The Plutus language implementation and tools
Apache License 2.0
1.57k stars 476 forks source link

Performance issues with `applyCostModelParams` #4962

Closed koslambrou closed 1 year ago

koslambrou commented 2 years ago

Summary

We noticed some significant performance issues with Cardano.Api.Fees.evaluateTransactionFee and Cardano.Api.TxBody.makeTransactionBody. After profiling the code, we noticed that the reason was because the functions would convert Cardano.Api.ProtocolParameters to cardano-ledger's PParams every time we call one of the functions. However, the conversion between the two datatypes take s a significant amount of time because of a plutus-core function (applyCostModelParams ) when creating the cost model.

An issue was created on cardano-node repo https://github.com/input-output-hk/cardano-node/issues/4622 in order to bypass the conversion.

Steps to reproduce the behavior

Run profiling on a code which uses any function from cardano-api which uses toLedgerPParams such as makeTransactionBody and evaluateTransactionFee.

Actual Result

Slow

Expected Result

Faster

Describe the approach you would take to fix this

No idea.

System info

OS: NixOS Version: master commit hash: a56c96598b4b25c9e28215214d25189331087244

thealmarty commented 1 year ago

I'm unsure if this can be solved, but maybe Kenneth can take a look.

kwxm commented 1 year ago

There's an issue for this at PLT-1243, and it seems that @effectfully has some ideas. We should try to find some time to work on it.

effectfully commented 1 year ago

There was some more discussion here. I think the conclusion was that we shouldn't worry. I'd be fine if this issue was simply closed.

thealmarty commented 1 year ago

As Kenneth pointed out, ticket created already. We can close this when we finish it (PLT-1243).

kwxm commented 1 year ago

Checking the various discussions mentioned, it seems that the solution is that callers should call applyCostModelParams once and cache the results (presumably the ledger has to cache the result for every set of protocol parameters it encounters). Ideally we'd make applyCostModelParams a lot faster, but we've thought about that and couldn't come up with a good solution: let's keep thinking though (that's what PLT-1243 is). Perhaps we should put a prominent comment in CostModelInterface.hs so that no-one gets bitten by this in future.

So do we think it's safe to close this? Any comments from @koslambrou / @michaelpj / @zliu41 ?

effectfully commented 1 year ago

Ideally we'd make applyCostModelParams a lot faster, but we've thought about that and couldn't come up with a good solution

I have one idea that I'm pretty sure is going to help with at least a part of the issue (maybe not a bottleneck, though) and one hunch that I never had the time/need to investigate. The idea is to stop doing quadratic things when superlinear will suffice (documented somewhere in Jira) and a hunch is that we can structure applyCostModelParams in a way that allows GHC to pull the most expensive stuff out of it, so that it gets cached automatically at each call site. I'm not sure if the latter is worth it, given that explicit caching is simple and reliable, unlike implicit one.

kwxm commented 1 year ago

explicit caching is simple and reliable, unlike implicit one.

Agreed.

koslambrou commented 1 year ago

@kwxm I'm good with closing it. Explicit caching is the way to go :)

kwxm commented 1 year ago

Oh, GitHub seems to have automatically closed this when I merged the branch. I didn't know it could do that. It seems that it's because of the "Closes #4962" comment.