Skip to content

Commit de2dc0d

Browse files
eengleMichael Bolin
authored andcommitted
Per discussion with kirilll, some checks that we desire to run at debug time only have been converted to asserts, and commented out pending an answer from the java libraries team on how best to handle debug time only checks. One of these changes overlaps a change started by ikaushan in CL 25025671 -- hopefully he beats me to the punch.
S2EdgeIndex incorrectly copied the S2 Minimum Width metric, rather than referring to the metric in S2Projections. This change should have no effect, except in the case that the projection in S2 is changed, in which case it may work fine now, but would certainly have been incredibly wrong before. Vital performance fixes: - Several classes and methods are now final, to avoid subclasses tampering with them and/or to allow the jvm to apply additional runtime optimizations. - Fixed performance of S2PolygonBuilder#assembleLoop(), which was removing N vertices from the start of an ArrayList *one at a time*, by calling ArrayList.remove(0) N times. Which of course takes N*list.size() steps, aka very quadratic performance in the common case of many loops in the builder. Instead we now call list.subList(n, list.size()), to just skip past the vertices we don't care about. - Fixed S2Polygon.S2PolygonIndex creating a copy of the whole array of vertices in *every call* to edgeFromTo(), because S2Loop's copy constructor was being used, for no apparent reason, which clones the vertices. Since edgeFromTo() is called twice per vertex to create the polygon index, a polygon with N vertices was inadverdantly copying 2*N^2 vertices to build the polygon index. - Fixed performance of S2Polygon#clipBoundary(), which was testing every vertex of a polygon b for containment in polygon a, only to emit a log message that essentially never occurs. In C++, this check is a debug time only check, so the pattern has been followed of making this a commented-out assertion instead of doing this extremely expensive check in production code. Also greatly simplified the addition of edges to the S2PolygonBuilder by relying on the checks already being done in the builder. - Fixed performance of S2Loop#vertex(int), which was doing a check before indexing into the vertices array, which itself does a range check. Since we don't need or want want both checks, the array dereference exception is caught and rethrown so the external behavior of the method is unchanged, but it is now significantly faster. - Fixed performance of S2Polygon.S2LoopSequenceIndex, which was suffering from using List<Integer>. By using int[] instead, we save extra method calls and a lot of boxing/unboxing in this performance critical area. - S2EdgeIndex now stores cells and edges more efficiently in parallel arrays, and edge lookups have been made significantly faster by a local implementation of binary search. - S2EdgeIndex methods getEdgesInParentCells and getEdgesInChildrenCells now take a Set instead of a List that is then filtered through a set. This saves an extra copy of all the edges, and keeps us from accumulating redundant copies of the same edge as the search progresses. ------------- Created by MOE: http://code.google.com/p/moe-java MOE_MIGRATED_REVID=25397727
1 parent a497b82 commit de2dc0d

4 files changed

Lines changed: 199 additions & 191 deletions

File tree

src/com/google/common/geometry/S2EdgeIndex.java

Lines changed: 116 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@
2121
import com.google.common.collect.Sets;
2222

