mirrors/bisq
1
0
Fork 0
mirror of https://github.com/bisq-network/bisq.git synced 2024-11-20 02:12:00 +01:00
Code Issues Packages Projects Releases Wiki Activity

ProposalService::onProtectedDataRemoved signals listeners once on batch removes

Browse Source 
#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 #3148 and those interfaces will be removed in
the next patch.
This commit is contained in:
Julian Knutsen 2019-11-16 14:30:46 -08:00
parent eae641ee73
commit a50e59f7eb
No known key found for this signature in database
GPG Key ID: D85F536DB3615B2D
2 changed files with 160 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java
View File

@ -50,6 +50,7 @@ import javax.inject.Named;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
@ -142,9 +143,7 @@ public class ProposalService implements HashMapChangedListener, AppendOnlyDataSt
@Override
public void onRemoved(Collection<ProtectedStorageEntry> protectedStorageEntries) {
protectedStorageEntries.forEach(protectedStorageEntry -> {
onProtectedDataRemoved(protectedStorageEntry);
});
onProtectedDataRemoved(protectedStorageEntries);
}
@ -271,30 +270,39 @@ public class ProposalService implements HashMapChangedListener, AppendOnlyDataSt
}
}
private void onProtectedDataRemoved(ProtectedStorageEntry entry) {
ProtectedStoragePayload protectedStoragePayload = entry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof TempProposalPayload) {
Proposal proposal = ((TempProposalPayload) protectedStoragePayload).getProposal();
// We allow removal only if we are in the proposal phase.
boolean inPhase = periodService.isInPhase(daoStateService.getChainHeight(), DaoPhase.Phase.PROPOSAL);
boolean txInPastCycle = periodService.isTxInPastCycle(proposal.getTxId(), daoStateService.getChainHeight());
Optional<Tx> tx = daoStateService.getTx(proposal.getTxId());
boolean unconfirmedOrNonBsqTx = !tx.isPresent();
// if the tx is unconfirmed we need to be in the PROPOSAL phase, otherwise the tx must be confirmed.
if (inPhase || txInPastCycle || unconfirmedOrNonBsqTx) {
if (tempProposals.contains(proposal)) {
tempProposals.remove(proposal);
log.debug("We received a remove request for a TempProposalPayload and have removed the proposal " +
"from our list. proposal creation date={}, proposalTxId={}, inPhase={}, " +
"txInPastCycle={}, unconfirmedOrNonBsqTx={}",
proposal.getCreationDateAsDate(), proposal.getTxId(), inPhase, txInPastCycle, unconfirmedOrNonBsqTx);
private void onProtectedDataRemoved(Collection<ProtectedStorageEntry> protectedStorageEntries) {
// The listeners of tmpProposals can do large amounts of work that cause performance issues. Apply all of the
// updates at once using retainAll which will cause all listeners to be updated only once.
ArrayList<Proposal> tempProposalsWithUpdates = new ArrayList<>(tempProposals);
protectedStorageEntries.forEach(protectedStorageEntry -> {
ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof TempProposalPayload) {
Proposal proposal = ((TempProposalPayload) protectedStoragePayload).getProposal();
// We allow removal only if we are in the proposal phase.
boolean inPhase = periodService.isInPhase(daoStateService.getChainHeight(), DaoPhase.Phase.PROPOSAL);
boolean txInPastCycle = periodService.isTxInPastCycle(proposal.getTxId(), daoStateService.getChainHeight());
Optional<Tx> tx = daoStateService.getTx(proposal.getTxId());
boolean unconfirmedOrNonBsqTx = !tx.isPresent();
// if the tx is unconfirmed we need to be in the PROPOSAL phase, otherwise the tx must be confirmed.
if (inPhase || txInPastCycle || unconfirmedOrNonBsqTx) {
if (tempProposalsWithUpdates.contains(proposal)) {
tempProposalsWithUpdates.remove(proposal);
log.debug("We received a remove request for a TempProposalPayload and have removed the proposal " +
"from our list. proposal creation date={}, proposalTxId={}, inPhase={}, " +
"txInPastCycle={}, unconfirmedOrNonBsqTx={}",
proposal.getCreationDateAsDate(), proposal.getTxId(), inPhase, txInPastCycle, unconfirmedOrNonBsqTx);
}
} else {
log.warn("We received a remove request outside the PROPOSAL phase. " +
"Proposal creation date={}, proposal.txId={}, current blockHeight={}",
proposal.getCreationDateAsDate(), proposal.getTxId(), daoStateService.getChainHeight());
}
} else {
log.warn("We received a remove request outside the PROPOSAL phase. " +
"Proposal creation date={}, proposal.txId={}, current blockHeight={}",
proposal.getCreationDateAsDate(), proposal.getTxId(), daoStateService.getChainHeight());
}
}
});
tempProposals.retainAll(tempProposalsWithUpdates);
}
private void onAppendOnlyDataAdded(PersistableNetworkPayload persistableNetworkPayload, boolean fromBroadcastMessage) {

127
core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java Normal file
View File

@ -0,0 +1,127 @@
/*
* This file is part of Bisq.
*
* Bisq is free software: you can redistribute it and/or modify it
* under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or (at
* your option) any later version.
*
* Bisq is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
* License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with Bisq. If not, see <http://www.gnu.org/licenses/>.
*/
package bisq.core.dao.governance.proposal;
import bisq.core.dao.governance.period.PeriodService;
import bisq.core.dao.governance.proposal.storage.appendonly.ProposalStorageService;
import bisq.core.dao.governance.proposal.storage.temp.TempProposalPayload;
import bisq.core.dao.governance.proposal.storage.temp.TempProposalStorageService;
import bisq.core.dao.state.DaoStateService;
import bisq.core.dao.state.model.governance.DaoPhase;
import bisq.core.dao.state.model.governance.Proposal;
import bisq.network.p2p.P2PService;
import bisq.network.p2p.storage.payload.ProtectedStorageEntry;
import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreService;
import bisq.network.p2p.storage.persistence.ProtectedDataStoreService;
import javafx.collections.ListChangeListener;
import java.util.Arrays;
import java.util.Collections;
import org.junit.Before;
import org.junit.Test;
import static org.mockito.Mockito.*;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
/**
* Tests of the P2PDataStorage::onRemoved callback behavior to ensure that the proper number of signal events occur.
*/
public class ProposalServiceP2PDataStorageListenerTest {
private ProposalService proposalService;
@Mock
private PeriodService periodService;
@Mock
private DaoStateService daoStateService;
@Mock
private ListChangeListener<Proposal> tempProposalListener;
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
this.proposalService = new ProposalService(
mock(P2PService.class),
this.periodService,
mock(ProposalStorageService.class),
mock(TempProposalStorageService.class),
mock(AppendOnlyDataStoreService.class),
mock(ProtectedDataStoreService.class),
this.daoStateService,
mock(ProposalValidatorProvider.class),
true);
// Create a state so that all added/removed Proposals will actually update the tempProposals list.
when(this.periodService.isInPhase(anyInt(), any(DaoPhase.Phase.class))).thenReturn(true);
when(this.daoStateService.isParseBlockChainComplete()).thenReturn(false);
}
private static ProtectedStorageEntry buildProtectedStorageEntry() {
ProtectedStorageEntry protectedStorageEntry = mock(ProtectedStorageEntry.class);
TempProposalPayload tempProposalPayload = mock(TempProposalPayload.class);
Proposal tempProposal = mock(Proposal.class);
when(protectedStorageEntry.getProtectedStoragePayload()).thenReturn(tempProposalPayload);
when(tempProposalPayload.getProposal()).thenReturn(tempProposal);
return protectedStorageEntry;
}
// TESTCASE: If an onRemoved callback is called which does not remove anything the tempProposals listeners
// are not signaled.
@Test
public void onRemoved_noSignalIfNoChange() {
this.proposalService.onRemoved(Collections.singletonList(mock(ProtectedStorageEntry.class)));
verify(this.tempProposalListener, never()).onChanged(any());
}
// TESTCASE: If an onRemoved callback is called with 1 element AND it creates a remove of 1 element, the tempProposal
// listeners are signaled once.
@Test
public void onRemoved_signalOnceOnOneChange() {
ProtectedStorageEntry one = buildProtectedStorageEntry();
this.proposalService.onAdded(Collections.singletonList(one));
this.proposalService.getTempProposals().addListener(this.tempProposalListener);
this.proposalService.onRemoved(Collections.singletonList(one));
verify(this.tempProposalListener).onChanged(any());
}
// TESTCASE: If an onRemoved callback is called with 2 elements AND it creates a remove of 2 elements, the
// tempProposal listeners are signaled once.
@Test
public void onRemoved_signalOnceOnMultipleChanges() {
ProtectedStorageEntry one = buildProtectedStorageEntry();
ProtectedStorageEntry two = buildProtectedStorageEntry();
this.proposalService.onAdded(Arrays.asList(one, two));
this.proposalService.getTempProposals().addListener(this.tempProposalListener);
this.proposalService.onRemoved(Arrays.asList(one, two));
verify(this.tempProposalListener).onChanged(any());
}
}