Skip to content

Commit f866c80

Browse files
ejona86ejona86
committed
util: SocketAddress.toString() cannot be used for equality
Some addresses are equal even though their toString is different (InetSocketAddress ignores the hostname when it has an address). And some addresses are not equal even though their toString might be the same (AnonymousInProcessSocketAddress doesn't override toString()). InetSocketAddress/InetAddress do not cache the toString() result. Thus, even in the worst case that uses a HashSet, this should use less memory than the earlier approach, as no strings are formatted. It probably also significantly improves performance in the reasonably common case when an Endpoint is created just for looking up a key, because the string creation in the constructor isn't then amorized. updateChildrenWithResolvedAddresses(), for example, creates n^2 Endpoint objects for lookups.
1 parent 72a977b commit f866c80

File tree

3 files changed

+54
-50
lines changed

3 files changed

+54
-50
lines changed

‎util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@
3737
import io.grpc.internal.PickFirstLoadBalancerProvider;
3838
import java.net.SocketAddress;
3939
import java.util.ArrayList;
40-
import java.util.Arrays;
4140
import java.util.Collection;
4241
import java.util.Collections;
4342
import java.util.HashMap;
43+
import java.util.HashSet;
4444
import java.util.LinkedHashMap;
4545
import java.util.List;
4646
import java.util.Map;
@@ -494,25 +494,27 @@ protected Helper delegate() {
494494

495495
/**
496496
* Endpoint is an optimization to quickly lookup and compare EquivalentAddressGroup address sets.
497-
* Ignores the attributes, orders the addresses in a deterministic manner and converts each
498-
* address into a string for easy comparison. Also caches the hashcode.
499-
* Is used as a key for ChildLbState for most load balancers (ClusterManagerLB uses a String).
497+
* It ignores the attributes. Is used as a key for ChildLbState for most load balancers
498+
* (ClusterManagerLB uses a String).
500499
*/
501500
protected static class Endpoint {
502-
final String[] addrs;
501+
final Collection<SocketAddress> addrs;
503502
final int hashCode;
504503

505504
public Endpoint(EquivalentAddressGroup eag) {
506505
checkNotNull(eag, "eag");
507506

508-
addrs = new String[eag.getAddresses().size()];
509-
int i = 0;
507+
if (eag.getAddresses().size() < 10) {
508+
addrs = eag.getAddresses();
509+
} else {
510+
// This is expected to be very unlikely in practice
511+
addrs = new HashSet<>(eag.getAddresses());
512+
}
513+
int sum = 0;
510514
for (SocketAddress address : eag.getAddresses()) {
511-
addrs[i++] = address.toString();
515+
sum += address.hashCode();
512516
}
513-
Arrays.sort(addrs);
514-
515-
hashCode = Arrays.hashCode(addrs);
517+
hashCode = sum;
516518
}
517519

518520
@Override
@@ -525,24 +527,21 @@ public boolean equals(Object other) {
525527
if (this == other) {
526528
return true;
527529
}
528-
if (other == null) {
529-
return false;
530-
}
531530

532531
if (!(other instanceof Endpoint)) {
533532
return false;
534533
}
535534
Endpoint o = (Endpoint) other;
536-
if (o.hashCode != hashCode || o.addrs.length != addrs.length) {
535+
if (o.hashCode != hashCode || o.addrs.size() != addrs.size()) {
537536
return false;
538537
}
539538

540-
return Arrays.equals(o.addrs, this.addrs);
539+
return o.addrs.containsAll(addrs);
541540
}
542541

543542
@Override
544543
public String toString() {
545-
return Arrays.toString(addrs);
544+
return addrs.toString();
546545
}
547546
}
548547

‎util/src/test/java/io/grpc/util/MultiChildLoadBalancerTest.java‎

Lines changed: 23 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import static io.grpc.ConnectivityState.READY;
2222
import static io.grpc.ConnectivityState.SHUTDOWN;
2323
import static org.junit.Assert.assertEquals;
24-
import static org.junit.Assert.assertNotEquals;
2524
import static org.junit.Assert.assertTrue;
2625
import static org.junit.Assert.fail;
2726
import static org.mockito.AdditionalAnswers.delegatesTo;
@@ -34,6 +33,7 @@
3433
import static org.mockito.Mockito.verify;
3534

3635
import com.google.common.collect.Lists;
36+
import com.google.common.testing.EqualsTester;
3737
import io.grpc.Attributes;
3838
import io.grpc.ConnectivityState;
3939
import io.grpc.ConnectivityStateInfo;
@@ -244,37 +244,28 @@ public void testEndpoint_toString() {
244244

245245
@Test
246246
public void testEndpoint_equals() {
247-
assertEquals(
248-
createEndpoint(Attributes.EMPTY, "addr1"),
249-
createEndpoint(Attributes.EMPTY, "addr1"));
250-
251-
assertEquals(
252-
createEndpoint(Attributes.EMPTY, "addr1", "addr2"),
253-
createEndpoint(Attributes.EMPTY, "addr2", "addr1"));
254-
255-
assertEquals(
256-
createEndpoint(Attributes.EMPTY, "addr1", "addr2"),
257-
createEndpoint(affinity, "addr2", "addr1"));
258-
259-
assertEquals(
260-
createEndpoint(Attributes.EMPTY, "addr1", "addr2").hashCode(),
261-
createEndpoint(affinity, "addr2", "addr1").hashCode());
262-
263-
}
264-
265-
@Test
266-
public void testEndpoint_notEquals() {
267-
assertNotEquals(
268-
createEndpoint(Attributes.EMPTY, "addr1", "addr2"),
269-
createEndpoint(Attributes.EMPTY, "addr1", "addr3"));
270-
271-
assertNotEquals(
272-
createEndpoint(Attributes.EMPTY, "addr1"),
273-
createEndpoint(Attributes.EMPTY, "addr1", "addr2"));
274-
275-
assertNotEquals(
276-
createEndpoint(Attributes.EMPTY, "addr1", "addr2"),
277-
createEndpoint(Attributes.EMPTY, "addr1"));
247+
new EqualsTester()
248+
.addEqualityGroup(
249+
createEndpoint(Attributes.EMPTY, "addr1"),
250+
createEndpoint(Attributes.EMPTY, "addr1"))
251+
.addEqualityGroup(
252+
createEndpoint(Attributes.EMPTY, "addr1", "addr2"),
253+
createEndpoint(Attributes.EMPTY, "addr2", "addr1"),
254+
createEndpoint(affinity, "addr1", "addr2"))
255+
.addEqualityGroup(
256+
createEndpoint(Attributes.EMPTY, "addr1", "addr3"))
257+
.addEqualityGroup(
258+
createEndpoint(Attributes.EMPTY, "addr1", "addr2", "addr3", "addr4", "addr5", "addr6",
259+
"addr7", "addr8", "addr9", "addr10"),
260+
createEndpoint(Attributes.EMPTY, "addr2", "addr1", "addr3", "addr4", "addr5", "addr6",
261+
"addr7", "addr8", "addr9", "addr10"))
262+
.addEqualityGroup(
263+
createEndpoint(Attributes.EMPTY, "addr1", "addr2", "addr3", "addr4", "addr5", "addr6",
264+
"addr7", "addr8", "addr9", "addr11"))
265+
.addEqualityGroup(
266+
createEndpoint(Attributes.EMPTY, "addr1", "addr2", "addr3", "addr4", "addr5", "addr6",
267+
"addr7", "addr8", "addr9", "addr10", "addr11"))
268+
.testEquals();
278269
}
279270

280271
private String addressesOnlyString(EquivalentAddressGroup eag) {

‎util/src/testFixtures/java/io/grpc/util/AbstractTestHelper.java‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ public String toString() {
276276
}
277277
}
278278

279-
public static class FakeSocketAddress extends SocketAddress {
279+
public static final class FakeSocketAddress extends SocketAddress {
280280
private static final long serialVersionUID = 0L;
281281
final String name;
282282

@@ -288,6 +288,20 @@ public static class FakeSocketAddress extends SocketAddress {
288288
public String toString() {
289289
return "FakeSocketAddress-" + name;
290290
}
291+
292+
@Override
293+
public boolean equals(Object o) {
294+
if (!(o instanceof FakeSocketAddress)) {
295+
return false;
296+
}
297+
FakeSocketAddress that = (FakeSocketAddress) o;
298+
return this.name.equals(that.name);
299+
}
300+
301+
@Override
302+
public int hashCode() {
303+
return name.hashCode();
304+
}
291305
}
292306
}
293307

0 commit comments

Comments
 (0)