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

[Builtins] Simplify 'Emitter' #4230

Conversation

effectfully
Copy link
Contributor

@effectfully effectfully commented Nov 22, 2021

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
Comment on lines -536 to +539
:: ( 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -536 to +539
:: ( 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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
Copy link
Contributor Author

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
Copy link
Contributor

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

@michaelpj michaelpj merged commit e995df9 into IntersectMBO:master Nov 22, 2021
@effectfully effectfully deleted the effectfully/builtins/simplify-emitter branch March 3, 2022 14:30
@effectfully
Copy link
Contributor Author

effectfully commented Apr 2, 2022

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.

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

Successfully merging this pull request may close these issues.

2 participants