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

Optimise the balance function #1771

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Optimise the balance function #1771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/STS/Chain.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ data AdaPots = AdaPots
deriving (Show, Eq)

-- | Calculate the total ada pots in the chain state
totalAdaPots :: Crypto crypto => ChainState crypto -> AdaPots
totalAdaPots :: ChainState crypto -> AdaPots
totalAdaPots (ChainState nes _ _ _ _ _ _) =
AdaPots
{ treasuryAdaPot = treasury_,
Expand All @@ -381,7 +381,7 @@ totalAdaPots (ChainState nes _ _ _ _ _ _) =
circulation = balance u

-- | Calculate the total ada in the chain state
totalAda :: Crypto crypto => ChainState crypto -> Coin
totalAda :: ChainState crypto -> Coin
totalAda cs =
treasuryAdaPot + reservesAdaPot + rewardsAdaPot + utxoAdaPot + depositsAdaPot + feesAdaPot
where
Expand Down
7 changes: 4 additions & 3 deletions shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/UTxO.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{-# LANGUAGE BangPatterns #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DerivingVia #-}
Expand Down Expand Up @@ -223,10 +224,10 @@ makeWitnessesFromScriptKeys txbodyHash hashKeyMap scriptHashes =
in makeWitnessesVKey txbodyHash (Map.elems witKeys)

-- | Determine the total balance contained in the UTxO.
balance :: Crypto crypto => UTxO crypto -> Coin
balance (UTxO utxo) = foldr addCoins 0 utxo
balance :: UTxO crypto -> Coin
balance (UTxO utxo) = fromIntegral $ Map.foldl' addCoins 0 utxo
Copy link
Contributor Author

@mrBliss mrBliss Aug 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note that I use Word64 to sum them and only convert to Coin at the end. Is this fine or do we expect an overflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

a property of the system is that there are never more than 45 * 10 ^ 15 coins in this map, so 2 ^ 64 is more that enough. that said, if there ever were a bug, this could obfuscate the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a property of the system is that there are never more than 45 * 10 ^ 15 coins in this map, so 2 ^ 64 is more that enough.

I see.

that said, if there ever were a bug, this could obfuscate the problem.

So what do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, when summing using Coin instead of Word64:

time                 36.17 ms   (35.04 ms .. 37.35 ms)
                     0.998 R²   (0.996 R² .. 0.999 R²)
mean                 36.61 ms   (36.15 ms .. 37.22 ms)
std dev              1.112 ms   (812.8 μs .. 1.637 ms)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong preference, I'm just making sure we all have the same understanding. Thank you for helping out with the performance @mrBliss !

where
addCoins (TxOut _ a) b = a + b
addCoins !b (TxOutCompact _ a) = a + b

-- | Determine the total deposit amount needed.
-- The block may (legitimately) contain multiple registration certificates
Expand Down