2323
import java.util.ArrayList;
24-
import java.util.Collections;
24+
import java.util.Arrays;
25+
import java.util.Comparator;
2526
import java.util.HashSet;
2627
import java.util.List;
2728
import java.util.Set;
@@ -40,41 +41,16 @@ public abstract strictfp class S2EdgeIndex {
4041
private static final double MAX_DET_ERROR = 1e-14;
4142

4243
/**
43-
* Associates each edge index with the cell ID that contains it. Implements
44-
* Comparable to sort by the cell ID.
44+
* The cell containing each edge, as given in the parallel array
45+
* <code>edges</code>.
4546
*/
46-
private static final class Entry implements Comparable<Entry> {
47-
/** The cell ID of this indexed edge. */
48-
public final long cellId;
49-
/** The edge index of this edge. */
50-
public final int edgeIndex;
51-
/** Construct a new entry with the given cell ID and edge index. */
52-
public Entry(long cellId, int edgeIndex) {
53-
this.cellId = cellId;
54-
this.edgeIndex = edgeIndex;
55-
}
56-
/** Compares Entry instances by cell id and edge index. */
57-
@Override
58-
public int compareTo(Entry that) {
59-
if (this.cellId < that.cellId) {
60-
return -1;
61-
} else if (this.cellId > that.cellId) {
62-
return 1;
63-
} else if (this.edgeIndex < that.edgeIndex) {
64-
return -1;
65-
} else if (this.edgeIndex > that.edgeIndex) {
66-
return 1;
67-
} else {
68-
return 0;
69-
}
70-
}
71-
}
47+
private long[] cells;
7248

7349
/**
74-
* Maps cell ids to covered edges; has the property that the set of all cell
75-
* ids mapping to a particular edge forms a covering of that edge.
50+
* The edge contained by each cell, as given in the parallel array
51+
* <code>cells</code>.
7652
*/
77-
private final ArrayList<Entry> sortedMapping = Lists.newArrayList();
53+
private int[] edges;
7854

7955
/**
8056
* No cell strictly below this level appears in mapping. Initially leaf level,
@@ -99,44 +75,93 @@ public void reset() {
9975
minimumS2LevelUsed = S2CellId.MAX_LEVEL;
10076
indexComputed = false;
10177
queryCount = 0;
102-
sortedMapping.clear();
78+
cells = null;
79+
edges = null;
10380
}
10481

105-
10682
/**
107-
* Computes the index (if it has not been previously done).
83+
* Compares [cell1, edge1] to [cell2, edge2], by cell first and edge second.
84+
*
85+
* @return -1 if [cell1, edge1] is less than [cell2, edge2], 1 if [cell1,
86+
* edge1] is greater than [cell2, edge2], 0 otherwise.
10887
*/
109-
public void computeIndex() {
88+
private static final int compare(long cell1, int edge1, long cell2, int edge2) {
89+
if (cell1 < cell2) {
90+
return -1;
91+
} else if (cell1 > cell2) {
92+
return 1;
93+
} else if (edge1 < edge2) {
94+
return -1;
95+
} else if (edge1 > edge2) {
96+
return 1;
97+
} else {
98+
return 0;
99+
}
100+
}
101+
102+
/** Computes the index (if it has not been previously done). */
103+
public final void computeIndex() {
110104
if (indexComputed) {
111105
return;
112106
}
113-
114-
sortedMapping.clear();
107+
List<Long> cellList = Lists.newArrayList();
108+
List<Integer> edgeList = Lists.newArrayList();
115109
for (int i = 0; i < getNumEdges(); ++i) {
116110
S2Point from = edgeFrom(i);
117111
S2Point to = edgeTo(i);
118112
ArrayList<S2CellId> cover = Lists.newArrayList();
119113
int level = getCovering(from, to, true, cover);
120114
minimumS2LevelUsed = Math.min(minimumS2LevelUsed, level);
121115
for (S2CellId cellId : cover) {
122-
sortedMapping.add(new Entry(cellId.id(), i));
116+
cellList.add(cellId.id());
117+
edgeList.add(i);
123118
}
124119
}
125-
126-
sortedMapping.trimToSize();
127-
Collections.sort(sortedMapping);
120+
cells = new long[cellList.size()];
121+
edges = new int[edgeList.size()];
122+
for (int i = 0; i < cells.length; i++) {
123+
cells[i] = cellList.get(i);
124+
edges[i] = edgeList.get(i);
125+
}
126+
sortIndex();
128127
indexComputed = true;
129128
}
130129

131-
public boolean isIndexComputed() {
130+
/** Sorts the parallel <code>cells</code> and <code>edges</code> arrays. */
131+
private void sortIndex() {
132+
// create an array of indices and sort based on the values in the parallel
133+
// arrays at each index
134+
Integer[] indices = new Integer[cells.length];
135+
for (int i = 0; i < indices.length; i++) {
136+
indices[i] = i;
137+
}
138+
Arrays.sort(indices, new Comparator<Integer>() {
139+
@Override
140+
public int compare(Integer index1, Integer index2) {
141+
return S2EdgeIndex.compare(cells[index1], edges[index1], cells[index2], edges[index2]);
142+
}
143+
});
144+
// copy the cells and edges in the order given by the sorted list of indices
145+
long[] newCells = new long[cells.length];
146+
int[] newEdges = new int[edges.length];
147+
for (int i = 0; i < indices.length; i++) {
148+
newCells[i] = cells[indices[i]];
149+
newEdges[i] = edges[indices[i]];
150+
}
151+
// replace the cells and edges with the sorted arrays
152+
cells = newCells;
153+
edges = newEdges;
154+
}
155+
156+
public final boolean isIndexComputed() {
132157
return indexComputed;
133158
}
134159

135160
/**
136161
* Tell the index that we just received a new request for candidates. Useful
137162
* to compute when to switch to quad tree.
138163
*/
139-
protected void incrementQueryCount() {
164+
protected final void incrementQueryCount() {
140165
++queryCount;
141166
}
142167

@@ -176,7 +201,7 @@ protected void incrementQueryCount() {
176201
* while the marginal cost to find is 3ms. Thus, this is a reasonable thing to
177202
* do.
178203
*/
179-
public void predictAdditionalCalls(int n) {
204+
public final void predictAdditionalCalls(int n) {
180205
if (indexComputed) {
181206
return;
182207
}
@@ -207,18 +232,18 @@ public void predictAdditionalCalls(int n) {
207232
protected void findCandidateCrossings(S2Point a, S2Point b, List<Integer> candidateCrossings) {
208233
Preconditions.checkState(indexComputed);
209234
ArrayList<S2CellId> cover = Lists.newArrayList();
210-
211235
getCovering(a, b, false, cover);
212-
getEdgesInParentCells(cover, sortedMapping, minimumS2LevelUsed, candidateCrossings);
236+
237+
// Edge references are inserted into the map once for each covering cell, so
238+
// absorb duplicates here
239+
Set<Integer> uniqueSet = new HashSet<Integer>();
240+
getEdgesInParentCells(cover, uniqueSet);
213241

214242
// TODO(user): An important optimization for long query
215243
// edges (Contains queries): keep a bounding cap and clip the query
216244
// edge to the cap before starting the descent.
217-
getEdgesInChildrenCells(a, b, cover, sortedMapping, candidateCrossings);
245+
getEdgesInChildrenCells(a, b, cover, uniqueSet);
218246

219-
// Remove duplicates: This is necessary because edge references are
220-
// inserted into the map once for each covering cell.
221-
Set<Integer> uniqueSet = new HashSet<Integer>(candidateCrossings);
222247
candidateCrossings.clear();
223248
candidateCrossings.addAll(uniqueSet);
224249
}
@@ -281,24 +306,13 @@ private int getCovering(
281306
S2Point a, S2Point b, boolean thickenEdge, ArrayList<S2CellId> edgeCovering) {
282307
edgeCovering.clear();
283308

284-
// kMinWidth taken from util/geometry/s2.[h,cc], and assumes that
285-
// (S2_PROJECTION == S2_QUADRATIC_PROJECTION).
286-
// TODO(andriy): this should live in S2.java, but that part of the code
287-
// hasn't been ported from the C++ S2 library yet, so putting our own
288-
// copy here for now. Also, this Java version differs from the C++ version
289-
// in that the C++ version uses a different coordinate system (in the C++
290-
// version, CL 1904327 changed the definition of the (s,t) coordinate
291-
// system to occupy the square [0,1]x[0,1] rather than [-1,1]x[-1,1].
292-
// So, kMinWidth here reflects the old [-1,1]x[-1,1] coordinate system.
293-
S2.Metric kMinWidth = new S2.Metric(1, Math.sqrt(2) / 3 /* 0.471 */);
294-
295309
// Selects the ideal s2 level at which to cover the edge, this will be the
296310
// level whose S2 cells have a width roughly commensurate to the length of
297311
// the edge. We multiply the edge length by 2*THICKENING to guarantee the
298312
// thickening is honored (it's not a big deal if we honor it when we don't
299313
// request it) when doing the covering-by-cap trick.
300314
double edgeLength = a.angle(b);
301-
int idealLevel = kMinWidth.getMaxLevel(edgeLength * (1 + 2 * THICKENING));
315+
int idealLevel = S2Projections.MIN_WIDTH.getMaxLevel(edgeLength * (1 + 2 * THICKENING));
302316

303317
S2CellId containingCellId;
304318
if (!thickenEdge) {
@@ -363,13 +377,11 @@ private int getCovering(
363377
* Filters a list of entries down to the inclusive range defined by the given
364378
* cells, in <code>O(log N)</code> time.
365379
*
366-
* @param entries The entries to filter.
367380
* @param cell1 One side of the inclusive query range.
368381
* @param cell2 The other side of the inclusive query range.
369-
* @return A sublist of the given list of entries that intersect the given
370-
* range of cell IDs.
382+
* @return An array of length 2, containing the start/end indices.
371383
*/
372-
private static List<Entry> getSubEntries(List<Entry> entries, long cell1, long cell2) {
384+
private int[] getEdges(long cell1, long cell2) {
373385
// ensure cell1 <= cell2
374386
if (cell1 > cell2) {
375387
long temp = cell1;
@@ -380,22 +392,34 @@ private static List<Entry> getSubEntries(List<Entry> entries, long cell1, long c
380392
// if an exact match cannot be found. Since the edge indices queried for are
381393
// not valid edge indices, we will always get -N-1, so we immediately
382394
// convert to N.
383-
int start = -1 - Collections.binarySearch(entries, new Entry(cell1, Integer.MIN_VALUE));
384-
int end = -1 - Collections.binarySearch(entries, new Entry(cell2, Integer.MAX_VALUE));
385-
// Note that when both query cells are either before or after the cell IDs
386-
// spanned by entries, the start and end indices will be equal, and in that
387-
// case List.subList returns the empty list.
388-
return entries.subList(start, end);
395+
return new int[]{
396+
-1 - binarySearch(cell1, Integer.MIN_VALUE),
397+
-1 - binarySearch(cell2, Integer.MAX_VALUE)};
398+
}
399+
400+
private int binarySearch(long cell, int edge) {
401+
int low = 0;
402+
int high = cells.length - 1;
403+
while (low <= high) {
404+
int mid = (low + high) >>> 1;
405+
int cmp = compare(cells[mid], edges[mid], cell, edge);
406+
if (cmp < 0) {
407+
low = mid + 1;
408+
} else if (cmp > 0) {
409+
high = mid - 1;
410+
} else {
411+
return mid;
412+
}
413+
}
414+
return -(low + 1);
389415
}
390416

391417
/**
392418
* Adds to candidateCrossings all the edges present in any ancestor of any
393419
* cell of cover, down to minimumS2LevelUsed. The cell->edge map is in the
394420
* variable mapping.
395421
*/
396-
private static void getEdgesInParentCells(List<S2CellId> cover,
397-
List<Entry> sortedMapping, int minimumS2LevelUsed,
398-
List<Integer> candidateCrossings) {
422+
private void getEdgesInParentCells(List<S2CellId> cover, Set<Integer> candidateCrossings) {
399423
// Find all parent cells of covering cells.
400424
Set<S2CellId> parentCells = Sets.newHashSet();
401425
for (S2CellId coverCell : cover) {
@@ -409,8 +433,9 @@ private static void getEdgesInParentCells(List<S2CellId> cover,
409433

410434
// Put parent cell edge references into result.
411435
for (S2CellId parentCell : parentCells) {
412-
for (Entry entry : getSubEntries(sortedMapping, parentCell.id(), parentCell.id())) {
413-
candidateCrossings.add(entry.edgeIndex);
436+
int[] bounds = getEdges(parentCell.id(), parentCell.id());
437+
for (int i = bounds[0]; i < bounds[1]; i++) {
438+
candidateCrossings.add(edges[i]);
414439
}
415440
}
416441
}
@@ -419,9 +444,9 @@ private static void getEdgesInParentCells(List<S2CellId> cover,
419444
* Returns true if ab possibly crosses cd, by clipping tiny angles to zero.
420445
*/
421446
private static boolean lenientCrossing(S2Point a, S2Point b, S2Point c, S2Point d) {
422-
Preconditions.checkArgument(S2.isUnitLength(a));
423-
Preconditions.checkArgument(S2.isUnitLength(b));
424-
Preconditions.checkArgument(S2.isUnitLength(c));
447+
// assert (S2.isUnitLength(a));
448+
// assert (S2.isUnitLength(b));
449+
// assert (S2.isUnitLength(c));
425450

426451
double acb = S2Point.crossProd(a, c).dotProd(b);
427452
double bda = S2Point.crossProd(b, d).dotProd(a);
@@ -463,23 +488,24 @@ private static boolean edgeIntersectsCellBoundary(S2Point a, S2Point b, S2Cell c
463488
* refined to eliminate quickly subcells that contain many edges but do not
464489
* intersect with edge.
465490
*/
466-
private static void getEdgesInChildrenCells(S2Point a, S2Point b, List<S2CellId> cover,
467-
List<Entry> entries, List<Integer> candidateCrossings) {
491+
private void getEdgesInChildrenCells(S2Point a, S2Point b, List<S2CellId> cover,
492+
Set<Integer> candidateCrossings) {
468493
// Put all edge references of (covering cells + descendant cells) into
469494
// result.
470495
// This relies on the natural ordering of S2CellIds.
471496
S2Cell[] children = null;
472497
while (!cover.isEmpty()) {
473498
S2CellId cell = cover.remove(cover.size() - 1);
474-
List<Entry> nearEntries = getSubEntries(entries, cell.rangeMin().id(), cell.rangeMax().id());
475-
if (nearEntries.size() <= 16) {
476-
for (Entry entry : nearEntries) {
477-
candidateCrossings.add(entry.edgeIndex);
499+
int[] bounds = getEdges(cell.rangeMin().id(), cell.rangeMax().id());
500+
if (bounds[1] - bounds[0] <= 16) {
501+
for (int i = bounds[0]; i < bounds[1]; i++) {
502+
candidateCrossings.add(edges[i]);
478503
}
479504
} else {
480505
// Add cells at this level
481-
for (Entry entry : getSubEntries(entries, cell.id(), cell.id())) {
482-
candidateCrossings.add(entry.edgeIndex);
506+
bounds = getEdges(cell.id(), cell.id());
507+
for (int i = bounds[0]; i < bounds[1]; i++) {
508+
candidateCrossings.add(edges[i]);
483509
}
484510
// Recurse on the children -- hopefully some will be empty.
485511
if (children == null) {
@@ -516,7 +542,7 @@ public static class DataEdgeIterator {
516542
/**
517543
* The structure containing the data edges.
518544
*/
519-
private S2EdgeIndex edgeIndex;
545+
private final S2EdgeIndex edgeIndex;
520546

521547
/**
522548
* Tells whether getCandidates() obtained the candidates through brute force

0 commit comments

Comments
 (0)