[bisq-network/bisq] Add transient tx map to DaoState to speed up getTx queries (#3773)

[Julian Knutsen]

julianknutsen requested changes on this pull request.

I want to preface this by saying I appreciate you jumping in and tackling one of the performance bottlenecks as one of your first projects. I think this PR will start a great discussion on the future of perf work in this layer of the code.

At the concept level, I think that caching dao state will be a requirement for performance. We see the same type of issues show up in jprofiler traces during startup as well.

But, I have reservations with the way this PR solves the problem. I think that applying performance optimizations on code that isn't tested can be extremely harmful and performance optimizations tend to create bugs that are much harder to track down.

The fact that the DAO is untested doesn't lend itself well to this type of work. I would like to know a lot more about how you tested this code.

1. What manual tests did you run and what should be added to the release cycle testing to ensure that caching bugs are found quickly?

2. Do you have any profile traces to share that motivate this particular caching work and the impact this patch has on performance? There are always tradeoffs and understanding which parts get slower (when rebuilding the cache) and which parts get faster (when using the cache) will help weight the pros and cons.


I've also added comments inline about design choices. I don't have much experience in the DAO and have been scared to touch any of the code due to the current testing and correctness requirements. I will leave the more dao-related feedback to @chimp1984 and @sqrrm.

The reality is that this code will need to change if we want performance gains. I think it would be a good idea to get everyone on the same page with what can/can't be done in this area and how much testing needs to be a prerequiste to changes.

> @@ -348,16 +360,16 @@ public Coin getGenesisTotalSupply() {
                 .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() {

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

>      }
 
     public Set<Tx> getTxs() {
-        return getTxStream().collect(Collectors.toSet());
+        return new HashSet<>(getTxMap().values());

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

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

This seems like a roundabout way to initialize the `Block` object that is error-prone. It seems easier to reason about if the `BlockParser` is responsible for initializing the block txs and having the DaoStateService call back in breaks encapsulation.

> @@ -226,7 +230,15 @@ 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(Tx tx) {
+        getLastBlock().ifPresent(block -> {
+            block.getTxs().add(tx);

I don't think that these getters should be mutable references. I would prefer to see `getTxs` return an immutable list (pending performance impact) so this type of pattern doesn't propagate. Lombok helps reduce boilerplate but is a major headache for understanding ownership.

> @@ -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

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

> @@ -176,6 +189,10 @@ public Message toProtoMessage() {
     }
 
     public static DaoState fromProto(protobuf.DaoState proto) {
+        Map<String, Tx> txMap = proto.getBlocksList().stream()

The cache initialization code doesn't need to live in the protobuf handlers. The cache is an internal optimization of the `DaoState`. By passing it in it isn't clear if that state could also live elsewhere and expands the testing breadth.

> @@ -226,7 +230,15 @@ 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(Tx tx) {
+        getLastBlock().ifPresent(block -> {

Just retrieving the last block opens up bugs when this function is no longer called in the right sequence. I would recommend passing in the block itself or at a minimum asserting that the tx is contained in the block as a defensive technique.

Isn't it also a programming error if getBlock() is not present? It seems like just doing nothing, in that case, is a mistake.

> @@ -115,6 +116,9 @@ public void applySnapshot(DaoState snapshot) {
 
         daoState.setChainHeight(snapshot.getChainHeight());
 
+        daoState.getTxMap().clear();
+        daoState.getTxMap().putAll(snapshot.getTxMap());

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.

> @@ -115,6 +116,9 @@ public void applySnapshot(DaoState snapshot) {
 
         daoState.setChainHeight(snapshot.getChainHeight());
 
+        daoState.getTxMap().clear();
+        daoState.getTxMap().putAll(snapshot.getTxMap());

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?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3773#pullrequestreview-330019725
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191210/160d281a/attachment-0001.html>