Speed up bond repository update and remove some raw types #7082
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use the simpler & slightly more efficient 'Map::computeIfAbsent' method in place of the common pattern: map.putIfAbsent(key, newValue()); V value = map.get(); (Clean up BondRepository + some cases missed from BurningManService.)
Replace all raw uses of 'Bond<T extends BondedAsset>', mostly with wildcards (that is, 'Bond<?>'), to prevent compiler/IDE warnings. Also rename the 'T extends Bond<R>' & 'R extend BondedAsset' type params of 'BondRepository<..>' to 'B' & 'T' respectively, as this is a little less confusing.
Fix raw usage of the following types, all of which (apart from Comparator) touch the DAO packages somewhere: Comparable, Comparator, GetStateHashesResponse, NewStateHashMessage, RequestStateHashesHandler, PersistenceManager (Also replace 'Integer.valueOf' with the non-boxing but otherwise identical method 'Integer.parseInt', in the class 'TxOutputKey'.)
Fix the broken stubbing of 'PersistenceManager', which had gone stale as a result of the conversion of 'Preferences' to asynchronous persistence in commit 3f4d6e6 (2020/10/12). This caused the assertions in the 'readPersisted' continuation blocks of 3 of the 4 tests not to be reached. Fix by stubbing the async 'persistenceManager::readPersisted' method with a callback, instead of stubbing 'getPersisted'. NOTE: Alternatively, we could add a testing-only 'readPersistedSync' method to 'Preferences' for consistency, as this is how the other broken (failing) tests resulting from 3f4d6e6 were fixed (in commit 68583d8).
This is in anticipation of speedups we wish to make, as JProfiler reveals it to be a hotspot during new block arrivals (which are tricky to profile, as they occur at random).
Alleviate a cubic time bug during the update of the bonded roles repository, reducing it to quadratic running time. On Mainnet, this gives a roughly ten-fold speedup and should allow better scaling in the event that many new bonded roles are added. Replace calls to 'BondedRolesRepository.findBondedAssetByHash' with a lookup into a lazily initialised map of bonded assets (Roles) by hash (reset at the start of each call to 'BondRepository.update()' to prevent stale caching). This avoids rescanning the roles list for every pair of roles and lockup tx outputs, thus reducing the number of steps (to highest order) from: #roles * #roles * #lockup-tx-outputs to: #roles * #lockup-tx-outputs (The logs show 2 or 3 calls to 'BondedRepository.update()' every time a new block arrives, and while this was only taking around a second or so on Mainnet, it could potentially grow to something problematic with cubic scaling in the number of bonded roles.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alleviate a cubic time bug during the update of the bonded roles repository, reducing it to quadratic running time. On Mainnet, this gives a roughly ten-fold speedup and should allow better scaling in the event that many new bonded roles are added.
Replace calls to
BondedRolesRepository.findBondedAssetByHashwith a lookup into a lazily initialised map of bonded assets (Roles) by hash (reset at the start of each call toBondRepository.update()to prevent stale caching). This avoids rescanning the roles list for every pair of roles and lockup tx outputs, thus reducing the number of steps (to highest order) from:to:
(The logs show 2 or 3 calls to
BondedRepository.update()every time a new block arrives, and while this was only taking around a second or so on Mainnet, it could, I think, potentially grow to something problematic with cubic scaling in the number of bonded roles.)--
In addition to the above optimisation, this PR also generifies the raw types which involve the DAO classes (mainly
Bondoccurrences) and fixes a broken test encountered during that cleanup.The code changes touch the DAO packages, though not in a way that should cause any functional change.
--
Before the optimisation (last commit in the PR), the following update times are seen in the logs on my laptop:
(The first very short update time of 5ms occurs when the Bisq instance is starting up and the DAO state has not yet been loaded.)
After the optimisation, the following update times are seen in the logs:
Thus, there is something around a ten-fold speedup. As can be seen from the logs,
update()usually gets called 3 times per incoming block (though this may depend on whether accounting is enabled, which it was on my instance).