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

Add transient tx map to DaoState to speed up getTx queries #3773

Merged
merged 3 commits into from
Dec 16, 2019
Merged

Add transient tx map to DaoState to speed up getTx queries #3773

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fa21b5
Add transient tx map to DaoState to speed up getTx queries
stejbac Dec 9, 2019
4277cf8
Use unordered Tx stream from daoState.txMap wherever possible
stejbac Dec 11, 2019
b22e4ad
Encapsulate mutable Block tx list and DaoState tx cache
stejbac Dec 12, 2019
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
5 changes: 1 addition & 4 deletions core/src/main/java/bisq/core/dao/node/parser/BlockParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import bisq.core.dao.node.parser.exceptions.BlockHeightNotConnectingException;
import bisq.core.dao.state.DaoStateService;
import bisq.core.dao.state.model.blockchain.Block;
import bisq.core.dao.state.model.blockchain.Tx;

import bisq.common.app.DevEnv;

Expand All @@ -31,7 +30,6 @@
import javax.inject.Inject;

import java.util.LinkedList;
import java.util.List;

import lombok.extern.slf4j.Slf4j;

Expand Down Expand Up @@ -106,14 +104,13 @@ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingExceptio
// one get resolved.
// Lately there is a patter with 24 iterations observed
long startTs = System.currentTimeMillis();
List<Tx> txList = block.getTxs();

rawBlock.getRawTxs().forEach(rawTx ->
txParser.findTx(rawTx,
genesisTxId,
genesisBlockHeight,
genesisTotalSupply)
.ifPresent(txList::add));
.ifPresent(tx -> daoStateService.onNewTxForLastBlock(block, tx)));

log.info("Parsing {} transactions at block height {} took {} ms", rawBlock.getRawTxs().size(),
blockHeight, System.currentTimeMillis() - startTs);
Expand Down
27 changes: 21 additions & 6 deletions core/src/main/java/bisq/core/dao/state/DaoStateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,21 @@
import bisq.core.dao.state.model.governance.Issuance;
import bisq.core.dao.state.model.governance.IssuanceType;
import bisq.core.dao.state.model.governance.ParamChange;
import bisq.core.util.coin.BsqFormatter;
import bisq.core.util.ParsingUtils;
import bisq.core.util.coin.BsqFormatter;

import org.bitcoinj.core.Coin;

import javax.inject.Inject;

import com.google.common.base.Preconditions;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
Expand Down Expand Up @@ -115,6 +118,9 @@ public void applySnapshot(DaoState snapshot) {

daoState.setChainHeight(snapshot.getChainHeight());

daoState.getTxMap().clear();
daoState.getTxMap().putAll(snapshot.getTxMap());
Copy link
Contributor

Choose a reason for hiding this comment

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

How did caching at the DaoState level compare to caching at the Block level? Keeping object in-sync is complicated and I'd be interested in understanding if the simpler block-level cache has most of the gain without any of the synchronization complication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure what you mean by caching at the block level - do you mean adding a transient field of some kind to Block?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Instead of keeping a map of all txns that needs to be kept in sync in the DaoState object, the Block could just cache the list of transactions for itself and the lookup functions changed from O(txns) to O(blocks). I only bring this up because it would require less complexity and since there are no tests it may make it easier to guarantee correctness through just code review.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fear that might not have much effect on performance as usually there are very few txs in a block. In average we have 1 tx in 2 blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

The snapshot is created from the persisted state, but the transient map isn't saved to disk. Is the tx map always empty after applying a snapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it shouldn't be because the snapshot is just another DaoState instance constructed via DaoState.fromProto(), and in that method it recalculates and populates the transient field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. The DaoStateStore calls DaoState.fromProto so the cache would be created.


daoState.getBlocks().clear();
daoState.getBlocks().addAll(snapshot.getBlocks());

Expand Down Expand Up @@ -226,7 +232,16 @@ public void onNewBlockWithEmptyTxs(Block block) {
}
}

