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

Optimize DFS while marking connected components #14022

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Optimize DFS while marking connected components #14022

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d0539d
Optimize DFS while marking connected components
viswanathk Nov 27, 2024
ea82561
Use IntHashSet instead of FixedBitSet
viswanathk Nov 28, 2024
d6ed90d
Add only components with non zero nodes
viswanathk Dec 18, 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
14 changes: 12 additions & 2 deletions lucene/core/src/java/org/apache/lucene/util/hnsw/HnswUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.lucene.index.FilterLeafReader;
import org.apache.lucene.index.IndexReader;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.internal.hppc.IntHashSet;
import org.apache.lucene.util.FixedBitSet;

/** Utilities for use in tests involving HNSW graphs */
Expand Down Expand Up @@ -105,7 +106,9 @@ static List<Component> components(
} else {
entryPoint = connectedNodes.nextSetBit(0);
}
components.add(new Component(entryPoint, total));
if (total > 0) {
components.add(new Component(entryPoint, total));
}
if (level == 0) {
int nextClear = nextClearBit(connectedNodes, 0);
while (nextClear != NO_MORE_DOCS) {
Expand Down Expand Up @@ -163,6 +166,10 @@ private static Component markRooted(
throws IOException {
// Start at entry point and search all nodes on this level
// System.out.println("markRooted level=" + level + " entryPoint=" + entryPoint);
if (connectedNodes.get(entryPoint)) {
return new Component(entryPoint, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should never happen, right? because we enter with the next non-connected node. Can we add an assert false here before the return statement so we catch during testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, this can happen because we iterate over all the entryPoints. Q: do we need this zero-size component for anything? Can we recall what happens with these componentws when we're done - the only purpose is to use them for reconnecting the graph. Yeah it looks like we will try to connect them again, which we could skip. Let's not add these empty components to the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, this can happen because we iterate over all the entryPoints. Q: do we need this zero-size component for anything? Can we recall what happens with these componentws when we're done - the only purpose is to use them for reconnecting the graph. Yeah it looks like we will try to connect them again, which we could skip. Let's not add these empty components to the list.

I don't think we are adding the empty components to the list though. We are adding to the list with the total of the entryPoints for that level (which seems unlikely).

In the other places we add, we start the markRooted process with the nextClearBit, so it won't return 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we are adding the empty components to the list though. We are adding to the list with the total of the entryPoints for that level (which seems unlikely).

But seems like a good check. Updated the PR.

}
IntHashSet nodesInStack = new IntHashSet();
Deque<Integer> stack = new ArrayDeque<>();
stack.push(entryPoint);
int count = 0;
Expand All @@ -178,7 +185,10 @@ private static Component markRooted(
int friendCount = 0;
while ((friendOrd = hnswGraph.nextNeighbor()) != NO_MORE_DOCS) {
++friendCount;
stack.push(friendOrd);
if (connectedNodes.get(friendOrd) == false && nodesInStack.contains(friendOrd) == false) {
viswanathk marked this conversation as resolved.
Show resolved Hide resolved
stack.push(friendOrd);
nodesInStack.add(friendOrd);
}
}
if (friendCount < maxConn && notFullyConnected != null) {
notFullyConnected.set(node);
Expand Down
Loading