(2/6) Remove onBatch interfaces in P2PDataStore #3621
Merged
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
julianknutsen
force-pushed
the
batch-listener
branch
2 times, most recently
from
b008b26
to
d1ab3c0
Compare
julianknutsen
changed the title
Commit 833611f is also part of this PR #3628 (review) |
ripcurlx
reviewed
Nov 19, 2019
ripcurlx
reviewed
Nov 19, 2019
Instead of using a subclass that overwrites a value, utilize Guice to inject the real value of 10000 in the app and let the tests overwrite it with their own.
Remove unused imports and clean up some access modifiers now that the final test structure is complete
Previously, this interface was called each time an item was changed. This required listeners to understand performance implications of multiple adds or removes in a short time span. Instead, give each listener the ability to process a list of added or removed entrys which can help them avoid performance issues. This patch is just a refactor. Each listener is called once for each ProtectedStorageEntry. Future patches will change this.
Minor performance overhead for constructing MapEntry and Collections of one element, but keeps the code cleaner and all removes can still use the same logic to remove from map, delete from data store, signal listeners, etc. The MapEntry type is used instead of Pair since it will require less operations when this is eventually used in the removeExpiredEntries path.
…batch All current users still call this one-at-a-time. But, it gives the ability for the expire code path to remove in a batch.
This will cause HashMapChangedListeners to receive just one onRemoved() call for the expire work instead of multiple onRemoved() calls for each item. This required a bit of updating for the remove validation in tests so that it correctly compares onRemoved with multiple items.
…ch removes bisq-network#3143 identified an issue that tempProposals listeners were being signaled once for each item that was removed during the P2PDataStore operation that expired old TempProposal objects. Some of the listeners are very expensive (ProposalListPresentation::updateLists()) which results in large UI performance issues. Now that the infrastructure is in place to receive updates from the P2PDataStore in a batch, the ProposalService can apply all of the removes received from the P2PDataStore at once. This results in only 1 onChanged() callback for each listener. The end result is that updateLists() is only called once and the performance problems are reduced. This removes the need for bisq-network#3148 and those interfaces will be removed in the next patch.
Now that the only user of this interface has been removed, go ahead and delete it. This is a partial revert of f5d75c4 that includes the code that was added into ProposalService that subscribed to the P2PDataStore.
julianknutsen
force-pushed
the
batch-listener
branch
from
d1ab3c0
to
a8139f3
Compare
julianknutsen
changed the title
julianknutsen
changed the title
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.
Patches start 78211a6
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 callingupdateLists()less often. There is still an outstanding bug thatupdateLists()can take seconds to run on each invocation.Initial Bug #3143
If the
P2PDataStoreremoves multiple items during the expiration work, each item is signaled separately. TheProposalServiceis a listener for these removes and would eventually callupdateLists()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
P2PDataStorethat would signal when the expire code starts and ends. TheProposalListPresentationwould subscribe to this interface and remove its listener from the ProposalService so that all of the extraonRemoved()calls would not triggerupdateLists().The code did remove the proposal listener when the expire code started, but it had a bug where it would ALWAYS call
updateLists()when the expire code finished if ANYTempProposalobject was received (not necessarily a new one).In the pre-fix code, the
ProposalListPresentationcode only calledupdateList()when theProposalServiceproposal list changed. TheProposalServicehad validation to ensure that it only signaled listeners if theProposalwas new. By going around theProposalServiceand directly listening to theP2PDataStore, the new code was running every time an existingProposalobject was seen.Duplicate
Proposalobjects (same payload different sequence number) can be added during startup, onrestart(), or republished on startup by ownersMyProposalListService::rePublishMyProposalsOnceWellConnected(). The effect is that the next call to the expiration code would causeupdateLists()to run when it didn't need to.Fix
The root of the original performance issue is straightforward. Don't update listeners of the
P2PDataStoreor theProposalServiceon each remove when batching them is possible.This pull request implements batching by:
onAdded()andonRemoved()listener functions to take aCollectionofProtectedStorageEntryobjects instead of just one.remove()operations done inP2PDataStore::removeExpiredEntries()to a singleonRemoved()call that includes all of the removedProtectedStorageEntryobjects.ProposalServiceto only update its listeners once for eachonRemoved()call.onBatchlistener changesThe end result is that any number of removes from
P2PDataStore::removeExpiredEntries()will result in 1 call toProposalListPresentation::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.