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.

Sign up for GitHub

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

Jump to bottom

PLT-5287: Define a runCekM-like function giveCekReqs for the debugger #5229

Merged
merged 1 commit into from Mar 31, 2023
Merged

PLT-5287: Define a runCekM-like function giveCekReqs for the debugger #5229

merged 1 commit into from Mar 31, 2023

Conversation

bezirg
Copy link
Contributor

@bezirg bezirg commented Mar 23, 2023

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@bezirg bezirg added the No Changelog Required Add this to skip the Changelog Check label Mar 23, 2023
@bezirg bezirg requested a review from michaelpj March 23, 2023 14:59
@bezirg bezirg marked this pull request as ready for review March 23, 2023 14:59
@bezirg bezirg self-assigned this Mar 23, 2023
@bezirg bezirg force-pushed the bezirg/dbg-run branch 2 times, most recently from bd4be1d to f559514 Compare March 23, 2023 15:04
michaelpj
michaelpj reviewed Mar 23, 2023
View reviewed changes
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.

I'm still a bit suspicious that GivenCekReqs escapes. I would have thought we could e.g. pass the CEK arguments to handleStep etc. I'm not really sure. It just feels like a mess atm.

plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug/Internal.hs Outdated Show resolved Hide resolved
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug/Internal.hs Outdated
?cekStepCounter = ctr
in act exBudgetInfo
-- note that we do not call the final budget&emit getters like in `runCekM`,
-- since we do not need it for our usecase.
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely act can do this itself if it needs to?

plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug/Internal.hs Outdated
=> MachineParameters CekMachineCosts fun (CekValue uni fun ann)
-> ExBudgetMode cost uni fun
-> EmitterMode uni fun
-> (GivenCekReqs uni fun ann s => ExBudgetInfo cost uni fun s -> m a)
Copy link
Contributor

Choose a reason for hiding this comment

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

The passing of the ExBudgetInfo is kind of mixed up in it also. And nothing uses it? So it doesn't just set up the requirements? I'm confused.

zliu41
zliu41 approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

Looks good. The fewer implicit parameters the better!

plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug/Internal.hs Outdated Show resolved Hide resolved
@bezirg
Copy link
Contributor Author

bezirg commented Mar 28, 2023

After discussing with Michael, I will try to improve this PR a bit to make it less leaky

@bezirg bezirg requested a review from michaelpj March 29, 2023 15:58
@bezirg
Copy link
Contributor Author

bezirg commented Mar 29, 2023

/benchmark plutus-benchmark:validation

@bezirg
Copy link
Contributor Author

bezirg commented Mar 29, 2023

/benchmark plutus-benchmark:validation

@github-actions
Copy link
Contributor

Click here to check the status of your benchmark.

@github-actions
Copy link
Contributor

Comparing benchmark results of ' plutus-benchmark:validation' on 'f49230f650' (base) and '9e0788f643' (PR)

