Skip to content
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.

Sign up for GitHub

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

Improve performance of ShortNameComparator. #1027

Merged
merged 4 commits into from
Dec 23, 2024
Merged

Improve performance of ShortNameComparator. #1027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a279775
Improve performance of ShortNameComparator.
Dec 19, 2024
8a1fe37
Added additional test.
Dec 19, 2024
bc8c40b
Merge branch 'main' into ImproveShortNameComparator
Dec 23, 2024
d9411f2
Changes from PR feedback.
Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 40 additions & 14 deletions src/main/java/emissary/util/ShortNameComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import emissary.core.Family;
import emissary.core.IBaseDataObject;

import org.apache.commons.lang3.Validate;

import java.io.Serializable;
import java.util.Comparator;

Expand All @@ -17,36 +19,60 @@ public class ShortNameComparator implements Comparator<IBaseDataObject>, Seriali

@Override
public int compare(IBaseDataObject obj1, IBaseDataObject obj2) {
Validate.isTrue(obj1 != null, "Required: obj1 != null");
Validate.isTrue(obj2 != null, "Required: obj2 != true");

String s1 = obj1.shortName();
String s2 = obj2.shortName();
int index1 = s1.indexOf(Family.SEP);
int index2 = s2.indexOf(Family.SEP);

String[] p1 = s1.split(Family.SEP);
String[] p2 = s2.split(Family.SEP);

for (int i = 1; i < p1.length; i++) {

if (i >= p2.length) {
// Loop through each level in obj1's shortName.
while (index1 != -1) {
// If obj1's shortName has more levels than obj2's shortName then obj1 > obj2.
if (index2 == -1) {
return 1;
}

if (p1[i].equals(p2[i])) {
// Get start character of level.
index1 += Family.SEP.length();
index2 += Family.SEP.length();

// Get character length of level.
final int nextIndex1 = s1.indexOf(Family.SEP, index1);
final int nextIndex2 = s2.indexOf(Family.SEP, index2);
final int length1 = ((nextIndex1 < 0) ? s1.length() : nextIndex1) - index1;
final int length2 = ((nextIndex2 < 0) ? s2.length() : nextIndex2) - index2;

// If the obj1's level and obj2's level are the same, then go to the next level.
if (length1 == length2 && s1.regionMatches(index1, s2, index2, length1)) {
index1 = nextIndex1;
index2 = nextIndex2;

continue;
}

// Otherwise, try comparing the levels as integers.
try {
int i1 = Integer.parseInt(p1[i]);
int i2 = Integer.parseInt(p2[i]);
return i1 - i2;
int int1 = Integer.parseInt(s1, index1, index1 + length1, 10);
int int2 = Integer.parseInt(s2, index2, index2 + length2, 10);

return int1 - int2;
} catch (NumberFormatException e) {
return p1[i].compareTo(p2[i]);
}
// Otherwise, compare the levels as strings.
final String substring1 = s1.substring(index1, index1 + length1);
final String substring2 = s2.substring(index2, index2 + length2);

return substring1.compareTo(substring2);
}
}

if (p2.length > p1.length) {
// If obj1's shortName has fewer levels than obj2's shortName then obj1 < obj2.
if (index2 != -1) {
return -1;
}

// Otherwise, obj1 and obj2 compare as equivalent
return 0;
}

}
129 changes: 62 additions & 67 deletions src/test/java/emissary/util/ShortNameComparatorTest.java
View file
Open in desktop
jdcove2 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -2,94 +2,89 @@

import emissary.core.BaseDataObject;
import emissary.core.DataObjectFactory;
import emissary.core.Family;
import emissary.core.IBaseDataObject;
import emissary.test.core.junit5.UnitTest;

import org.junit.jupiter.api.Test;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.assertIterableEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

class ShortNameComparatorTest extends UnitTest {
private final byte[] nobytes = new byte[0];
private static final String b = "foo";
private final String ba = b + Family.SEP;
// @formatter: off
private static final String ABC = "abc";
private static final String DEF = "def";
private static final String FOO_ATT_ABC = "foo-att-abc";
private static final String FOO_ATT_DEF = "foo-att-def";
private static final String FOO_ATT_1 = "foo-att-1";
private static final String FOO_ATT_2 = "foo-att-2";
private static final String FOO_ATT_1_ATT_1 = "foo-att-1-att-1";
private static final String FOO_ATT_1_ATT_2 = "foo-att-1-att-2";
private static final String FOO_ATT_10 = "foo-att-10";
private static final String FOO_ATT_10_ATT_1 = "foo-att-10-att-1";
// @formatter: on
private static final ShortNameComparator COMPARATOR = new ShortNameComparator();

@Test
void testOrdering() {
final List<IBaseDataObject> l = new ArrayList<>();
void testParameters() {
final ShortNameComparator comparator = new ShortNameComparator();
final IBaseDataObject ibdo = new BaseDataObject();

fillList(l);
l.sort(new ShortNameComparator());
checkList(l);
assertThrows(IllegalArgumentException.class, () -> comparator.compare(null, ibdo));
assertThrows(IllegalArgumentException.class, () -> comparator.compare(ibdo, null));
}

@Test
void testNonsenseNames() {
final List<IBaseDataObject> l = new ArrayList<>();
l.add(DataObjectFactory.getInstance(null, "foo-att-def"));
l.add(DataObjectFactory.getInstance(null, "foo-att-abc"));
l.sort(new ShortNameComparator());
assertEquals("foo-att-abc", l.get(0).shortName(), "Bad components sort in alpha order");
}

private void fillList(final List<IBaseDataObject> l) {
l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "1"));
l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3"));
l.add(DataObjectFactory.getInstance(this.nobytes, b));
l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3" + Family.getSep(2)));
l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3" + Family.getSep(1)));
l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "2"));
l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3" + Family.getSep(1) + Family.getSep(1)));
}