// Third we get the onParseBlockComplete called after all rawTxs of blocks have been parsed
// Third we add each successfully parsed BSQ tx to the last block
public void onNewTxForLastBlock(Block block, Tx tx) {
// At least one block must be present else no rawTx would have been recognised as a BSQ tx.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add here the assertDaoStateChange(); check for conistency. All methods which alter the DAO state call that method to guard against changes outside of the permitted process. Only at parsing the DAO state must be changed.

Preconditions.checkArgument(block == getLastBlock().orElseThrow());
Copy link
Contributor

@chimp1984 chimp1984 Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This would change the original behaviour.

We used the onNewBlockWithEmptyTxs to add a block, but in case we got into the if path we did not add the block but only log a warning. Adding of transactions would had no effect as it was a local list in the original code. Now we throw an exception in such a case as we expect that the block has been added.

EDIT:
To be more clear. We should use a if case and ignore if the block is not the last block. This would reflect the existing behaviour.

public void onNewBlockWithEmptyTxs(Block block) {
        assertDaoStateChange();
        if (daoState.getBlocks().isEmpty() && block.getHeight() != getGenesisBlockHeight()) {
            log.warn("We don't have any blocks yet and we received a block which is not the genesis block. " +
                    "We ignore that block as the first block need to be the genesis block. " +
                    "That might happen in edge cases at reorgs. Received block={}", block);
        } else {
            daoState.getBlocks().add(block);

            if (parseBlockChainComplete)
                log.info("New Block added at blockHeight {}", block.getHeight());
        }
    }

I am not sure if that case is valid and can happen, but as the log suggests there might be tricky edge cases in re-org scenarious where this was a possible scenario. Testing those edge cases is pretty tricky and it can be that it was during development an issue which disappeared later and is not present anymore. But I would prefer to stay very conservative/restrictive in the DAO domain as a consensus bug can have severe consequences and the DAO has a very deep level of complexity. If we are not 100% sure that existing code is wrong I prefer to stick with it, as this code base has been tested excessively and is in production since April.

Copy link
Contributor

Choose a reason for hiding this comment

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

NACK for that Preconditions check, otherwise it looks good. Thanks for working on that.
I need a bit more time to go again over all and check if anything might be missing.

Could you provide a comparision of performance gains from that PR?


block.getTxs().add(tx);
daoState.getTxMap().put(tx.getId(), tx);
}

