More benchmarks for natives #124
Conversation
| @@ -97,7 +97,8 @@ lookupFqName fqn = | |||
| getDefCap :: (MonadEval b i m) => i -> FullyQualifiedName -> m (EvalDefCap b i) | |||
| getDefCap info fqn = lookupFqName fqn >>= \case | |||
| Just (DCap d) -> pure d | |||
| _ -> failInvariant info "Expected DefCap" | |||
| Just _ -> failInvariant info "Expected DefCap" | |||
| _ -> failInvariant info "Expected DefCap; got nothing" | |||
There was a problem hiding this comment.
Indeed. I agree, we need a better ADT for invariant failures.
| @@ -1984,7 +2065,7 @@ coreBuiltinRuntime = \case | |||
| CoreDefineNamespace -> coreDefineNamespace | |||
| CoreDescribeNamespace -> coreDescribeNamespace | |||
| CoreZkPairingCheck -> zkPairingCheck | |||
| CoreZKScalarMult -> zkScalaMult | |||
| CoreZKScalarMult -> zkScalarMult | |||
There was a problem hiding this comment.
I like that we are now no longer multiplying Scalas :)
| @@ -1656,8 +1735,10 @@ coreDescribeNamespace :: (CEKEval step b i m, MonadEval b i m) => NativeFunction | |||
| coreDescribeNamespace info b cont handler _env = \case | |||
| [VString n] -> do | |||
| pdb <- viewEvalEnv eePactDb | |||
| chargeGasArgs info $ GRead $ sizeOf SizeOfV0 n | |||
There was a problem hiding this comment.
HMM. This does not seem right. Why are we charging gas on the size of the name of the namespace?
There was a problem hiding this comment.
Should charge for an object with the name + two guards (user and admin one)
There was a problem hiding this comment.
The namespace itself is only charged if it's found by name. Hence to prevent undercharging in case the namespace isn't found, it's easier and less error-prone to just charge for the mere fact of looking up the name in the DB. After it's found, it's also charged for the namespace object with its guards.
| -- THIS CANNOT MAKE IT TO PROD | ||
| renderPactValue :: PactValue -> T.Text | ||
| renderPactValue = T.pack . show . Pretty.pretty | ||
| -- Todo: this is kinda hacky |
There was a problem hiding this comment.
I agree with this change.
That said: you already gassed Show. I think we could reuse your Show gas for this. After all, format is essentially just Show with string splicing.
| [VString s] -> | ||
| checkLen info s *> doBase info cont handler 10 s | ||
| [VString s] -> do | ||
| chargeGasArgs info $ GStrOp $ StrOpLength $ T.length s |
There was a problem hiding this comment.
I'm curious what we're charging for here? Shouldn't we be charging for parsing?
| Right bs -> returnCEKValue cont handler $ VInteger (bsToInteger bs) | ||
| | base >= 2 && base <= 16 -> checkLen info s *> doBase info cont handler base s | ||
| | base == 64 -> do | ||
| chargeGasArgs info $ GStrOp $ StrOpLength $ T.length s |
There was a problem hiding this comment.
Ditto, I believe we should be charging for parsing.
gasmodel/Pact/Core/GasModel/Utils.hs
Outdated
| . (esLoaded.loAllLoaded %~ M.insert fqn dfn) | ||
| where | ||
| fqn = mkGasModelFqn name | ||
| dkind = case dfn of |
There was a problem hiding this comment.
call defKind from Pact.Core.IR.Term
0xd34df00d
force-pushed
the
gr/gas-bench-natives-b1
branch
from
275ae10
to
719bad3
Compare
PR checklist:
Additionally, please justify why you should or should not do the following: