[bisq-network/bisq] [WIP] Remove onBatch interfaces in P2PDataStore (#3621)
Julian Knutsen
notifications at github.com
Sun Nov 17 01:50:22 UTC 2019
**_[WIP going to spend some time on testing the code I changed in ProposalService to see how hard it is, but I wanted to get this out there for early review of interested people]_**
Patches start 4e24a2d
Before:
After:
A bug was introduced in #3148 where ProposalListPresentation would incorrectly call `updateLists()` when the proposal state didn't change. This pull request should help #3143 (the target of #3148) in a more maintainable way that removes the bug and UI performance issues by calling `updateLists()` less often. There is still an outstanding bug that `updateLists()` can take **_seconds_** to run on each invocation.
**Initial Bug #3143**
If the `P2PDataStore` removes multiple items during the expiration work, each item is signaled separately. The `ProposalService` is a listener for these removes and would eventually call `updateLists()` for each remove.
__Exacerbating Issue__
Old TempProposals were not correctly deleted from disk. They would be recreated at startup and the expire code would always remove them. This was fixed with #3150.
**Follow Up Bug #3148**
The fix for the initial bug was to create a new listener interface on P2PDataStore that would signal when the expire code starts and ends. The `ProposalListPresentation` would subscribe to this interface and remove its listener from the ProposalService so that all of the extra `onRemoved()` calls would not trigger `updateLists()`.
The code did remove the listener from when the expire code started, but it had a bug where it would **_ALWAYS_** call `updateLists()` when the expire code finished if **_ANY_** `TempProposal` object was received (not necessarily a new one).
In the pre-fix code, the `ProposalListPresentation` code only called `updateList()` when the `ProposalService` proposal list changed. The `ProposalService` had validation to ensure that it only signaled listeners if the `Proposal` was new. By going around the `ProposalService` and directly listening to the P2PDataStore, the new code was running every time an existing `Proposal` object was seen.
Duplicate `Proposal` objects (same payload different sequence number) can be added during startup, on `restart()`, or republished on startup by owners (see MyProposalListService::rePublishMyProposalsOnceWellConnected()). The effect is that the next call to the expiration code would cause `updateLists()` to run when it didn't need to.
**Fix**
The root of the original performance issue is straightforward. Don't update listeners of the P2PDataStore or the ProposalService on each remove when bundling them is possible.
This pull request implements batching by:
- Modifying the `onAdded()` and `onRemoved()` listener functions to take a collection of `ProtectedStorageEntry` objects instead of just one.
- Combining all `remove()` operations done in `P2PDataStore::removeExpiredEntries()` to a single `onRemoved()` call that includes all of the removed `ProtectedStorageEntry` objects.
- Using this new behavior in `ProposalService` to only update its listeners once for each `onRemoved()` call.
- Reverting the `onBatch` listener changes
The end result is that any number of removes from `P2PDataStore::removeExpiredEntries()` will result in 1 call to `ProposalListPresentation::updateLists()` which was the original intention of the fix.
It also cleans up one of the existing code smells for the P2PDataStorage layer that requires listeners to understand if and when `removeExpiredEntries()` is called and if they should do something special.
You can view, comment on, or merge this pull request online at:
https://github.com/bisq-network/bisq/pull/3621
-- Commit Summary --
* [BUGFIX] Don't try and remove() if addMailboxData() fails
* [REFACTOR] P2PDataStorage::addPersistableNetworkPayload()
* [REFACTOR] P2PDataStorage::addProtectedStorageEntry()
* [REFACTOR] P2PDataStorage::refreshTTL()
* [REFACTOR] P2PDataStorage::remove()
* [REFACTOR] P2PDataStorage::removeMailboxData()
* [PR COMMENTS] Clean up logging messages
* Update behavior of P2PDataStorage::addProtectedStorageEntry() on duplicates
* Update behavior of P2PDataStorage::refreshTTL() on duplicates
* Update behavior of P2PDataStorage::remove() & removeMailboxData() on duplicate sequence #s
* Use dependency injected Clock in P2PDataStore
* [REFACTOR] Clean up ProtectedStorageEntry ctor
* Use Clock in ProtectedStorageEntry
* [TESTS] Use ClockFake in tests to control time
* [TESTS] Test onBootstrapComplete()
* [TESTS] Lower entries required for purge in tests
* [REFACTOR] Remove duplicated code in refreshTTL
* [TESTS] Add more tests around mis-wrapped ProtectedStorageEntrys
* [REFACTOR] ProtectedStorageEntry::validForAddOperation()
* [REFACTOR] ProtectedStorageEntry::isValidForRemoveOperation()
* [REFACTOR] Remove checkPublicKeys()
* Clean up toString() methods
* [REFACTOR] Move signature validation behind isValidForAddOperation()
* [REFACTOR] Move signature validation behind isValidForRemoveOperation()
* [REFACTOR] Move receiversPubKey check behind isValidForRemoveOperation()
* Remove duplicate check in refreshTTL
* Introduce isMetadataEquals and use it
* [TESTS] Update remove validation with BroadcastMessage type
* Combine remove() and removeMailboxData()
* [PR COMMENTS] Logging format and function rename
* [TESTS] Start deprecating integrations tests
* [TESTS] Remove dead tests and code
* [DEAD CODE] Remove unused functions and imports
* [DEAD CODE] Remove obsolete tests
* [TESTS] Add JavaDocs for test objects
* [TESTS] Split monolithic P2PDataStoreTest into separate files
* [TESTS] Clean up TestState static methods
* [TESTS] Make SavedTestState ctor private to TestState
* [TESTS] Add missing license text to test files
* [TESTS] Rename P2PDataStoreTest
* Add payload safety checks in ProtectedStorageEntry
* [REFACTOR] Clean up removeExpiredEntries
* [BUGFIX] Correctly remove PersistablePayload in onDisconnect path
* [REFACTOR] Use common path for updating map/data store on remove
* [BUGFIX] Validate Entry.receiversPubKey for MailboxPayloads
* [PR COMMENTS] Make maxSequenceNumberBeforePurge final
* [REFACTOR] HashMapListener::onAdded/onRemoved
* [REFACTOR] removeFromMapAndDataStore can operate on Collections
* Change removeFromMapAndDataStore to signal listeners at the end in a batch
* Update removeExpiredEntries to remove all items in a batch
* ProposalService::onProtectedDataRemoved signals listeners once on batch removes
* Remove HashmapChangedListener::onBatch operations
-- File Changes --
M common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java (4)
M core/src/main/java/bisq/core/alert/AlertManager.java (32)
M core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java (49)
M core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java (65)
M core/src/main/java/bisq/core/filter/FilterManager.java (31)
M core/src/main/java/bisq/core/offer/OfferBookService.java (37)
M core/src/main/java/bisq/core/proto/CoreProtoResolver.java (6)
M core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java (5)
M core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java (23)
M monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java (6)
M monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java (4)
M p2p/src/main/java/bisq/network/p2p/P2PModule.java (1)
M p2p/src/main/java/bisq/network/p2p/P2PService.java (23)
M p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java (14)
M p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java (500)
M p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java (168)
M p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java (189)
M p2p/src/test/java/bisq/network/p2p/TestUtils.java (5)
D p2p/src/test/java/bisq/network/p2p/storage/ObsoleteP2PDataStorageTest.java (217)
A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java (242)
A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageOnMessageHandlerTest.java (101)
A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java (195)
A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java (630)
A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java (202)
D p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java (1526)
A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java (203)
D p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java (229)
A p2p/src/test/java/bisq/network/p2p/storage/TestState.java (393)
M p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java (4)
M p2p/src/test/java/bisq/network/p2p/storage/mocks/AppendOnlyDataStoreServiceFake.java (6)
A p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java (54)
M p2p/src/test/java/bisq/network/p2p/storage/mocks/DateTolerantPayloadStub.java (7)
A p2p/src/test/java/bisq/network/p2p/storage/mocks/ExpirableProtectedStoragePayloadStub.java (59)
A p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableExpirableProtectedStoragePayloadStub.java (40)
M p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java (7)
M p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedDataStoreServiceFake.java (6)
M p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java (10)
A p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java (199)
A p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java (252)
-- Patch Links --
https://github.com/bisq-network/bisq/pull/3621.patch
https://github.com/bisq-network/bisq/pull/3621.diff
--
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/3621
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191116/9c8b94e8/attachment-0001.html>
More information about the bisq-github
mailing list