Strict plutus-cbor #213
Closed
Strict plutus-cbor #213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No performance improvement, but was a potential space leak?
Slight improvement in cpu time
No noticable performance improvement, but same potential space leak as in encodeMap
This should have subtractions be performed right away. Small improvement on benchmark
This did not achieve a noticable improvement, but made all functions consistent in their strictness behavior with internal functions
|
I just realized that this is breaking the normal plutus compilation of our validators using these functions 😓 |
KtorZ
reviewed
Feb 9, 2022
| @@ -86,7 +87,7 @@ import PlutusTx.Builtins (subtractInteger) | |||
| newtype Encoding = Encoding (BuiltinByteString -> BuiltinByteString) | |||
|
|
|||
| instance Semigroup Encoding where | |||
| (Encoding a) <> (Encoding b) = Encoding (a . b) | |||
| (Encoding f) <> (Encoding g) = Encoding (\(!x) -> f $ g x) | |||
There was a problem hiding this comment.
We probably don't want that one... Being too strict is also not good as it prevents some compiler optimizations. Here, I'd heavily favor the composition over strictness.. I don't think it buys us much.
| @@ -115,7 +116,7 @@ encodeBool = \case | |||
| -- | |||
| -- Note (2): This can only encode numbers up to @2^64 - 1@ and down to @-2^63@ | |||
| encodeInteger :: Integer -> Encoding | |||
| encodeInteger n | |||
| encodeInteger !n | |||
| @@ -124,14 +125,15 @@ encodeInteger n | |||
|
|
|||
| -- | Encode a 'BuiltinByteString' as a CBOR type-02 major type. | |||
| encodeByteString :: BuiltinByteString -> Encoding | |||
| encodeByteString bytes = | |||
| encodeByteString !bytes = | |||
| @@ -161,7 +163,7 @@ encodeBreak = Encoding (consByteString 0xFF) | |||
| -- <> encodeInteger 42 | |||
| -- @ | |||
| encodeListLen :: Integer -> Encoding | |||
| encodeListLen = Encoding . encodeUnsigned 4 | |||
| encodeListLen !n = Encoding (encodeUnsigned 4 n) | |||
There was a problem hiding this comment.
I'd also favor Lazyness and composition here.
| @@ -188,7 +190,7 @@ encodeList :: (a -> Encoding) -> [a] -> Encoding | |||
| encodeList encodeElem = | |||
| step 0 mempty | |||
| where | |||
| step n bs = \case | |||
| step !n !bs = \case | |||
There was a problem hiding this comment.
This one is definitely needed!
|
I guess we can close that one? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introducing more strictness to plutus-cbor
Could shave off about 30% on computation time on the expensive 100 asset case, but still not linear in input data! That means, that benchmarking in Haskell is still vastly different than evaluating & estimating execution costs in plutus.
Before:
After:
