Improve fee estimation performance / fix accidentally quadratic function #2116
Conversation
KtorZ
added
the
IMPROVEMENT
KtorZ
force-pushed
the
KtorZ/2032/fee-estimation-slow
branch
from
94d4812 to
d4b0fd9
Compare
| @@ -109,12 +107,12 @@ data TargetRange = TargetRange | |||
| -- we set. | |||
| -- @ | |||
| random | |||
| :: forall m e. MonadRandom m | |||
| :: forall e. () | |||
There was a problem hiding this comment.
Perhaps in future, rather than using IO, we could explicity pass around the DRG state. Or maybe provide an infinite list of random UTxO to the function (if possible).
There was a problem hiding this comment.
Yes, testing was made a little harder because of this, plus, we also now loose the nice type signature that was nicely indicating which side-effects were needed. I might as well flag it as a technical debt to address later on and restore some purity in this.
| :: Fee | ||
| -> StateT UTxO (ExceptT ErrAdjustForFee IO) ([(TxIn, TxOut)], Coin) | ||
| coverRemainingFee (Fee fee) = go [] 0 where | ||
| go additionalInputs surplus |
| Just entry -> | ||
| go (entry : acc) | ||
| Just input@(_, out) -> | ||
| go (input : additionalInputs) (getCoin $ coin out) |
There was a problem hiding this comment.
Does the value of out need to be added to surplus here?
There was a problem hiding this comment.
🤦 .... yes. That's the whole point.
There was a problem hiding this comment.
Fortunately our test suite catches it too 💪
There was a problem hiding this comment.
Aaaand, fixed, tests are now passing :)
| @@ -1231,7 +1228,10 @@ balance = | |||
| -- | Compute the balance of a unwrapped UTxO | |||
| balance' :: [(TxIn, TxOut)] -> Word64 | |||
| balance' = | |||
| fromIntegral . balance . UTxO . Map.fromList | |||
| foldl' fn 0 | |||
There was a problem hiding this comment.
What about just sum . map (getCoin . coin . snd)?
There was a problem hiding this comment.
I've went for sum initially and switch to foldl' for consistency with the other balance calculation. It makes it more obvious that the same thing is happening.
…f resolved inputs This is somehow causing massive slowness in the fee estimation. Changing this reduces the time in benchmarks from 18,000ms to 3,000ms!!
There's really no need for strong cryptographic randomness at this level, this is needlessly slower than it could be an called pretty often during fee calculation.
Instead, compute the balance incrementally. The 'coverRemainingFee' was computing the balance of the extra inputs selected to cover for fee. In worst-case scenario where many inputs needed to be selected, for example, 1000 extra inputs, it'll recompute the balance of an ever growing list a thousand times! Not only this is accidentally quadratic, but, it is also completely unbounded and will potentially select way too many inputs. In theory, this should not select more than the max allowed inputs. Next commit will fix this.
KtorZ
force-pushed
the
KtorZ/2032/fee-estimation-slow
branch
from
d4b0fd9 to
89f8a06
Compare
|
bors r+ |
|
Build succeeded: |
Issue Number
#2032 & rel
Overview
ca9dd1e
📍 remove use of DerivingVia in Sqlite types: can't build with --profile with it
bc1bacc
📍 avoid re-creating and folding on a map to compute balance of a list of resolved inputs
This is somehow causing massive slowness in the fee estimation. Changing this reduces the time in benchmarks from 18,000ms to 3,000ms!!
f82a7fe
📍 use standard random generator for picking random UTxO
There's really no need for strong cryptographic randomness at this level, this is needlessly slower than it could be an called pretty often during fee calculation.
94d4812
📍 do not re-compute the full balance after picking new inputs
Instead, compute the balance incrementally. The 'coverRemainingFee' was computing the balance of the extra inputs selected to cover for fee. In worst-case scenario where many inputs needed to be selected, for example, 1000 extra inputs, it'll recompute the balance of an ever growing list a thousand times! Not only this is accidentally quadratic, but, it is also completely unbounded and will potentially select way too many inputs. In theory, this should not select more than the max allowed inputs. Next commit will fix this.
Comments
Before:
After: