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

[Builtins] Monomorphize 'makeKnown' #4308

Closed
wants to merge 1 commit into from
Closed

[Builtins] Monomorphize 'makeKnown' #4308

wants to merge 1 commit into from

Conversation

effectfully
Copy link
Contributor

Don't look here yet.

@effectfully
Copy link
Contributor Author

/benchmark

@iohk-devops
Copy link

Comparing benchmark results of 'c8c5183f7' (base) and '9f5d83965' (PR)

Script c8c5183 9f5d839 Change
auction_1-1 385.0 μs 383.8 μs -0.3%
auction_1-2 1.158 ms 1.168 ms +0.9%
auction_1-3 1.162 ms 1.171 ms +0.8%
auction_1-4 511.1 μs 514.2 μs +0.6%
auction_2-1 388.4 μs 388.3 μs -0.0%
auction_2-2 1.156 ms 1.169 ms +1.1%
auction_2-3 1.450 ms 1.476 ms +1.8%
auction_2-4 1.162 ms 1.171 ms +0.8%
auction_2-5 507.3 μs 509.5 μs +0.4%
crowdfunding-success-1 455.4 μs 453.7 μs -0.4%
crowdfunding-success-2 455.2 μs 453.9 μs -0.3%
crowdfunding-success-3 455.6 μs 454.0 μs -0.4%
currency-1 468.0 μs 472.8 μs +1.0%
escrow-redeem_1-1 710.0 μs 716.9 μs +1.0%
escrow-redeem_1-2 709.8 μs 715.7 μs +0.8%
escrow-redeem_2-1 815.2 μs 822.0 μs +0.8%
escrow-redeem_2-2 815.7 μs 821.4 μs +0.7%
escrow-redeem_2-3 816.5 μs 823.7 μs +0.9%
escrow-refund-1 336.7 μs 335.9 μs -0.2%
future-increase-margin-1 469.2 μs 471.9 μs +0.6%
future-increase-margin-2 1.035 ms 1.044 ms +0.9%
future-increase-margin-3 1.031 ms 1.043 ms +1.2%
future-increase-margin-4 942.3 μs 954.5 μs +1.3%
future-increase-margin-5 1.391 ms 1.408 ms +1.2%
future-pay-out-1 469.3 μs 473.2 μs +0.8%
future-pay-out-2 1.034 ms 1.046 ms +1.2%
future-pay-out-3 1.034 ms 1.045 ms +1.1%
future-pay-out-4 1.384 ms 1.405 ms +1.5%
future-settle-early-1 468.2 μs 474.5 μs +1.3%
future-settle-early-2 1.032 ms 1.048 ms +1.6%
future-settle-early-3 1.029 ms 1.047 ms +1.7%
future-settle-early-4 1.094 ms 1.108 ms +1.3%
game-sm-success_1-1 787.3 μs 796.1 μs +1.1%
game-sm-success_1-2 430.5 μs 433.2 μs +0.6%
game-sm-success_1-3 1.167 ms 1.177 ms +0.9%
game-sm-success_1-4 505.3 μs 505.9 μs +0.1%
game-sm-success_2-1 790.4 μs 793.6 μs +0.4%
game-sm-success_2-2 430.2 μs 431.4 μs +0.3%
game-sm-success_2-3 1.163 ms 1.176 ms +1.1%
game-sm-success_2-4 505.0 μs 504.8 μs -0.0%
game-sm-success_2-5 1.165 ms 1.175 ms +0.9%
game-sm-success_2-6 506.3 μs 505.2 μs -0.2%
multisig-sm-1 796.7 μs 796.0 μs -0.1%
multisig-sm-2 783.9 μs 785.7 μs +0.2%
multisig-sm-3 791.3 μs 794.8 μs +0.4%
multisig-sm-4 798.7 μs 799.0 μs +0.0%
multisig-sm-5 1.035 ms 1.044 ms +0.9%
multisig-sm-6 794.6 μs 796.9 μs +0.3%
multisig-sm-7 780.4 μs 783.2 μs +0.4%
multisig-sm-8 789.7 μs 795.6 μs +0.7%
multisig-sm-9 799.1 μs 803.0 μs +0.5%
multisig-sm-10 1.034 ms 1.047 ms +1.3%
ping-pong-1 652.5 μs 656.2 μs +0.6%
ping-pong-2 652.8 μs 657.4 μs +0.7%
ping-pong_2-1 417.7 μs 416.2 μs -0.4%
prism-1 360.2 μs 360.0 μs -0.1%
prism-2 854.1 μs 857.6 μs +0.4%
prism-3 731.8 μs 733.1 μs +0.2%
pubkey-1 307.2 μs 307.0 μs -0.1%
stablecoin_1-1 1.632 ms 1.645 ms +0.8%
stablecoin_1-2 425.5 μs 421.1 μs -1.0%
stablecoin_1-3 1.856 ms 1.879 ms +1.2%
stablecoin_1-4 451.3 μs 448.2 μs -0.7%
stablecoin_1-5 2.360 ms 2.407 ms +2.0%
stablecoin_1-6 556.7 μs 556.4 μs -0.1%
stablecoin_2-1 1.624 ms 1.646 ms +1.4%
stablecoin_2-2 422.6 μs 419.9 μs -0.6%
stablecoin_2-3 1.848 ms 1.882 ms +1.8%
stablecoin_2-4 451.3 μs 449.7 μs -0.4%
token-account-1 369.4 μs 372.1 μs +0.7%
token-account-2 651.7 μs 657.1 μs +0.8%
uniswap-1 737.4 μs 748.4 μs +1.5%
uniswap-2 450.6 μs 452.9 μs +0.5%
uniswap-3 2.999 ms 3.035 ms +1.2%
uniswap-4 734.7 μs 732.4 μs -0.3%
uniswap-5 2.150 ms 2.171 ms +1.0%
uniswap-6 699.0 μs 696.5 μs -0.4%
vesting-1 668.2 μs 672.9 μs +0.7%

@effectfully effectfully force-pushed the effectfully/builtins/performance/monomorphize-makeKnown branch from 9f5d839 to ab46d64 Compare January 3, 2022 17:45
@effectfully
Copy link
Contributor Author

/benchmark

@iohk-devops
Copy link

Comparing benchmark results of 'c8c5183f7' (base) and 'ab46d64f5' (PR)

Script c8c5183 ab46d64 Change
auction_1-1 382.7 μs 381.4 μs -0.3%
auction_1-2 1.153 ms 1.168 ms +1.3%
auction_1-3 1.156 ms 1.170 ms +1.2%
auction_1-4 508.0 μs 506.3 μs -0.3%
auction_2-1 385.8 μs 384.5 μs -0.3%
auction_2-2 1.154 ms 1.169 ms +1.3%
auction_2-3 1.452 ms 1.478 ms +1.8%
auction_2-4 1.160 ms 1.170 ms +0.9%
auction_2-5 510.2 μs 506.8 μs -0.7%
crowdfunding-success-1 456.9 μs 455.0 μs -0.4%
crowdfunding-success-2 457.3 μs 453.9 μs -0.7%
crowdfunding-success-3 459.0 μs 454.2 μs -1.0%
currency-1 470.6 μs 474.2 μs +0.8%
escrow-redeem_1-1 712.8 μs 716.6 μs +0.5%
escrow-redeem_1-2 711.5 μs 716.9 μs +0.8%
escrow-redeem_2-1 818.7 μs 823.3 μs +0.6%
escrow-redeem_2-2 819.3 μs 823.0 μs +0.5%
escrow-redeem_2-3 818.1 μs 822.5 μs +0.5%
escrow-refund-1 337.3 μs 335.9 μs -0.4%
future-increase-margin-1 469.5 μs 473.0 μs +0.7%
future-increase-margin-2 1.035 ms 1.048 ms +1.3%
future-increase-margin-3 1.037 ms 1.050 ms +1.3%
future-increase-margin-4 950.7 μs 954.1 μs +0.4%
future-increase-margin-5 1.392 ms 1.410 ms +1.3%
future-pay-out-1 469.9 μs 472.7 μs +0.6%
future-pay-out-2 1.037 ms 1.043 ms +0.6%
future-pay-out-3 1.041 ms 1.044 ms +0.3%
future-pay-out-4 1.398 ms 1.400 ms +0.1%
future-settle-early-1 470.2 μs 473.3 μs +0.7%
future-settle-early-2 1.037 ms 1.047 ms +1.0%
future-settle-early-3 1.038 ms 1.047 ms +0.9%
future-settle-early-4 1.093 ms 1.108 ms +1.4%
game-sm-success_1-1 788.8 μs 797.0 μs +1.0%
game-sm-success_1-2 430.5 μs 432.6 μs +0.5%
game-sm-success_1-3 1.164 ms 1.182 ms +1.5%
game-sm-success_1-4 506.3 μs 504.5 μs -0.4%
game-sm-success_2-1 788.1 μs 797.6 μs +1.2%
game-sm-success_2-2 433.0 μs 432.5 μs -0.1%
game-sm-success_2-3 1.169 ms 1.180 ms +0.9%
game-sm-success_2-4 506.9 μs 504.3 μs -0.5%
game-sm-success_2-5 1.166 ms 1.182 ms +1.4%
game-sm-success_2-6 505.9 μs 502.9 μs -0.6%
multisig-sm-1 796.2 μs 796.1 μs -0.0%
multisig-sm-2 780.7 μs 784.9 μs +0.5%
multisig-sm-3 790.5 μs 793.8 μs +0.4%
multisig-sm-4 799.1 μs 802.2 μs +0.4%
multisig-sm-5 1.042 ms 1.048 ms +0.6%
multisig-sm-6 799.4 μs 799.4 μs 0.0%
multisig-sm-7 784.2 μs 788.0 μs +0.5%
multisig-sm-8 792.3 μs 795.3 μs +0.4%
multisig-sm-9 797.7 μs 802.4 μs +0.6%
multisig-sm-10 1.036 ms 1.044 ms +0.8%
ping-pong-1 653.0 μs 654.4 μs +0.2%
ping-pong-2 651.1 μs 657.4 μs +1.0%
ping-pong_2-1 416.8 μs 416.3 μs -0.1%
prism-1 359.7 μs 357.6 μs -0.6%
prism-2 853.6 μs 861.5 μs +0.9%
prism-3 731.4 μs 735.7 μs +0.6%
pubkey-1 306.6 μs 308.2 μs +0.5%
stablecoin_1-1 1.623 ms 1.646 ms +1.4%
stablecoin_1-2 423.0 μs 420.1 μs -0.7%
stablecoin_1-3 1.860 ms 1.885 ms +1.3%
stablecoin_1-4 451.2 μs 447.4 μs -0.8%
stablecoin_1-5 2.361 ms 2.399 ms +1.6%
stablecoin_1-6 558.1 μs 554.7 μs -0.6%
stablecoin_2-1 1.625 ms 1.645 ms +1.2%
stablecoin_2-2 423.6 μs 420.3 μs -0.8%
stablecoin_2-3 1.852 ms 1.885 ms +1.8%
stablecoin_2-4 450.1 μs 447.6 μs -0.6%
token-account-1 368.4 μs 369.5 μs +0.3%
token-account-2 653.4 μs 657.2 μs +0.6%
uniswap-1 739.5 μs 752.4 μs +1.7%
uniswap-2 449.0 μs 454.8 μs +1.3%
uniswap-3 2.980 ms 3.057 ms +2.6%
uniswap-4 734.8 μs 734.3 μs -0.1%
uniswap-5 2.137 ms 2.180 ms +2.0%
uniswap-6 701.3 μs 697.6 μs -0.5%
vesting-1 669.6 μs 673.8 μs +0.6%

@effectfully
Copy link
Contributor Author

It does make sense that this is slower than the baseline. makeKnown does not normally contain any >>= to get inlined and in this PR we return a tuple, which does require some bookkeeping, plus we still want a monad, so we have to make a monoid out of the log messages, which is cheap, but still not as cheap as not doing it, because now we have to always call traverse_ over the log messages, which is cheap but wasteful in the vast majority of cases, since we don't return log messages that often. Plus what's in master is kinda like a Church-encoded version of what we have here.

So overall, this is slower than master for valid reasons, so I'm closing the PR (I'll add some docs about this experiment in the PR that does the same, but for readKnown).

@effectfully effectfully closed this Jan 3, 2022
@effectfully effectfully deleted the effectfully/builtins/performance/monomorphize-makeKnown branch January 3, 2022 23:22
@michaelpj
Copy link
Contributor

Also the results are pretty close to the noise threshold! Do agree it's not worth it, though.

@michaelpj michaelpj added the EXPERIMENT Experiments that we probably don't want to merge label Jan 4, 2022
@michaelpj
Copy link
Contributor

That said... I'd be somewhat tempted to do this anyway just to make it consistent with readKnown if we merge that!

@effectfully
Copy link
Contributor Author

I'll split the KnownTypeIn class into two classes or do something even more drastic anyway, so let's not worry about consistency for now.

@effectfully
Copy link
Contributor Author

This is also not the first time when a certain transformation affects makeKnown differently than what it does for readKnown. For example passing throwError from MonadError worked better for makeKnown than it did for readKnown. I'm currently going after maximum performance, so I don't want to commit to any restricting design choices.

Plus I simply dislike this version of the code.

And yeah, 2+% slowdown is double the noise threshold.

@effectfully effectfully mentioned this pull request Feb 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Builtins Don't look here yet EXPERIMENT Experiments that we probably don't want to merge Performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@effectfully @iohk-devops @michaelpj