// Fourth we get the onParseBlockComplete called after all rawTxs of blocks have been parsed
public void onParseBlockComplete(Block block) {
if (parseBlockChainComplete)
log.info("Parse block completed: Block height {}, {} BSQ transactions.", block.getHeight(), block.getTxs().size());
Expand Down Expand Up @@ -348,16 +363,16 @@ public Stream<Tx> getTxStream() {
.flatMap(block -> block.getTxs().stream());
}

public TreeMap<String, Tx> getTxMap() {
return new TreeMap<>(getTxStream().collect(Collectors.toMap(Tx::getId, tx -> tx)));
public Map<String, Tx> getTxMap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like this function is used outside of this file. Probably worth inlining it appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed it was unused originally. Perhaps it should just be inlined.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always in favor of removing 1-line functions as well as public functions that are unused. Just one less way for people to do the wrong thing in the future.

return daoState.getTxMap();
}

public Set<Tx> getTxs() {
return getTxStream().collect(Collectors.toSet());
return new HashSet<>(getTxMap().values());
Copy link
Contributor

@julianknutsen julianknutsen Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What is the contract difference between getTxMap.values() and getTxStream()? Why have both?

If they have the same data with different performance characteristics I would be in favor of using composition here and having all users go through the cache. It is much less error-prone and easier to reason about when all users just need to deal with one object for txns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched it from getTxStream() to getTxMap.values() for slightly better performance, as more indirection is required to iterate through all the block tx lists (and many of the blocks are empty). They're not interchangeable elsewhere, though, since getTxMap.values() is unordered. It doesn't matter in this case since (before and after) it's just collecting into an unordered set.

Actually, the only place getTxs() is used is in one of the views to get the total number of transactions (using size()), so it doesn't need to collect into a new HashSet and the return type could just be a Collection.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are the callers I found of getTxStream(). It seems like most/all of them can be satisfied by the cache, but you have been looking at it longer than me so I might be missing a use case.

getInvalidTxs: size()
getIrregularTxs: size()
getTotalBurntFee: sum()
getTotalAmountOfInvalidatedBsq: sum()
getTotalAmountOfBurntBsq: sum()
getBurntFeeTxs: Set<Tx>
getTx: Optional<Tx>
getTxOutputStream: [boolean, Optional<TxOutput>, Set<TxOutput>

maybeExportToJson: <no idea if this needs to be ordered...>

My point at a high level was that if we are going to build a cache for something, we should just use it for everything. By keeping an interface and users for the old slow way, It just creates additional performance work in the future that could be avoided by doing something better now.

Adding javadocs as you change methods is also a great way to reduce the technical debt and help explain to future users the guarantees or any gotchas that you learned the hard way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked to all usages as well:
Here my comments:

getInvalidTxs
Used in UI for numTxs and in a Set for the chart. As it's a Set order is irrelevant.

getIrregularTxs: size()
Used in UI for numTxs -> order is irrelevant.

getTotalBurntFee: sum()
getTotalAmountOfInvalidatedBsq: sum()
getTotalAmountOfBurntBsq: sum()

Order is irrelevant for sum().

getBurntFeeTxs: Set<Tx>
Used in UI for numTxs and in a Set for the chart. As it's a Set order is irrelevant.

getTx: Optional<Tx>
TxId is unique so order is irrelevant.

getTxOutputStream: [boolean, Optional<TxOutput>, Set<TxOutput>
Used in existsTxOutput and getTxOutput:
Both use txOutputKeyand as txOutputKey is unique tx order is irrelevant.

maybeExportToJson
It dumps txs and txOutputs to disk using the txId and txOutputKey as file name. As both are unique, order is irrelevant.

I guess the explorer will love to get some performance improvement from that change as well ;-). Writing to disk at each new block will be probably a major performance iussue there (another "low hanging fruit" -;) ).

Also looked into all usages of block.getTxs() and I do not see any write beside the addition of a tx in onNewTxForLastBlock. Maybe a public method inside Block encapsulating the write operation and exposing the getTxs() as unmodifiable List would make the code more safe. The read access cannot be replaced by the txMap, but as the write for both data structures happen at only one place I think it is safe (we do not have multiple threads here).

All the other data objects are immutable, only with Block it was hard as during parsing we need to know the state of past transaction in the same block. So keeping that local as we do with txs would have duplicate lots of code in DoaStateService where we need to look up if for instance BSQ is already spent by a previous tx.

}

public Optional<Tx> getTx(String txId) {
return getTxStream().filter(tx -> tx.getId().equals(txId)).findAny();
return Optional.ofNullable(getTxMap().get(txId));
}

public List<Tx> getInvalidTxs() {
Expand Down
14 changes: 14 additions & 0 deletions core/src/main/java/bisq/core/dao/state/model/DaoState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import bisq.core.dao.state.model.blockchain.Block;
import bisq.core.dao.state.model.blockchain.SpentInfo;
import bisq.core.dao.state.model.blockchain.Tx;
import bisq.core.dao.state.model.blockchain.TxOutput;
import bisq.core.dao.state.model.blockchain.TxOutputKey;
import bisq.core.dao.state.model.governance.Cycle;
Expand All @@ -28,16 +29,19 @@
import bisq.core.dao.state.model.governance.ParamChange;

import bisq.common.proto.persistable.PersistablePayload;
import bisq.common.util.JsonExclude;

import com.google.protobuf.Message;

import javax.inject.Inject;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import java.util.function.Function;
import java.util.stream.Collectors;

import lombok.Getter;
Expand Down Expand Up @@ -98,6 +102,11 @@ public static DaoState getClone(DaoState daoState) {
@Getter
private final List<DecryptedBallotsWithMerits> decryptedBallotsWithMeritsList;

// Transient data used only as an index - must be kept in sync with the block list
@Getter
@JsonExclude
private transient final Map<String, Tx> txMap; // key is txId
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the memory footprint change with this cache and how is it expected to scale over time?

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's using a hash table rather than a tree set, so I don't think the additional memory will be a problem - there are only about 10,000 txs or so right now, so I don't think it will take up more than 100KB or so. (The fact that DaoState.blocks is a linked list instead of an array list is probably a more significant memory issue that could be easily fixed.)



///////////////////////////////////////////////////////////////////////////////////////////
// Constructor
Expand Down Expand Up @@ -145,6 +154,10 @@ private DaoState(int chainHeight,
this.paramChangeList = paramChangeList;
this.evaluatedProposalList = evaluatedProposalList;
this.decryptedBallotsWithMeritsList = decryptedBallotsWithMeritsList;

txMap = blocks.stream()
.flatMap(block -> block.getTxs().stream())
.collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> y, HashMap::new));
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the mergeFunction is only passed as you want to have the mapFactory. Not sure what the mergeFunction really should do as conflicts are not expected and not clear how to handle it. Maybe throwing an exception would be more appropriate here? Or maybe just add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to guarantee that the output is a HashMap and it looks like the only toMap overload with a mapFactory param also has a mergeFunction param, so I was forced to provide a dummy merge function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I expected that intention...

I just was wondering how we can be sure to not change behaviour. The previous code used the flatMap.

 public Stream<Tx> getTxStream() {
        return getBlocks().stream()
                .flatMap(block -> block.getTxs().stream());
    }

Do you know how potential key conflics would have been handled there? i assume your mergeFunction to overwrite with a new value if it happens is likely the standad behaviour if not otherwise defined. So your mergeFunction is likely better than throwing an exception if flatMap behaves the same. Anyway a bit "esoteric" but the DAO might deserve a bit of extra paranoia ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the (x, y) -> y merge function it's currently using, it will always select the last tx with a given txId from the tx stream when building the map, whereas in the original code getTx is calling findAny on the filtered getTxStream output, which will probably behave the same as findFirst in this case. So perhaps the merge function should be (x, y) -> x to be absolutely sure the behaviour doesn't change.

Also, for consistent merge behaviour, putIfAbsent would need to be substituted into the line:

daoState.getTxMap().put(tx.getId(), tx);

in DaoState.onNewTxForLastBlock.

}

@Override
Expand Down Expand Up @@ -237,6 +250,7 @@ public String toString() {
",\n paramChangeList=" + paramChangeList +
",\n evaluatedProposalList=" + evaluatedProposalList +
",\n decryptedBallotsWithMeritsList=" + decryptedBallotsWithMeritsList +
",\n txMap=" + txMap +
"\n}";
}
}