-
Notifications
You must be signed in to change notification settings - Fork 321
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
CPS-0013? | Better builtin data structures in Plutus #638
Conversation
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.
Very nice to see such a proposal sourced in cooperation with the community 🤓
Thanks for taking the time to compile this @michaelpj! Not sure if this is appropriate for a CPS, but in the spirit of inspiring some CIPs to solve this CPS, let me document a few of the rough ideas we kicked around on that twitter thread: Length-prefixed arrays with constant time lookupNew builtin type
New builtin function New builtin function
(or perhaps you provide the initial value?) New builtin function
New builtin function
Why is this useful?There are several good candidates in the script context that are "index heavy" (most notably the inputs and outputs, but also withdrawals) that would benefit from being made into arrays. Even if not, a single linear pass to construct the array, and then constant time lookups after that, this would save quadratic work in many contracts. GADTs from languages that compile to UPLC could compile structs to "Constructors with one field, which is an array of the logical fields of the struct", and then any field access is a Associative ArraysI have less fidelity here, so either a new builtin or a new Plutus Data type that got accelerated with a hashmap implementation; ideally, one that mapped directly onto CBOR maps for efficient encoding. Would likely need builtins for looking up by key, and inserting (producing a new map with the entry added). Why is this useful?Structs could compile down to maps, for log (with very small constants) time lookup of fields. Preserve the order of inputs from the transactionThis one is kind of an orthogonal / out of the box solution, but is highlighted by the struggles in this CPS; in particular, the best optimization is just to not do the work at all anymore, and a huge source of indexing slowdown is having to re-sort the transaction inputs in the first place. |
#### Operations on `Value` | ||
|
||
The `Value` type is a nested map: it is a map from bytestrings (representing policy IDs) to maps from bytestrings (representing token names) to integers (representing quantities). | ||
Since map operations are currently linear, this means that even simple operations like checking whether one value is less than another can have quadratic cost. |
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.
Since map operations are currently linear
To be precise, operations that should be logarithmic are currently linear (e.g., insertion, deletion, member), whereas operations should be linear (or nlogn) are currently quadratic (e.g., union)
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.
If we had a list with O(1) index lookup then valueOf
would be constant time in the majority of DApps in practice. This is because every robust DApp already explicitly prevents non-protocol related tokens from being stored in protocol script UTxOs, and in the vast majority of cases the validator will have access to the currency symbol of all the protocol tokens that it must check for. In all such cases you can just predetermine the indices for the relevant protocol tokens and then use elemAt index valuePairList
instead of valueOf
@rphair I think this is ready for review |
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.
@michaelpj all looks generally in order to me. Have added it to CIP agenda # 81 for Review this time, since at meeting # 79 it only got Triage. I expect we'll be able to assign it a number at the meeting & I'm ready to green-check this soon after that's done... unless any major outcry or new unresolved issues. cc @Quantumplation
To avoid confusion for reviewers, can you rename the containing directory to something beginning with CPS-
and link to the latest file in your branch in your original comment up top?
Also we agreed ## References
is OK but please see discussion in #638 (comment) about what to do with the link currently there.
Note this was supposed to be combined with #638 (comment) but my Internet connection went boogey.
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.
This looks good and in order for a CPS to me. Thanks @rphair @michaelpj @Quantumplation for getting this together. Definitely worthy of a number at the next meeting at the very least!
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.
This appears to be a well written problem statement, with good discussions, happy to see a number applied then merged.
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"m also very happy with it but will wait until it gets a number (very likely in couple days' time after meeting) in the usual cycle before I ✅ it for merge.
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.
@michaelpj this has passed editorial review but zooming ahead too quickly to merge now. Please update the number here & in your branch (and correcting the document link in your OP please)... it's on Last Check
now for next meeting agenda, if not merged by popular request & consensus sooner 🚀
I think #638 (comment) still needs to be resolved although I believe that's a document quality & not a "standards" issue & therefore up to @michaelpj @Quantumplation about whether it needs to be fixed; so I won't let the merge be contingent upon it.
- TL;DR (already documented above) I think our body of CIPs shouldn't have
References
which are links of unknown significance to social discussions included without context: our CIPs are generally written at a higher calibre than that.
Here are benchmarks that are relevant to the discussion of the benefits of indexed data-structures. They illustrate how incredibly expensive recursive lookups are currently. |
Based on some discussions with Pi and others.
Some tasks today are difficult because we don't have the data structures to do them efficiently. This CPS outlines a couple of examples and gestures towards some possible solutions (which will require a proper CIP).
Rendered Version