From 13a0cd06e01b24820b09a117462bb2327aa509f1 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 14:30:46 -0800 Subject: [PATCH] ProposalService::onProtectedDataRemoved signals listeners once on batch removes #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. --- .../governance/proposal/ProposalService.java | 58 ++++---- ...osalServiceP2PDataStorageListenerTest.java | 127 ++++++++++++++++++ 2 files changed, 160 insertions(+), 25 deletions(-) create mode 100644 core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java index 594ce6c10af..7953d403b66 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java @@ -50,6 +50,7 @@ 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 void onAdded(Collection protectedStorageEntries) { @Override public void onRemoved(Collection protectedStorageEntries) { - protectedStorageEntries.forEach(protectedStorageEntry -> { - onProtectedDataRemoved(protectedStorageEntry); - }); + onProtectedDataRemoved(protectedStorageEntries); } @@ -271,30 +270,39 @@ private void onProtectedDataAdded(ProtectedStorageEntry entry, boolean fromBroad } } - 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 = 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 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 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 = 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) { diff --git a/core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java b/core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java new file mode 100644 index 00000000000..7945a241b28 --- /dev/null +++ b/core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java @@ -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 . + */ + +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 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()); + } +}