-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor detectMultipleHolderNames for efficient use of RAM. #7008
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Good catch! The toString()
comparison is indeed bad.
I saw that you're using a AtomicLong
s to count the item. Does this imply that detectMultipleHolderNames()
is called from multiple threads? If that's the case suspiciousDisputesByTraderMap
shouldn't be a HashMap
.
Otherwise the following change is simpler and faster:
diff --git a/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java b/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java
index 1be96332ef..09c722eed5 100644
--- a/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java
+++ b/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java
@@ -146,7 +146,10 @@ public class MultipleHolderNameDetection {
///////////////////////////////////////////////////////////////////////////////////////////
public void detectMultipleHolderNames() {
- String previous = suspiciousDisputesByTraderMap.toString();
+ int previous = suspiciousDisputesByTraderMap.values().stream()
+ .mapToInt(List::size)
+ .sum();
+
getAllDisputesByTraderMap().forEach((key, value) -> {
Set<String> userNames = value.stream()
.map(dispute -> {
@@ -161,8 +164,12 @@ public class MultipleHolderNameDetection {
suspiciousDisputesByTraderMap.put(key, value);
}
});
- String updated = suspiciousDisputesByTraderMap.toString();
- if (!previous.equals(updated)) {
+
+ int updated = suspiciousDisputesByTraderMap.values().stream()
+ .mapToInt(List::size)
+ .sum();
+
+ if (previous != updated) {
listeners.forEach(Listener::onSuspiciousDisputeDetected);
}
}
Fixes issue #7006 (Out Of Memory Exception thrown)
Review change to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Fixes #7006 (Out Of Memory Exception thrown)
Purpose of the method is to (a) maintain a list of flagged disputes, and (b) notify listeners if new items were added.
Using
toString()
on a large map of String to List caused an OOM exception.For (b) notifying listeners it is only the change in flagged dispute count that is relevant (so the UI can display an alert icon). The same effect can be obtained by counting identified records.
Verification
User opens a dispute using a payment account that has a different holder name than in a previous dispute. Mediator clicks on the alert icon.