-
Notifications
You must be signed in to change notification settings - Fork 481
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
[Builtins] Make 'toBuiltinRuntime' a class method #4419
Changes from all commits
3cc2416
5f2ef8f
89eab5e
307cbbd
3cf50ef
1e38f47
eb3b68c
799d9fc
669a4e3
695c053
2418cc7
f47869d
d537941
c7fea84
606cfde
2bb4736
9ed2131
aea0952
5d8edc6
1222e80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,19 @@ | ||
{-# LANGUAGE DataKinds #-} | ||
{-# LANGUAGE GADTs #-} | ||
{-# LANGUAGE KindSignatures #-} | ||
{-# LANGUAGE TypeOperators #-} | ||
{-# LANGUAGE DataKinds #-} | ||
{-# LANGUAGE DefaultSignatures #-} | ||
{-# LANGUAGE GADTs #-} | ||
{-# LANGUAGE KindSignatures #-} | ||
{-# LANGUAGE MultiParamTypeClasses #-} | ||
{-# LANGUAGE RankNTypes #-} | ||
{-# LANGUAGE TypeOperators #-} | ||
|
||
{-# LANGUAGE StrictData #-} | ||
{-# LANGUAGE StrictData #-} | ||
|
||
module PlutusCore.Builtin.Runtime where | ||
|
||
import PlutusPrelude | ||
|
||
import PlutusCore.Builtin.HasConstant | ||
import PlutusCore.Builtin.KnownType | ||
import PlutusCore.Builtin.Meaning | ||
import PlutusCore.Builtin.TypeScheme | ||
import PlutusCore.Core | ||
|
@@ -19,7 +23,7 @@ import Control.Lens (ix, (^?)) | |
import Control.Monad.Except | ||
import Data.Array | ||
import Data.Kind qualified as GHC (Type) | ||
import PlutusCore.Builtin.KnownType | ||
import GHC.Exts (inline) | ||
|
||
-- | Same as 'TypeScheme' except this one doesn't contain any evaluation-irrelevant types stuff. | ||
data RuntimeScheme val (args :: [GHC.Type]) res where | ||
|
@@ -66,7 +70,7 @@ newtype BuiltinsRuntime fun val = BuiltinsRuntime | |
|
||
-- | Convert a 'TypeScheme' to a 'RuntimeScheme'. | ||
typeSchemeToRuntimeScheme :: TypeScheme val args res -> RuntimeScheme val args res | ||
typeSchemeToRuntimeScheme TypeSchemeResult = RuntimeSchemeResult | ||
typeSchemeToRuntimeScheme TypeSchemeResult = RuntimeSchemeResult | ||
typeSchemeToRuntimeScheme (TypeSchemeArrow schB) = | ||
RuntimeSchemeArrow $ typeSchemeToRuntimeScheme schB | ||
typeSchemeToRuntimeScheme (TypeSchemeAll _ schK) = | ||
|
@@ -77,13 +81,28 @@ toBuiltinRuntime :: cost -> BuiltinMeaning val cost -> BuiltinRuntime val | |
toBuiltinRuntime cost (BuiltinMeaning sch f exF) = | ||
BuiltinRuntime (typeSchemeToRuntimeScheme sch) f (exF cost) | ||
|
||
-- | Calculate runtime info for all built-in functions given denotations of builtins | ||
-- and a cost model. | ||
toBuiltinsRuntime | ||
:: (cost ~ CostingPart uni fun, HasConstantIn uni val, ToBuiltinMeaning uni fun) | ||
=> cost -> BuiltinsRuntime fun val | ||
toBuiltinsRuntime cost = | ||
BuiltinsRuntime . tabulateArray $ toBuiltinRuntime cost . toBuiltinMeaning | ||
-- | This is a function turned into a class for the sake of specialization and inspection. | ||
-- Every instance of this class MUST use the default implementation of 'toBuiltinsRuntime', | ||
-- i.e. not define 'toBuiltinsRuntime' at all. Having this class allows us to inline away lots of | ||
-- abstractions used by the builtins machinery. | ||
-- | ||
-- For performance-critical code both @fun@ and @val@ need to be specified as concrete data types | ||
-- for inlining to happen. In other places it's fine to only specify one of them. | ||
class ToBuiltinsRuntime fun val where | ||
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. Let me try and write down what I think this is doing. We want to specialize That suggests the following question: does this basically amount to a 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 also genuinely quite unclear why inlining 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. Probably also need to inline 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.
Yes, unless the call site has Note that we're running the CEK machine from different places, so we'd need to track all of them. How would we even do that? Using a class just seems more reliable than hoping a 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. Well, except this is only for the call to
This is why we have benchmarking, no? We've done all kinds of things that we might accidentally break. 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 want to have a little try and see if I can make a version with SPECIALIZE work also, just for comparison. 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 kind of have that in the cost model benchmarks for the builtins, if you're willing to wait 12 hours or so. I'm not suggesting that seriously because (a) it takes too long, and (b) it tells us far more than we need to know. Maybe we could do something like extracting some small subset for checking the performance of the machinery though? I'm already trying to make some attempt to do that with some extra I suppose that an advantage of having some separate benchmarks would be that it'd make it easier to spot regressions in the efficiency of the builtin machinery. That makes me a bit nervous because a lot of it depends on delicate implementation choices and I can imagine things going backwards if something changes in GHC. 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.
What do you consider small? Here's a breakdown of how much speedup we got from builtins-related PRs affecting performance that are already merged. A speedup of 31.22% seems to be quite a decent one. Add to that the monomorphic
I'm almost sure it will. Which is one of the reasons why I want the 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 can't of course just add up those individual changes, I'm cutting some corners here. 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.
My rusty math skills suggest that with these two PRs we'll get a total of 34% speedup, which I think is not bad at all. |
||
-- Somehow getting rid of the @uni ~ UniOf val@ constraint by substituting @UniOf val@ for @uni@ | ||
-- completely breaks inlining of 'toBuiltinMeaning'. | ||
-- | Calculate runtime info for all built-in functions given a cost model. | ||
toBuiltinsRuntime :: uni ~ UniOf val => CostingPart uni fun -> BuiltinsRuntime fun val | ||
default toBuiltinsRuntime | ||
:: (HasConstantIn uni val, ToBuiltinMeaning uni fun) | ||
=> CostingPart uni fun -> BuiltinsRuntime fun val | ||
-- Do we want to mark this as @INLINE@? Or @NOINLINE@? The former has the advantage of allowing | ||
-- us to optimize some of the @cost@-related operations away at the cost of potentially blowing | ||
-- up compilation times or even the size of the executable (probably not though). The latter is | ||
-- the opposite. Currently we let GHC decide: it won't inline 'toBuiltinsRuntime' if it's huge. | ||
toBuiltinsRuntime cost = | ||
-- 'toBuiltinMeaning' can be a huge function, hence an explicit 'inline'. | ||
BuiltinsRuntime . tabulateArray $ toBuiltinRuntime cost . inline toBuiltinMeaning | ||
|
||
-- | Look up the runtime info of a built-in function during evaluation. | ||
lookupBuiltin | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ import PlutusPrelude | |
|
||
import PlutusCore.Builtin | ||
import PlutusCore.Core | ||
import PlutusCore.Default | ||
import PlutusCore.Evaluation.Machine.Exception | ||
import PlutusCore.Evaluation.Result | ||
import PlutusCore.Name | ||
|
@@ -43,7 +44,6 @@ import Data.DList (DList) | |
import Data.DList qualified as DList | ||
import Data.STRef | ||
import Data.Text (Text) | ||
import Universe | ||
|
||
infix 4 |>, <| | ||
|
||
|
@@ -59,6 +59,8 @@ data CkValue uni fun = | |
| VBuiltin (Term TyName Name uni fun ()) (BuiltinRuntime (CkValue uni fun)) | ||
deriving (Show) | ||
|
||
instance ToBuiltinsRuntime DefaultFun (CkValue DefaultUni DefaultFun) | ||
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 don't love the fact that now the functions exported from this module claim to work on any uni and fun, but you would need to add these instances to make it work in practice. 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. Err, it should be a general instance in the CK machine, because we don't care about performance there. I originally did experiments in this module and then forgot to fix it. I'll do that. The CEK machine is different though, there we do need the specialization. However note that 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. Err, not so fast, a general instance conflicts with the one for |
||
|
||
-- | Take pieces of a possibly partial builtin application and either create a 'CkValue' using | ||
-- 'makeKnown' or a partial builtin application depending on whether the built-in function is | ||
-- fully saturated or not. | ||
|
@@ -118,8 +120,9 @@ emitCkM str = do | |
type instance UniOf (CkValue uni fun) = uni | ||
|
||
instance HasConstant (CkValue uni fun) where | ||
asConstant _ (VCon val) = pure val | ||
asConstant mayCause _ = throwNotAConstant mayCause | ||
asConstant mayCause = \case | ||
VCon val -> pure val | ||
_ -> throwNotAConstant mayCause | ||
|
||
fromConstant = VCon | ||
|
||
|
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.
do we need this?
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 see, it's because we dropped the
Proxy
args. Does that matter? Perhaps this change should be done separately so we can see if it matters.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.
Probably not. I did it to see if it could change Core output, but couldn't tell if there was a difference in the end. I left it in the code, because
AllowAmbiguousTypes
looks better to my taste than those stupid proxies.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.
Ah, I actually did see it affect things in one of the previous PRs. Even with
INLINE
pragmas everywhere, stuff still wasn't inlining properly because of thatProxy
. I don't think we care about that here though, since the Core output is pretty chaotic anyway: some ofTypeScheme
are inlined and some are not and we're fine in both the cases, because we really only care aboutTypeScheme
s being outside of the loop that computesBuiltinsRuntime
, so that allBuiltinRuntime
s are shared and never recomputed.