private void checkList(final List<IBaseDataObject> l) {
assertEquals(b, l.get(0).shortName(), "Ordering of sort");
assertEquals(this.ba + "1", l.get(1).shortName(), "Ordering of sort");
assertEquals(this.ba + "2", l.get(2).shortName(), "Ordering of sort");
assertEquals(this.ba + "3", l.get(3).shortName(), "Ordering of sort");
assertEquals(this.ba + "3" + Family.getSep(1), l.get(4).shortName(), "Ordering of sort");
assertEquals(this.ba + "3" + Family.getSep(1) + Family.getSep(1), l.get(5).shortName(), "Ordering of sort");
assertEquals(this.ba + "3" + Family.getSep(2), l.get(6).shortName(), "Ordering of sort");
void testComparator() {
check(ABC, DEF, 0);
check(FOO_ATT_ABC, FOO_ATT_DEF, -3);
check(FOO_ATT_1, FOO_ATT_2, -1);
check(FOO_ATT_1, FOO_ATT_10, -9);
check(FOO_ATT_1, FOO_ATT_1_ATT_1, -1);
}

@Test
void testImplComparator() {
final List<IBaseDataObject> l = new ArrayList<>();

fillList(l);
l.sort(new ShortNameComparator());
checkList(l);
}

@Test
void testSubclassedComparator() {
final String defaultPayloadClass = DataObjectFactory.getImplementingClass();
DataObjectFactory.setImplementingClass(MyDataObject.class.getName());
try {
final List<IBaseDataObject> l = new ArrayList<>();

fillList(l);
l.sort(new ShortNameComparator());
checkList(l);
} catch (RuntimeException ex) {
fail("Cannot operate Comparator in subclass", ex);
} finally {
DataObjectFactory.setImplementingClass(defaultPayloadClass);
}
void testListSorting() {
// start with an unsorted list
List<String> unsorted = List.of(
FOO_ATT_1,
FOO_ATT_10,
FOO_ATT_2,
FOO_ATT_10_ATT_1,
FOO_ATT_1_ATT_2,
FOO_ATT_1_ATT_1);

// map to a list of IBDO, sort using Comparator, map back to a list of Strings
List<String> actual = unsorted
.stream()
.map(name -> DataObjectFactory.getInstance(new byte[0], name))
.sorted(COMPARATOR)
.map(IBaseDataObject::shortName)
.collect(Collectors.toList());

// list with shortnames in expected sort order
List<String> expected = List.of(
FOO_ATT_1,
FOO_ATT_1_ATT_1,
FOO_ATT_1_ATT_2,
FOO_ATT_2,
FOO_ATT_10,
FOO_ATT_10_ATT_1);

// verify that the sorted output is correct
assertIterableEquals(expected, actual, "List wasn't sorted in the expected order");
}

// An extension of BaseDataObject for testing the
// lower bounded generics on the comparator
public static class MyDataObject extends BaseDataObject {
static final long serialVersionUID = 7872122417333007868L;
private static void check(final String name1, final String name2, final int forwardResult) {
final IBaseDataObject ibdo1 = new BaseDataObject(null, name1);
final IBaseDataObject ibdo2 = new BaseDataObject(null, name2);
final ShortNameComparator snc = new ShortNameComparator();

public MyDataObject(final byte[] data, final String name) {
super(data, name);
}
assertEquals(forwardResult, snc.compare(ibdo1, ibdo2));
assertEquals(-forwardResult, snc.compare(ibdo2, ibdo1));
}
}
Loading