Results table
Script f49230f 9e0788f Change
auction_1-1 170.1 μs 169.1 μs -0.6%
auction_1-2 691.0 μs 692.6 μs +0.2%
auction_1-3 691.5 μs 681.4 μs -1.5%
auction_1-4 224.8 μs 220.2 μs -2.0%
auction_2-1 173.2 μs 172.1 μs -0.6%
auction_2-2 695.4 μs 683.5 μs -1.7%
auction_2-3 917.3 μs 887.3 μs -3.3%
auction_2-4 690.7 μs 669.5 μs -3.1%
auction_2-5 223.3 μs 220.3 μs -1.3%
crowdfunding-success-1 202.9 μs 202.5 μs -0.2%
crowdfunding-success-2 202.4 μs 202.2 μs -0.1%
crowdfunding-success-3 202.2 μs 201.9 μs -0.1%
currency-1 255.0 μs 251.3 μs -1.5%
escrow-redeem_1-1 353.4 μs 356.2 μs +0.8%
escrow-redeem_1-2 354.7 μs 353.4 μs -0.4%
escrow-redeem_2-1 417.9 μs 417.2 μs -0.2%
escrow-redeem_2-2 417.6 μs 420.8 μs +0.8%
escrow-redeem_2-3 419.4 μs 419.0 μs -0.1%
escrow-refund-1 150.4 μs 150.2 μs -0.1%
future-increase-margin-1 255.3 μs 251.9 μs -1.3%
future-increase-margin-2 558.4 μs 556.3 μs -0.4%
future-increase-margin-3 559.8 μs 560.1 μs +0.1%
future-increase-margin-4 524.0 μs 516.0 μs -1.5%
future-increase-margin-5 894.5 μs 893.6 μs -0.1%
future-pay-out-1 257.9 μs 253.7 μs -1.6%
future-pay-out-2 560.5 μs 554.3 μs -1.1%
future-pay-out-3 560.0 μs 555.2 μs -0.9%
future-pay-out-4 890.4 μs 875.0 μs -1.7%
future-settle-early-1 256.3 μs 251.5 μs -1.9%
future-settle-early-2 558.6 μs 553.8 μs -0.9%
future-settle-early-3 562.2 μs 555.1 μs -1.3%
future-settle-early-4 658.8 μs 649.8 μs -1.4%
game-sm-success_1-1 404.6 μs 400.7 μs -1.0%
game-sm-success_1-2 192.4 μs 190.0 μs -1.2%
game-sm-success_1-3 683.5 μs 672.3 μs -1.6%
game-sm-success_1-4 224.0 μs 219.8 μs -1.9%
game-sm-success_2-1 405.7 μs 398.5 μs -1.8%
game-sm-success_2-2 192.8 μs 189.8 μs -1.6%
game-sm-success_2-3 682.8 μs 674.5 μs -1.2%
game-sm-success_2-4 222.9 μs 219.2 μs -1.7%
game-sm-success_2-5 682.5 μs 675.4 μs -1.0%
game-sm-success_2-6 223.7 μs 219.9 μs -1.7%
multisig-sm-1 417.2 μs 411.5 μs -1.4%
multisig-sm-2 414.6 μs 401.8 μs -3.1%
multisig-sm-3 416.7 μs 409.4 μs -1.8%
multisig-sm-4 421.6 μs 415.4 μs -1.5%
multisig-sm-5 604.3 μs 591.9 μs -2.1%
multisig-sm-6 417.3 μs 409.7 μs -1.8%
multisig-sm-7 412.6 μs 401.3 μs -2.7%
multisig-sm-8 417.5 μs 409.6 μs -1.9%
multisig-sm-9 419.8 μs 413.3 μs -1.5%
multisig-sm-10 603.7 μs 592.7 μs -1.8%
ping-pong-1 345.2 μs 342.2 μs -0.9%
ping-pong-2 347.2 μs 344.4 μs -0.8%
ping-pong_2-1 204.1 μs 197.9 μs -3.0%
prism-1 161.1 μs 159.0 μs -1.3%
prism-2 434.3 μs 437.4 μs +0.7%
prism-3 369.8 μs 373.6 μs +1.0%
pubkey-1 136.3 μs 135.9 μs -0.3%
stablecoin_1-1 989.1 μs 980.8 μs -0.8%
stablecoin_1-2 187.7 μs 185.7 μs -1.1%
stablecoin_1-3 1.136 ms 1.131 ms -0.4%
stablecoin_1-4 199.8 μs 197.6 μs -1.1%
stablecoin_1-5 1.472 ms 1.440 ms -2.2%
stablecoin_1-6 258.3 μs 244.5 μs -5.3%
stablecoin_2-1 1.016 ms 982.0 μs -3.3%
stablecoin_2-2 191.2 μs 186.0 μs -2.7%
stablecoin_2-3 1.145 ms 1.121 ms -2.1%
stablecoin_2-4 201.9 μs 197.5 μs -2.2%
token-account-1 191.0 μs 185.9 μs -2.7%
token-account-2 339.1 μs 332.8 μs -1.9%
uniswap-1 433.6 μs 422.2 μs -2.6%
uniswap-2 220.4 μs 214.4 μs -2.7%
uniswap-3 1.923 ms 1.867 ms -2.9%
uniswap-4 325.4 μs 314.5 μs -3.3%
uniswap-5 1.227 ms 1.206 ms -1.7%
uniswap-6 312.0 μs 304.6 μs -2.4%
vesting-1 360.9 μs 359.9 μs -0.3%

michaelpj
michaelpj approved these changes Mar 30, 2023
View reviewed changes
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.

MUCH better IMO.

plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug/Driver.hs
(Breakpointable ann bps, MonadFree (DebugF uni fun ann bps) m)
=> NTerm uni fun ann -> m ()
runDriver = void . runReaderT driver . initState
runDriverT = void . runReaderT driver . initState
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a kind of weird name. It sounds like it should be a function that gets you out of the DriverT monad transformer... but that's definitely not what's happening here?

Copy link
Contributor Author

@bezirg bezirg Mar 31, 2023 •
edited

Choose a reason for hiding this comment

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

It is indeed a weird name. It does not actually run the whole Driving monad stack (i.e. reader,free); it only runs the Reader CekState which is the state of the driver.
So it is very similar to runReaderT. If you have a better name, let me know!

plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug/Internal.hs Outdated Show resolved Hide resolved
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Internal.hs
@@ -58,6 +58,7 @@ module UntypedPlutusCore.Evaluation.Machine.Cek.Internal
, Slippage
, defaultSlippage
, NTerm
, runCekM
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used somewhere? I guess it must be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by the steppable cek to provide the same runCekDeBruijn run-to-completion interface as the original cek

@bezirg bezirg force-pushed the bezirg/dbg-run branch 2 times, most recently from ba7c3f0 to 7776795 Compare March 31, 2023 12:39
@bezirg bezirg merged commit fd4f1f5 into master Mar 31, 2023
304 of 306 checks passed
@bezirg bezirg deleted the bezirg/dbg-run branch March 31, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj approved these changes

@zliu41 zliu41 zliu41 approved these changes

Assignees

@bezirg bezirg

Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bezirg @michaelpj @zliu41