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.

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

[Epic] Internals of builtins #4306

Open
effectfully opened this issue Dec 31, 2021 · 4 comments
Open

[Epic] Internals of builtins #4306

effectfully opened this issue Dec 31, 2021 · 4 comments

Comments

@effectfully
Copy link
Contributor

effectfully commented Dec 31, 2021

This issue is for dumping all the plans regarding builtins in a largely unstructured way.

@effectfully effectfully self-assigned this Dec 31, 2021
@effectfully
Copy link
Contributor Author

effectfully commented Dec 31, 2021

A lot of from the above has to be changed to become more efficient/expressive/etc. We'll need both major refactoring and small tweaking. This comment is about major refactorings.

@effectfully
Copy link
Contributor Author

effectfully commented Dec 31, 2021

Minor things, all fairly low priority:

case ww4_s1bxe `cast` <Co:5> of dt_XiSB
  { DefaultUniInteger ipv_s1apx -> (ValueOf dt_XiSB vx_ikbc) `cast` <Co:14>
  })

what is this matching on DefaultUniInteger for? UPD: no longer see it. It was probably used in that last cast.

  • there's a TODO in the CEK machine: "We pattern match on @arg@ twice: in 'readKnown' and in 'toExMemory'. Maybe we could fuse the two?". PR: [Builtins] Drop 'RuntimeScheme' #4778
  • make geq reducible statically in one way or another. PRs: [Builtins] Add an inlinable version of 'GEq' #4462, [Builtins] Unfold 'geq' for the default universe #4463, PLT-1432: [Builtins] Make 'geq' inlinable #5061
  • split KnownTypeIn into two type classes: one for lifting and one for unlifting. This is due to the fact that we have some types values of which can be lifted but not unlifted: EvaluationResult and Emitter. SomeConstantOf was an example of the opposite, but it's gone now. PR: [Builtins] Split 'KnownTypeIn' into two classes #4420
  • a built-in function is not supposed to fail with an exception, ever, so we need a test that for any set of built-in functions fun (properly constrained of course) checks that Arbitrary arguments don't trigger an exception. PRs: Test that builtin functions don't throw #4555, Make genConstant return SomeGen #4576
  • instead of relying on the denotation application being implicitly lazy at every step we could make all builtins return a (# #) and only be lazy there. PRs: [Builtins] Only be 'Lazy' for the result of a builtin application #4607, [Builtins] Drop 'RuntimeScheme' #4778
  • try let !cost :: (# #) -> ... in CostingFun/Core.hs so that the result of the function can be unboxed. UPD: I've tried it and nothing worked, it wouldn't unbox and it would inline instead
  • try to lazily lookup the builtin meanings during deserialization to avoid doing repeated lookups at runtime. Moot given [Builtins] [Evaluation] Drop lookups #4914
  • looking at Core I feel like EvaluationSuccess should be made strict. PR: [Evaluation] Make 'EvaluationResult' strict #4512
  • transformers are way too lazy for our use case, we need something stricter. PR: [Builtins] Optimize 'MakeKnownM' #4587
  • it would be nice to have some builtins-specific benchmarks to make changes in performance more apparent. UPD: lists and nofib do work quite OK for that
  • stuff that is now redundant should be dropped. PRs: [Builtins] Drop 'Proxy' from 'TypeSchemeAll' #4317, [Builtins] Fuse 'AsConstant' and 'FromConstant' into 'HasConstantIn' #4417
  • better module structure. PR: [Builtins] Restructure everything #4363
  • _BuiltinFailure is TH-derived and is not inlined. We should probably inline it just for tidier Core (it probably doesn't really affect evaluation time)
  • code, tests, docs polishing: UPD: we've got too many of those, so I removed all of them from here, they aren't important enough to be listed
  • Plutus Tx builtins are inconsistent in all kinds of ways. PR: [Test] [Builtins] Ensure 'Typeable', 'Lift' etc instances are present #5547
  • return False early if any of the costs from the two streams of costs mismatch in equalsData? May not be worth it, because in the happy case we're just going to end up doing more checks this way
  • consider interleaving costing calculations and the equality check in equalsData, so that if there's any mismatch we don't keep emitting costs pointlessly
  • unscrew matchList
  • Rename DefaultUni and DefaultFun to CardanoUni and CardanoFun and pull them out into their own sublibrary with all the specific code that they depend upon to make it easier to review and maintain Cardano-specific builtins-related code.
  • Add a Note about "good GHC Core" for builtins
  • Write a doc providing a high-level view on the built-in types machinery
  • Write a doc providing a high-level view of the machinery that calculates built-in function call costs during script execution
  • Improve docs about built-in type constructors not being allowed to get applied to type variables, Ziyang was confused about this
  • Add a Note about lazy costing in CostingFun/Core.hs
  • We need to have proper error message instead of just Maybe or MonadPlus in Universe.Core . Also probably worth monomorphizing tryUniApply and what it depends upon
  • When I implemented minCostStream I didn’t pay much attention to its performance. There may exist ways of optimizing it, for example should we make unconsCost return a (# CostingInteger, (# (# #) | CostStream #) #) or use UnliftedDatatypes or something? Same about HeadSpine
  • There was a long discussion about the safety and performance of flattenCostRose. Do we want this function to emit the next cost in O(1) worst case? Is it best to define it by concatenating forests in an accumulator or via CPS (all the code is at the link) performance-wise? We probably need some kind of benchmark to answer these questions.
  • Figure out if we want to store BuiltinRuntime lazily explicitly to make the NoThunks accurate (currently it's not). PR: [Builtins] Store 'BuiltinRuntime' lazily explicitly #5806
  • Try dropping extensible builtins completely to see how much they cost us
  • Discharging-related bookkeeping costs us some, we should try dropping discharging at least to see how much it costs us
  • Consider adding Proxy to the universe, so that it's possible to provide builtins like nil (currently impossible) or pair (currently subverts the type checker). PR: [Builtins] Add 'Proxy' to the default universe #4337
  • Require explicit Foralls in type signature of builtins so that we don't spend a lot of time elaborating them and also make things more clear
  • we need to consider dropping nullary builtins completely, so that we don't need to check if it's RuntimeSchemeResult yet every time a builtin is forced/applied. Plus it's good to reflect restrictions in the types anyway, otherwise somebody might add a nullary builtin which the CEK machine (and god knows what else) doesn't support. PR: [Builtins] Disallow builtins with type arguments appearing last  #4616 (not beneficial for performance, but we should still do it)
  • play with compilation flags, in particular try setting -O2 in various places (setting it globally very unexpectedly makes things worse). PR: [Builtins] Stop doing the worker-wrapper transformation for the denotations #4532

@effectfully
Copy link
Contributor Author

effectfully commented Jan 1, 2022

The comment about major refactorings does not include two big things:

  1. documenting builtins
  2. collaborating with James to incorporate extensible builtins into the metatheory

It's probably too early to think about the latter right now, given how much the whole builtins machinery is going to change (if everything works out), so I'll focus on the former.

The inline docs in the source code are fairly good, they might use some polishing, but overall they seem pretty comprehensive. What is missing however is a high-level overview of the whole builtins machinery that one can read before diving into the specifics of each of its parts. We do have one such file, Builtins.md, but it was written long ago and it doesn't talk about polymorphic built-in types, builtin meaning inference etc.

Another thing we need is a doc (and presentation) on how to add a new built-in type or function. After all the recent refactorings it's completely trivial to add a new built-in type (regardless of whether it's monomorphic or polymorphic): it's basically just warnings/errors-driven copy-paste, but adding a new built-in function is trickier, because the polymorphic function over a polymorphic built-in type case is so much different to the other cases (improving upon the status quo is one of the major tasks).

PRs:

@effectfully
Copy link
Contributor Author

effectfully commented Mar 3, 2022

Performance improvements in builtins-related PRs (each number is the mean performance change for validation benchmarks):

#4173: -8.23%
#4230: -5.09%
#4264: -2.42%
#4307: -12.01%
#4317: -2.01%
#4379: -1.78%
#4397: -2.75%
#4398: -2.02%
#4421: -4.87%
#4481: -6.7%
#4496: -2.59%
#4505: -3.2%
#4516: -5.88%
#4587: -7.91%
#4778: -4.56%
#4914: -10.35%
#5061: -1.16%
#5239: -3.12%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant