[Builtins] [Costing] Make costing more sharing-friendly #4505
Conversation
effectfully
added
Builtins
Performance
Costing
|
/benchmark plutus-benchmark:validation |
This comment was marked as outdated.
This comment was marked as outdated.
|
Well, that's sad. |
|
Wait, we have |
|
Ok, the Core is completely wrong, looking into it. |
|
/benchmark plutus-benchmark:validation |
This comment was marked as outdated.
This comment was marked as outdated.
|
-1.74% on average, which is nice, given how minimal the changes are. The generated Core now does look much better. The benchmarking results are kinda shaky though, gonna run |
|
/benchmark plutus-benchmark:nofib |
This comment was marked as outdated.
This comment was marked as outdated.
|
/benchmark plutus-benchmark:validation |
This comment was marked as outdated.
This comment was marked as outdated.
|
/benchmark plutus-benchmark:validation |
This comment was marked as outdated.
This comment was marked as outdated.
|
/benchmark plutus-benchmark:nofib |
This comment was marked as outdated.
This comment was marked as outdated.
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '33341b3fb' (base) and 'f02339369' (PR)
|
This reverts commit f023393.
|
/benchmark plutus-benchmark:nofib |
|
Comparing benchmark results of 'plutus-benchmark:nofib' on '33341b3fb' (base) and 'a59aba586' (PR)
|
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '33341b3fb' (base) and 'a59aba586' (PR)
|
| deriving via ModelJSON "costingFun" (CostingFun model) | ||
| instance FromJSON model => FromJSON (CostingFun model) | ||
| deriving via ModelJSON "costingFun" (CostingFun model) | ||
| instance ToJSON model => ToJSON (CostingFun model) |
There was a problem hiding this comment.
Is there any way to check that I didn't screw up JSON encoding/decoding?
|
|
||
| -- | A separate module for JSON instances, so that we can stick @-O0@ on it and avoid spending | ||
| -- a lot of time optimizing loads of Core whose performance doesn't matter. | ||
| module PlutusCore.Evaluation.Machine.CostingFun.JSON () where |
There was a problem hiding this comment.
Now in a separate file, so that we can keep -O0 here and use the defaults for the module with runCostingFunOneArgument etc functions (which helped by ~0.75%, which isn't much, but also saved my sanity because I now don't need to wait an eternity to see how Core changes after a tweak).
There was a problem hiding this comment.
Getting rid of -O0 removed a bunch of matching to extract an Int# from the arguments of ModelLinearSize etc. Which is particularly good since that matching wasn't getting cached (likely due to -O0 as well). Now the arguments are unpacked, so we don't need to extract that Int# at all.
|
Ready for review. |
|
@kwxm I think it still would be great to just define the costing functions manually without matching on models to check if we're squeezing enough of performance out of what we have right now. |
|
Example Core output for those who are curious: -- RHS size: {terms: 59, types: 30, coercions: 13, joins: 0/1}
runOneArgumentModel
= \ ds_dpuo ->
case ds_dpuo of {
ModelOneArgumentConstantCost dt_dpDt ->
let { c_sqx7 = I# dt_dpDt } in
lazy ((\ _ -> c_sqx7) `cast` <Co:4>);
ModelOneArgumentLinearCost ds1_dpvO ->
case ds1_dpvO of { ModelLinearSize dt_svSs dt1_svSt ->
lazy
(\ ds2_dpux ->
case ds2_dpux `cast` <Co:3> of { I# ww1_iq8j ->
case $w$c* ww1_iq8j dt1_svSt of ww4_iq8o { __DEFAULT ->
case addIntC# ww4_iq8o dt_svSs of { (# r#_iq89, ds3_iq8a #) ->
case ds3_iq8a of {
__DEFAULT ->
case andI# (># ww4_iq8o 0#) (># dt_svSs 0#) of {
__DEFAULT ->
case andI# (<# ww4_iq8o 0#) (<# dt_svSs 0#) of {
__DEFAULT -> case overflowError of wild3_00 { };
1# -> lvl31_rwWK `cast` <Co:2>
};
1# -> lvl32_rwWL `cast` <Co:2>
};
0# -> (I# r#_iq89) `cast` <Co:2>
}
}
}
})
}
}
-- RHS size: {terms: 25, types: 17, coercions: 2, joins: 0/0}
runCostingFunOneArgument
= \ ds_dpvP ->
case ds_dpvP of { CostingFun cpu_ahk7 mem_ahk8 ->
case runOneArgumentModel mem_ahk8 of runMem_ahka { __DEFAULT ->
case runOneArgumentModel cpu_ahk7 of runCpu_ahk9 { __DEFAULT ->
lazy
(\ mem1_ahkb ->
case (runCpu_ahk9 mem1_ahkb) `cast` <Co:1> of { I# dt1_inIU ->
case (runMem_ahka mem1_ahkb) `cast` <Co:1> of { I# dt3_inIW ->
ExBudget dt1_inIU dt3_inIW
}
})
}
}
}Seems pretty nice. It kinda sucks |
|
The speedup is 2.51% on average. |
|
/benchmark plutus-benchmark:validation |
| runCostingFunTwoArguments (CostingFun cpu mem) = | ||
| let !runCpu = runTwoArgumentModel cpu | ||
| !runMem = runTwoArgumentModel mem | ||
| in lazy $ \mem1 mem2 -> |
There was a problem hiding this comment.
Does using a local definition stop the floating?
runCostingFunTwoArguments (CostingFun cpu mem) =
let !runCpu = ...
!runMem = ...
go = \mem1 mem2 -> ...
in go
There was a problem hiding this comment.
Used case instead of let (suggested by Heinrich Apfelmus) and we don't need that lazy nonsense in there now. We still need some elsewhere though.
| deriving via ModelJSON "costingFun" (CostingFun model) | ||
| instance FromJSON model => FromJSON (CostingFun model) | ||
| deriving via ModelJSON "costingFun" (CostingFun model) | ||
| instance ToJSON model => ToJSON (CostingFun model) |
| they are partially computed and cached, which results in them being called only once per builtin | ||
| 2. the resulting lambda is wrapped with a call to 'lazy', so that GHC doesn't float the let-bound | ||
| functions inside the lambda | ||
| 3. the whole definition is marked with @INLINE@, because it gets worker-wrapper transformed and we |
There was a problem hiding this comment.
I guess specifically we can't benefit from worker-wrapper, because at the call sites we don't have a statically known usage of the ExBudget return value, so we can't unbox it nicely.
|
There's an interesting little puzzle here, you could ask in #ghc. The combination of wanting to get a partially-applied function returned (so you share the work from the first argument), combined with worker-wrapper-ing the second part... |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '33341b3fb' (base) and '2b6f1f522' (PR)
|
|
-3.2% on average with the slightly changed approach. Docs updated. Gonna merge this once CI is green. |
effectfully
deleted the
effectfully/builtins/costing/make-costing-more-sharing-friendly
branch
Dunno. Without
Where's that #ghc? I merged the PR for now, but I'll probably investigate further. It's a funny puzzle indeed. |
|
#ghc being a (pretty active) IRC channel with the GHC devs in it. |
Let's see if that helps. Don't look here yet.