[Builtins] Simplify 'Emitter' #4230

effectfully · 2021-11-22T05:03:53Z

I was annoyed in #4229 that 'makeKnown' and 'readKnown' require different monads,
so I started thinking about ways to unify them and this PR implements such a way.
The idea is that for all intents and purposes 'makeKnown' always receives an
emitting function, for example we have

flip unWithEmitterT ?cekEmitter $ makeKnown (Just term) x

so we can just make it an explicit argument transforming the line above to

makeKnown ?cekEmitter (Just term) x

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Relevant tickets are mentioned in commit messages
- Formatting, materialized Nix files, PNG optimization, etc. are updated
PR
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

I was annoyed in IntersectMBO#4229 that 'makeKnown' and 'readKnown' require different monads, so I started thinking about ways to unify them and this PR implements such a way. The idea is that for all intents and purposes 'makeKnown' always receives an emitting function, for example we have flip unWithEmitterT ?cekEmitter $ makeKnown (Just term) x so we can just make it an explicit argument transforming the line above to makeKnown ?cekEmitter (Just term) x

effectfully · 2021-11-22T05:06:50Z

plutus-core/plutus-core/src/PlutusCore/Constant/Typed.hs

-        :: ( MonadEmitter m, MonadError (ErrorWithCause err cause) m, AsEvaluationFailure err
+        :: ( MonadError (ErrorWithCause err cause) m, AsEvaluationFailure err
           )
-        => Maybe cause -> a -> m term
+        => (Text -> m ()) -> Maybe cause -> a -> m term


Kinda makes me wonder if instead of requiring m to implement MonadError and passing around that dictionary with catchError we could just pass another function explicitly.

"passing a dictionary with catchError" vs "passing catchError" doesn't seem that different to me, especially since I expect that accessing it isn't a hot path.

"passing catchError"

I mean that we don't need to pass catchError to that function at all, we don't do any error-catching in builtins from what I can remember (we do that outside of the builtins machinery in the CEK machine).

Or we could use MonadThrow.

michaelpj

Probably worth benchmarking it so we can claim credit if it's faster.

michaelpj · 2021-11-22T11:57:59Z

plutus-core/plutus-core/src/PlutusCore/Constant/Typed.hs

-        :: ( MonadEmitter m, MonadError (ErrorWithCause err cause) m, AsEvaluationFailure err
+        :: ( MonadError (ErrorWithCause err cause) m, AsEvaluationFailure err
           )
-        => Maybe cause -> a -> m term
+        => (Text -> m ()) -> Maybe cause -> a -> m term


"passing a dictionary with catchError" vs "passing catchError" doesn't seem that different to me, especially since I expect that accessing it isn't a hot path.

effectfully · 2021-11-22T15:27:29Z

Probably worth benchmarking it so we can claim credit if it's faster.

It's worth, but even if we get a speedup, that would be due to removing a constraint from makeKnown, which is something #4229 is ought to fix once and for all (and for readKnown as well).

michaelpj · 2021-11-22T15:32:17Z

Fair enough. We can always run the benchmarking job on it later if we're curious.

effectfully · 2022-04-02T22:00:11Z

Ok, I couldn't stand the fact that this was the only one that didn't get benchmarked, so I've run the benchmarks manually. For some inexplicable reason, bench-compare-markdown only worked for big microseconds and not small milliseconds (giving 0.0% change when it clearly wasn't 0.0%), so I just removed all .s from milliseconds and removed the results where we went from microseconds to milliseconds. I also removed two results where variance wasn't low for some reason. I also wasn't patient enough to wait for all benchmarks to finish, so there's only a subset of them.

So, not very scientific, but here are the results:

| Script                                   |  abc       |  def       |   Change   |
| :------                                  |  :------:  |  :------:  |  :------:  |
| auction_1-1                              |  399.6 μs  |  381.8 μs  |    -4.5%   |
| auction_1-2                              |  1185 ms  |  1181 ms  |    -0.3%   |
| auction_1-3                              |  1189 ms  |  1150 ms  |    -3.3%   |
| auction_1-4                              |  545.7 μs  |  544.2 μs  |    -0.2%   |
| auction_2-1                              |  405.3 μs  |  372.2 μs  |    -8.1%   |
| auction_2-2                              |  1182 ms  |  1087 ms  |    -8.0%   |
| auction_2-3                              |  1475 ms  |  1415 ms  |    -4.1%   |
| auction_2-4                              |  1182 ms  |  1082 ms  |    -8.5%   |
| auction_2-5                              |  544.3 μs  |  533.1 μs  |    -2.0%   |
| crowdfunding-success-1                   |  484.4 μs  |  479.2 μs  |    -1.0%   |
| crowdfunding-success-2                   |  485.0 μs  |  452.7 μs  |    -6.8%   |
| crowdfunding-success-3                   |  486.4 μs  |  453.1 μs  |    -6.8%   |
| currency-1                               |  471.3 μs  |  443.4 μs  |    -5.9%   |
| escrow-redeem_1-1                        |  755.1 μs  |  728.2 μs  |    -3.6%   |
| escrow-redeem_1-2                        |  755.2 μs  |  705.4 μs  |    -6.6%   |
| escrow-redeem_2-1                        |  870.2 μs  |  795.0 μs  |    -8.6%   |
| escrow-redeem_2-2                        |  880.9 μs  |  797.5 μs  |    -9.4%   |
| escrow-redeem_2-3                        |  859.1 μs  |  791.6 μs  |    -7.9%   |
| escrow-refund-1                          |  356.3 μs  |  327.2 μs  |    -8.1%   |
| future-increase-margin-1                 |  476.2 μs  |  440.1 μs  |    -7.6%   |
| future-increase-margin-3                 |  1087 ms  |  1001 ms  |    -7.9%   |
| future-increase-margin-4                 |  990.9 μs  |  901.6 μs  |    -9.0%   |
| future-increase-margin-5                 |  1438 ms  |  1321 ms  |    -8.1%   |
| future-pay-out-1                         |  467.6 μs  |  447.3 μs  |    -4.3%   |
| future-pay-out-2                         |  1074 ms  |  1057 ms  |    -1.6%   |
| future-pay-out-4                         |  1428 ms  |  1309 ms  |    -8.3%   |
| future-settle-early-1                    |  474.1 μs  |  443.6 μs  |    -6.5%   |
| future-settle-early-3                    |  1081 ms  |  1070 ms  |    -1.0%   |
| game-sm-success_1-1                      |  826.4 μs  |  763.9 μs  |    -7.6%   |
| game-sm-success_1-2                      |  463.0 μs  |  434.0 μs  |    -6.3%   |
| game-sm-success_1-3                      |  1217 ms  |  1201 ms  |    -1.3%   |
| game-sm-success_1-4                      |  537.0 μs  |  497.6 μs  |    -7.4%   |
| game-sm-success_2-1                      |  830.4 μs  |  758.9 μs  |    -8.7%   |
| game-sm-success_2-2                      |  459.8 μs  |  425.3 μs  |    -7.4%   |
| game-sm-success_2-3                      |  1217 ms  |  1161 ms  |    -4.6%   |
| game-sm-success_2-4                      |  537.0 μs  |  516.6 μs  |    -3.9%   |
| game-sm-success_2-5                      |  1220 ms  |  1174 ms  |    -3.8%   |
| game-sm-success_2-6                      |  538.9 μs  |  525.4 μs  |    -2.4%   |
| multisig-sm-1                            |  832.2 μs  |  811.1 μs  |    -2.5%   |
| multisig-sm-2                            |  819.9 μs  |  801.7 μs  |    -2.2%   |
| multisig-sm-3                            |  829.4 μs  |  805.9 μs  |    -2.9%   |
| multisig-sm-5                            |  1067 ms  |  1049 ms  |    -1.7%   |
| multisig-sm-6                            |  831.9 μs  |  815.2 μs  |    -1.9%   |
| multisig-sm-7                            |  862.5 μs  |  802.7 μs  |    -7.0%   |
| multisig-sm-8                            |  835.2 μs  |  808.9 μs  |    -3.2%   |
| multisig-sm-9                            |  838.7 μs  |  813.7 μs  |    -3.0%   |
| multisig-sm-10                           |  1084 ms  |  1047 ms  |    -3.4%   |

The average is -5.09% and this is what I'm going to claim. If somebody feels like rechecking, more power to you.

effectfully added Refactoring Builtins labels Nov 22, 2021

effectfully requested review from michaelpj, kwxm and thealmarty November 22, 2021 05:03

effectfully commented Nov 22, 2021

View reviewed changes

michaelpj approved these changes Nov 22, 2021

View reviewed changes

effectfully mentioned this pull request Nov 22, 2021

[Builtins] [Evaluation] Introduce 'TypeSchemeRuntime' #4229

Closed

michaelpj merged commit e995df9 into IntersectMBO:master Nov 22, 2021

effectfully mentioned this pull request Nov 27, 2021

Revert "[Builtins] Simplify 'Emitter'" #4247

Closed

effectfully deleted the effectfully/builtins/simplify-emitter branch March 3, 2022 14:30

effectfully mentioned this pull request Mar 3, 2022

[Epic] Internals of builtins #4306

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Builtins] Simplify 'Emitter' #4230

[Builtins] Simplify 'Emitter' #4230

effectfully commented Nov 22, 2021 •

edited

Loading

effectfully Nov 22, 2021

michaelpj Nov 22, 2021

effectfully Nov 22, 2021

michaelpj left a comment

michaelpj Nov 22, 2021

effectfully commented Nov 22, 2021

michaelpj commented Nov 22, 2021

effectfully commented Apr 2, 2022 •

edited

Loading

[Builtins] Simplify 'Emitter' #4230

[Builtins] Simplify 'Emitter' #4230

Conversation

effectfully commented Nov 22, 2021 • edited Loading

effectfully Nov 22, 2021

Choose a reason for hiding this comment

michaelpj Nov 22, 2021

Choose a reason for hiding this comment

effectfully Nov 22, 2021

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj Nov 22, 2021

Choose a reason for hiding this comment

effectfully commented Nov 22, 2021

michaelpj commented Nov 22, 2021

effectfully commented Apr 2, 2022 • edited Loading

effectfully commented Nov 22, 2021 •

edited

Loading

effectfully commented Apr 2, 2022 •

edited

Loading