diff --git a/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraph.java b/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraph.java index e2b06df79..68274be45 100644 --- a/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraph.java +++ b/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraph.java @@ -1,7 +1,7 @@ package org.monarchinitiative.phenol.graph.csr.mono; +import org.monarchinitiative.phenol.graph.NodeNotPresentInGraphException; import org.monarchinitiative.phenol.graph.OntologyGraph; -import org.monarchinitiative.phenol.graph.csr.util.Util; import org.monarchinitiative.phenol.utils.IterableIteratorWrapper; import java.util.*; @@ -15,19 +15,16 @@ public class CsrMonoOntologyGraph implements OntologyGraph { private final T root; - private final T[] nodes; - private final Comparator comparator; + private final Map nodesToIdx; private final StaticCsrArray parents; private final StaticCsrArray children; CsrMonoOntologyGraph(T root, - T[] nodes, - Comparator comparator, + Map nodesToIdx, StaticCsrArray parents, StaticCsrArray children) { this.root = Objects.requireNonNull(root); - this.nodes = Objects.requireNonNull(nodes); - this.comparator = Objects.requireNonNull(comparator); + this.nodesToIdx = Objects.requireNonNull(nodesToIdx); this.parents = Objects.requireNonNull(parents); this.children = Objects.requireNonNull(children); } @@ -40,8 +37,11 @@ StaticCsrArray getChildArray() { return children; } - T[] getNodes() { - return nodes; + private int getNodeIdx(T node) { + Integer idx = nodesToIdx.get(node); + if (idx == null) + throw new NodeNotPresentInGraphException(String.format("Item not found in the graph: %s", node)); + return idx; } @Override @@ -57,7 +57,7 @@ public Iterable getChildren(T source, boolean includeSource) { @Override public Iterable getDescendants(T source, boolean includeSource) { // Check if `source` is in the graph. - int intentionallyUnused = Util.getIndexOfUsingBinarySearch(source, nodes, comparator); + int intentionallyUnused = getNodeIdx(source); return new IterableIteratorWrapper<>(() -> new TraversingIterator<>(source, src -> getChildren(src, includeSource))); } @@ -70,7 +70,7 @@ public Iterable getParents(T source, boolean includeSource) { @Override public Iterable getAncestors(T source, boolean includeSource) { // Check if `source` is in the graph. - int intentionallyUnused = Util.getIndexOfUsingBinarySearch(source, nodes, comparator); + int intentionallyUnused = getNodeIdx(source); return new IterableIteratorWrapper<>(() -> new TraversingIterator<>(source, src -> getParents(src, includeSource))); } @@ -78,7 +78,8 @@ public Iterable getAncestors(T source, boolean includeSource) { private Iterable getImmediateNeighbors(StaticCsrArray array, T source, boolean includeSource) { - int index = Util.getIndexOfUsingBinarySearch(source, nodes, comparator); + int index = getNodeIdx(source); + Set nodes = array.getOutgoingNodes(index); return includeSource @@ -88,13 +89,12 @@ private Iterable getImmediateNeighbors(StaticCsrArray array, @Override public int size() { - return nodes.length; + return nodesToIdx.size(); } @Override public Iterator iterator() { - // TODO - can we do better here? - return Arrays.stream(nodes).iterator(); + return nodesToIdx.keySet().iterator(); } } diff --git a/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilder.java b/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilder.java index 0ea550606..7314115f7 100644 --- a/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilder.java +++ b/phenol-core/src/main/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilder.java @@ -6,10 +6,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -55,8 +52,13 @@ public CsrMonoOntologyGraph build(TermId root, Collection csrData = makeCsrData(nodes, hierarchyEdges); + Map nodeToIdx = new HashMap<>(); + for (int i = 0; i < nodes.length; i++) { + TermId node = nodes[i]; + nodeToIdx.put(node, i); + } - return new CsrMonoOntologyGraph<>(root, nodes, TermId::compareTo, csrData.getParents(), csrData.getChildren()); + return new CsrMonoOntologyGraph<>(root, nodeToIdx, csrData.getParents(), csrData.getChildren()); } private CsrData makeCsrData(TermId[] nodes, diff --git a/phenol-core/src/test/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilderTest.java b/phenol-core/src/test/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilderTest.java index 2de077c0d..fe94610ae 100644 --- a/phenol-core/src/test/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilderTest.java +++ b/phenol-core/src/test/java/org/monarchinitiative/phenol/graph/csr/mono/CsrMonoOntologyGraphBuilderTest.java @@ -56,7 +56,9 @@ public void checkData() { /* HP:03 */ {"HP:1"}, /* HP:1 */ {}, }; - assertThatGroupsAreConsistent(graph.getNodes(), graph.getParentArray(), expectedParents); + + + assertThatGroupsAreConsistent(graph.size(), graph.getParentArray(), expectedParents); String[][] expectedChildren = { /* HP:01 */ {"HP:010", "HP:011"}, @@ -70,15 +72,15 @@ public void checkData() { /* HP:03 */ {}, /* HP:1 */ {"HP:01", "HP:02", "HP:03"}, }; - assertThatGroupsAreConsistent(graph.getNodes(), graph.getChildArray(), expectedChildren); + assertThatGroupsAreConsistent(graph.size(), graph.getChildArray(), expectedChildren); } - private void assertThatGroupsAreConsistent(TermId[] nodes, + private void assertThatGroupsAreConsistent(int size, StaticCsrArray array, String[][] expectedCuries) { int[] indptr = array.getIndptr(); TermId[] data = array.getData(); - for (int i = 0; i < nodes.length; i++) { + for (int i = 0; i < size; i++) { int start = indptr[i]; int end = indptr[i + 1]; String[] curies = expectedCuries[i]; diff --git a/phenol-io/src/test/java/org/monarchinitiative/phenol/io/benches/OntologyGraphBench.java b/phenol-io/src/test/java/org/monarchinitiative/phenol/io/benches/OntologyGraphBench.java index bc3bcc130..f0bb343cd 100644 --- a/phenol-io/src/test/java/org/monarchinitiative/phenol/io/benches/OntologyGraphBench.java +++ b/phenol-io/src/test/java/org/monarchinitiative/phenol/io/benches/OntologyGraphBench.java @@ -13,29 +13,87 @@ public class OntologyGraphBench { @Benchmark public void poly_getParents(CsrPolyOntologyGraphSetup ontology, Blackhole blackhole) { for (TermId termId : ontology.primaryTermIds) { - blackhole.consume(ontology.ontology.graph().getParents(termId, false)); + ontology.ontology.graph().getParents(termId, false) + .forEach(blackhole::consume); } } @Benchmark public void poly_getChildren(CsrPolyOntologyGraphSetup ontology, Blackhole blackhole) { for (TermId termId : ontology.primaryTermIds) { - blackhole.consume(ontology.ontology.graph().getChildren(termId, false)); + ontology.ontology.graph().getChildren(termId, false) + .forEach(blackhole::consume); + } + } + + @Benchmark + public void poly_getAncestors(CsrPolyOntologyGraphSetup ontology, Blackhole blackhole) { + for (TermId termId : ontology.primaryTermIds) { + ontology.ontology.graph().getAncestors(termId, false) + .forEach(blackhole::consume); + } + } + + @Benchmark + public void poly_getDescendants(CsrPolyOntologyGraphSetup ontology, Blackhole blackhole) { + for (TermId termId : ontology.primaryTermIds) { + ontology.ontology.graph().getDescendants(termId, false) + .forEach(blackhole::consume); } } @Benchmark public void mono_getParents(CsrMonoOntologyGraphSetup ontology, Blackhole blackhole) { for (TermId termId : ontology.primaryTermIds) { - blackhole.consume(ontology.ontology.graph().getParents(termId, false)); + ontology.ontology.graph().getParents(termId, false) + .forEach(blackhole::consume); } } @Benchmark public void mono_getChildren(CsrMonoOntologyGraphSetup ontology, Blackhole blackhole) { for (TermId termId : ontology.primaryTermIds) { - blackhole.consume(ontology.ontology.graph().getChildren(termId, false)); + ontology.ontology.graph().getChildren(termId, false) + .forEach(blackhole::consume); } } + @Benchmark + public void mono_getAncestors(CsrMonoOntologyGraphSetup ontology, Blackhole blackhole) { + for (TermId termId : ontology.primaryTermIds) { + ontology.ontology.graph().getAncestors(termId, false) + .forEach(blackhole::consume); + } + } + + @Benchmark + public void mono_getDescendants(CsrMonoOntologyGraphSetup ontology, Blackhole blackhole) { + for (TermId termId : ontology.primaryTermIds) { + ontology.ontology.graph().getDescendants(termId, false) + .forEach(blackhole::consume); + } + } + + /* + # Run complete. Total time: 00:33:59 + Commit 6a470a0f + + REMEMBER: The numbers below are just data. To gain reusable insights, you need to follow up on + why the numbers are the way they are. Use profilers (see -prof, -lprof), design factorial + experiments, perform baseline and negative tests that provide experimental control, make sure + the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from the domain experts. + Do not assume the numbers tell you what you want them to tell. + + NOTE: Current JVM experimentally supports Compiler Blackholes, and they are in use. Please exercise + extra caution when trusting the results, look into the generated code to check the benchmark still + works, and factor in a small probability of new VM bugs. Additionally, while comparisons between + different JVMs are already problematic, the performance difference caused by different Blackhole + modes can be very significant. Please make sure you use the consistent Blackhole mode for comparisons. + + Benchmark Mode Cnt Score Error Units + OntologyGraphBench.mono_getChildren thrpt 25 163.773 ± 11.709 ops/s + OntologyGraphBench.mono_getParents thrpt 25 149.663 ± 8.890 ops/s + OntologyGraphBench.poly_getChildren thrpt 25 134.231 ± 7.379 ops/s + OntologyGraphBench.poly_getParents thrpt 25 131.846 ± 5.647 ops/s + */ }