-
Notifications
You must be signed in to change notification settings - Fork 473
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
[Costing] Provide support for multiple 'CostModel's #5851
Changes from 36 commits
49db6d5
e1d60b6
3c40023
1705f9e
32c08f0
3b5bd38
3f96021
430d7f9
aef3def
21bb075
8fa97cc
a9a71bd
3944b7f
0a1af53
1843823
3134e88
86a176d
53b49c2
d2b57a6
b6c9c3c
69fc298
12679c7
6151af8
20e2677
ecc9a85
e543b28
a336d02
12bbcc1
4d3ddb0
7a23b0e
846ae8e
5bf9d5e
ae5718b
809675d
28f46c7
9d8942d
039dce1
60bbf02
1f1bf5e
e507dae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,6 @@ import PlutusCore.Evaluation.Machine.ExBudgetStream | |
import PlutusCore.Evaluation.Machine.ExMemoryUsage | ||
import PlutusCore.Name.Unique | ||
|
||
import Control.DeepSeq | ||
import Data.Array | ||
import Data.Kind qualified as GHC | ||
import Data.Proxy | ||
|
@@ -399,22 +398,15 @@ toBuiltinsRuntime | |
-> cost | ||
-> BuiltinsRuntime fun val | ||
toBuiltinsRuntime semvar cost = | ||
let runtime = BuiltinsRuntime $ toBuiltinRuntime cost . inline toBuiltinMeaning semvar | ||
-- This pragma is very important, removing it destroys the carefully set up optimizations of | ||
-- of costing functions (see Note [Optimizations of runCostingFun*]). The reason for that is | ||
-- that if @runtime@ doesn't have a pragma, then GHC sees that it's only referenced once and | ||
-- inlines it below, together with this entire function (since we tell GHC to), at which | ||
-- point everything's inlined and we're completely at GHC's mercy to optimize things | ||
-- properly. Unfortunately, GHC doesn't want to cooperate and push 'toBuiltinRuntime' to | ||
-- the inside of the inlined to 'toBuiltinMeaning' call, creating lots of 'BuiltinMeaning's | ||
-- instead of 'BuiltinRuntime's with the former hiding the costing optimizations behind a | ||
-- lambda binding the @cost@ variable, which renders all the optimizations useless. By | ||
-- using a @NOINLINE@ pragma we tell GHC to create a separate thunk, which it can properly | ||
-- optimize, because the other bazillion things don't get in the way. | ||
{-# NOINLINE runtime #-} | ||
in | ||
-- Force each 'BuiltinRuntime' to WHNF, so that the thunk is allocated and forced at | ||
-- initialization time rather than at runtime. Not that we'd lose much by not forcing all | ||
-- 'BuiltinRuntime's here, but why pay even very little if there's an easy way not to pay. | ||
force runtime | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now have force in |
||
-- A call to 'lazy' is to make sure that the returned 'BuiltinsRuntime' is properly cached in a | ||
-- 'let'-binding. This makes it easier for GHC to optimize the internals of builtins, because | ||
-- without a 'let'-binding GHC would sometimes refuse to cooperate and push 'toBuiltinRuntime' | ||
-- to the inside of the inlined 'toBuiltinMeaning' call, creating lots of 'BuiltinMeaning's | ||
-- instead of 'BuiltinRuntime's with the former hiding the costing optimizations behind a lambda | ||
-- binding the @cost@ variable, which makes the optimizations useless. | ||
-- By using 'lazy' we tell GHC to create a separate thunk, which it can properly optimize, | ||
-- because the other bazillion things don't get in the way. We used to use an explicit | ||
-- 'let'-binding marked with @NOINLINE@, but that turned out to be unreliable, because GHC | ||
-- feels free to turn it into a join point instead of a proper thunk. | ||
lazy . BuiltinsRuntime $ toBuiltinRuntime cost . inline toBuiltinMeaning semvar | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I said during the GHC Core presentation, |
||
{-# INLINE toBuiltinsRuntime #-} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ import Data.Text.Encoding (decodeUtf8', encodeUtf8) | |
import Flat hiding (from, to) | ||
import Flat.Decoder (Get, dBEBits8) | ||
import Flat.Encoder as Flat (Encoding, NumBits, eBits) | ||
import NoThunks.Class (NoThunks) | ||
import Prettyprinter (viaShow) | ||
|
||
-- See Note [Pattern matching on built-in types]. | ||
|
@@ -1075,10 +1076,12 @@ instance uni ~ DefaultUni => ToBuiltinMeaning uni DefaultFun where | |
possibly different semantics. Note that DefaultFunSemanticsVariant1, | ||
DefaultFunSemanticsVariant1 etc. do not correspond directly to PlutusV1, | ||
PlutusV2 etc. in plutus-ledger-api: see Note [Builtin semantics variants]. -} | ||
data BuiltinSemanticsVariant DefaultFun = | ||
DefaultFunSemanticsVariant1 | ||
| DefaultFunSemanticsVariant2 | ||
deriving stock (Enum, Bounded, Show) | ||
data BuiltinSemanticsVariant DefaultFun | ||
= DefaultFunSemanticsVariant0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I vote that we change these to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine either way. I'm hopeful we'll be able to get rid of the semantics variants entirely and simply rely on the language and protocol versions directly (maybe in the "condensed" form). |
||
| DefaultFunSemanticsVariant1 | ||
| DefaultFunSemanticsVariant2 | ||
deriving stock (Eq, Enum, Bounded, Show, Generic) | ||
deriving anyclass (NFData, NoThunks) | ||
|
||
-- Integers | ||
toBuiltinMeaning | ||
|
@@ -1176,6 +1179,7 @@ instance uni ~ DefaultUni => ToBuiltinMeaning uni DefaultFun where | |
appendByteStringDenotation | ||
(runCostingFunTwoArguments . paramAppendByteString) | ||
|
||
-- See Note [Builtin semantics variants] | ||
toBuiltinMeaning semvar ConsByteString = | ||
-- The costing function is the same for all variants of this builtin, | ||
-- but since the denotation of the builtin accepts constants of | ||
|
@@ -1185,26 +1189,26 @@ instance uni ~ DefaultUni => ToBuiltinMeaning uni DefaultFun where | |
:: ExMemoryUsage a => BuiltinCostModel -> a -> BS.ByteString -> ExBudgetStream | ||
costingFun = runCostingFunTwoArguments . paramConsByteString | ||
{-# INLINE costingFun #-} | ||
-- See Note [Builtin semantics variants] | ||
in case semvar of | ||
DefaultFunSemanticsVariant1 -> | ||
consByteStringMeaning_V1 = | ||
let consByteStringDenotation :: Integer -> BS.ByteString -> BS.ByteString | ||
consByteStringDenotation n xs = BS.cons (fromIntegral n) xs | ||
{-# INLINE consByteStringDenotation #-} | ||
in makeBuiltinMeaning | ||
consByteStringDenotation | ||
costingFun | ||
-- For builtin semantics variants other (i.e. larger) than | ||
-- DefaultFunSemanticsVariant1, the first input must be in range | ||
-- [0..255]. See Note [How to add a built-in function: simple | ||
-- cases] | ||
DefaultFunSemanticsVariant2 -> | ||
-- For builtin semantics variants larger than 'DefaultFunSemanticsVariant1', the first | ||
-- input must be in range @[0..255]@. | ||
consByteStringMeaning_V2 = | ||
let consByteStringDenotation :: Word8 -> BS.ByteString -> BS.ByteString | ||
consByteStringDenotation = BS.cons | ||
{-# INLINE consByteStringDenotation #-} | ||
in makeBuiltinMeaning | ||
consByteStringDenotation | ||
costingFun | ||
in case semvar of | ||
DefaultFunSemanticsVariant0 -> consByteStringMeaning_V1 | ||
DefaultFunSemanticsVariant1 -> consByteStringMeaning_V1 | ||
DefaultFunSemanticsVariant2 -> consByteStringMeaning_V2 | ||
|
||
toBuiltinMeaning _semvar SliceByteString = | ||
let sliceByteStringDenotation :: Int -> Int -> BS.ByteString -> BS.ByteString | ||
|
@@ -1287,7 +1291,8 @@ instance uni ~ DefaultUni => ToBuiltinMeaning uni DefaultFun where | |
:: BS.ByteString -> BS.ByteString -> BS.ByteString -> BuiltinResult Bool | ||
verifyEd25519SignatureDenotation = | ||
case semvar of | ||
DefaultFunSemanticsVariant1 -> verifyEd25519Signature_V1 | ||
DefaultFunSemanticsVariant0 -> verifyEd25519Signature_V1 | ||
DefaultFunSemanticsVariant1 -> verifyEd25519Signature_V2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This mapping is potentially easy to get wrong. Is there any test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're just replacing one implementation with another for maintenance reasons, so doing this matching here is only for the purpose of being paranoid about not breaking anything in the past in case of some unknown unknown. There's nothing additional to test therefore beyond what we already test, which is how we use Note that So for both the builtins we only make sure that whatever their variant is, it behaves as expected, we cannot test that |
||
DefaultFunSemanticsVariant2 -> verifyEd25519Signature_V2 | ||
{-# INLINE verifyEd25519SignatureDenotation #-} | ||
in makeBuiltinMeaning | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ module PlutusCore.Evaluation.Machine.ExBudgetingDefaults | |
( defaultBuiltinsRuntimeForSemanticsVariant | ||
, defaultBuiltinsRuntime | ||
, defaultCekCostModel | ||
, toCekCostModel | ||
, defaultCekMachineCosts | ||
, defaultCekParameters | ||
, defaultCostModelParams | ||
|
@@ -85,6 +86,9 @@ defaultCekMachineCosts = | |
defaultCekCostModel :: CostModel CekMachineCosts BuiltinCostModel | ||
defaultCekCostModel = CostModel defaultCekMachineCosts defaultBuiltinCostModel | ||
|
||
toCekCostModel :: BuiltinSemanticsVariant DefaultFun -> CostModel CekMachineCosts BuiltinCostModel | ||
toCekCostModel _ = defaultCekCostModel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's where we get the additional extensibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it therefore have some comment explaining that? |
||
|
||
-- | The default cost model data. This is exposed to the ledger, so let's not | ||
-- confuse anybody by mentioning the CEK machine | ||
defaultCostModelParams :: Maybe CostModelParams | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved that
force
to the creation site, so that we can remove it from lots of call sites (which was risky to begin with, because we'd occasionally forget to add aforce
, thus distorting the benchmarking results).