rip timings gathering out from ResultBuilder

neo4j · Jan 10, 2024 · a6e7fac · a6e7fac
1 parent af2fcb1
commit a6e7fac
Show file tree

Hide file tree

Showing 9 changed files with 150 additions and 60 deletions.
diff --git a/...in/java/org/neo4j/gds/applications/algorithms/pathfinding/AlgorithmProcessingTimings.java b/...in/java/org/neo4j/gds/applications/algorithms/pathfinding/AlgorithmProcessingTimings.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) "Neo4j"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.gds.applications.algorithms.pathfinding;
+
+/**
+ * This is finalised timings for one algorithm processing. Going for a good old struct here
+ */
+public class AlgorithmProcessingTimings {
+    public final long preProcessingMillis;
+    public final long computeMillis;
+    public final long postProcessingMillis;
+
+    AlgorithmProcessingTimings(long preProcessingMillis, long computeMillis, long postProcessingMillis) {
+        this.preProcessingMillis = preProcessingMillis;
+        this.computeMillis = computeMillis;
+        this.postProcessingMillis = postProcessingMillis;
+    }
+}
diff --git a/.../org/neo4j/gds/applications/algorithms/pathfinding/AlgorithmProcessingTimingsBuilder.java b/.../org/neo4j/gds/applications/algorithms/pathfinding/AlgorithmProcessingTimingsBuilder.java
@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) "Neo4j"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.gds.applications.algorithms.pathfinding;
+
+/**
+ * This guy gathers timings, builder-stylee.
+ */
+class AlgorithmProcessingTimingsBuilder {
+    // This is a marker
+    private static final int NOT_AVAILABLE = -1;
+
+    // timings
+    protected long preProcessingMillis = NOT_AVAILABLE;
+    protected long computeMillis = NOT_AVAILABLE;
+    protected long postProcessingMillis = NOT_AVAILABLE; // mutate or write timing
+
+    public void withPreProcessingMillis(long preProcessingMillis) {
+        this.preProcessingMillis = preProcessingMillis;
+    }
+
+    public void withComputeMillis(long computeMillis) {
+        this.computeMillis = computeMillis;
+    }
+
+    public void withPostProcessingMillis(long postProcessingMillis) {
+        this.postProcessingMillis = postProcessingMillis;
+    }
+
+    AlgorithmProcessingTimings build() {
+        return new AlgorithmProcessingTimings(
+            preProcessingMillis,
+            computeMillis,
+            postProcessingMillis
+        );
+    }
+}
diff --git a/...org/neo4j/gds/applications/algorithms/pathfinding/DefaultAlgorithmProcessingTemplate.java b/...org/neo4j/gds/applications/algorithms/pathfinding/DefaultAlgorithmProcessingTemplate.java
@@ -73,7 +73,10 @@ public <CONFIGURATION extends AlgoBaseConfig & RelationshipWeightConfig, RESULT_
         Optional<MutateOrWriteStep<RESULT_FROM_ALGORITHM>> mutateOrWriteStep,
         ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder
     ) {
+        var timingsBuilder = new AlgorithmProcessingTimingsBuilder();
+
         Pair<Graph, GraphStore> graphWithGraphStore = graphLoadAndValidationWithTiming(
+            timingsBuilder,
             graphName,
             configuration,
             resultBuilder
@@ -82,18 +85,18 @@ public <CONFIGURATION extends AlgoBaseConfig & RelationshipWeightConfig, RESULT_
         var graph = graphWithGraphStore.getLeft();
         var graphStore = graphWithGraphStore.getRight();
 
-        if (graph.isEmpty()) return resultBuilder.build(graph, graphStore, Optional.empty());
+        if (graph.isEmpty()) return resultBuilder.build(graph, graphStore, Optional.empty(), timingsBuilder.build());
 
         memoryGuard.assertAlgorithmCanRun(humanReadableAlgorithmName, configuration, graph, estimationFactory);
 
         // do the actual computation
-        var result = computeWithTiming(humanReadableAlgorithmName, algorithmComputation, resultBuilder, graph);
+        var result = computeWithTiming(timingsBuilder, humanReadableAlgorithmName, algorithmComputation, resultBuilder, graph);
 
         // do any side effects
-        mutateOrWriteWithTiming(mutateOrWriteStep, resultBuilder, graph, graphStore, result);
+        mutateOrWriteWithTiming(mutateOrWriteStep, timingsBuilder, graph, graphStore, result, resultBuilder);
 
         // inject dependencies to render results
-        return resultBuilder.build(graph, graphStore, Optional.ofNullable(result));
+        return resultBuilder.build(graph, graphStore, Optional.ofNullable(result), timingsBuilder.build());
     }
 
     /**
@@ -110,11 +113,12 @@ public <CONFIGURATION extends AlgoBaseConfig & RelationshipWeightConfig, RESULT_
      * </ul>
      */
     <CONFIGURATION extends AlgoBaseConfig & RelationshipWeightConfig, RESULT_TO_CALLER, RESULT_FROM_ALGORITHM> Pair<Graph, GraphStore> graphLoadAndValidationWithTiming(
+        AlgorithmProcessingTimingsBuilder timingsBuilder,
         GraphName graphName,
         CONFIGURATION configuration,
         ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder
     ) {
-        try (ProgressTimer ignored = ProgressTimer.start(resultBuilder::withPreProcessingMillis)) {
+        try (ProgressTimer ignored = ProgressTimer.start(timingsBuilder::withPreProcessingMillis)) {
             // tee up the graph we want to work on
             var graphWithGraphStore = graphStoreCatalogService.getGraphWithGraphStore(
                 graphName,
@@ -131,12 +135,13 @@ <CONFIGURATION extends AlgoBaseConfig & RelationshipWeightConfig, RESULT_TO_CALL
     }
 
     <RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> RESULT_FROM_ALGORITHM computeWithTiming(
+        AlgorithmProcessingTimingsBuilder timingsBuilder,
         String humanReadableAlgorithmName,
         AlgorithmComputation<RESULT_FROM_ALGORITHM> algorithmComputation,
         ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder,
         Graph graph
     ) {
-        try (ProgressTimer ignored = ProgressTimer.start(resultBuilder::withComputeMillis)) {
+        try (ProgressTimer ignored = ProgressTimer.start(timingsBuilder::withComputeMillis)) {
             return computeWithMetric(humanReadableAlgorithmName, algorithmComputation, graph);
         }
     }
@@ -161,13 +166,14 @@ private <RESULT_FROM_ALGORITHM> RESULT_FROM_ALGORITHM computeWithMetric(
 
     <RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> void mutateOrWriteWithTiming(
         Optional<MutateOrWriteStep<RESULT_FROM_ALGORITHM>> mutateOrWriteStep,
-        ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder,
+        AlgorithmProcessingTimingsBuilder timingsBuilder,
         Graph graph,
         GraphStore graphStore,
-        RESULT_FROM_ALGORITHM result
+        RESULT_FROM_ALGORITHM result,
+        ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder
     ) {
         mutateOrWriteStep.ifPresent(step -> {
-            try (ProgressTimer ignored = ProgressTimer.start(resultBuilder::withPostProcessingMillis)) {
+            try (ProgressTimer ignored = ProgressTimer.start(timingsBuilder::withPostProcessingMillis)) {
                 step.execute(graph, graphStore, result, resultBuilder);
             }
         });

diff --git a/...inding/src/main/java/org/neo4j/gds/applications/algorithms/pathfinding/ResultBuilder.java b/...inding/src/main/java/org/neo4j/gds/applications/algorithms/pathfinding/ResultBuilder.java
@@ -25,7 +25,7 @@
 import java.util.Optional;
 
 /**
- * This builder gathers data as part of algorithm processing. Timings and such.
+ * This builder gathers data as part of algorithm processing
  * You specialise it for use cases, but a lot of what it needs is generic,
  * and it is part of algorithm processing instrumentation.
  * In-layer generic usage includes injecting the Graph, hence it is a parameter to the build method.
@@ -35,27 +35,13 @@
  * where any given usage probably won't need all of them.
  */
 public abstract class ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> {
-    // timings
-    protected long preProcessingMillis;
-    protected long computeMillis;
-    protected long postProcessingMillis = -1; // mutate or write timing
+    // a marker
+    private static final int NOT_AVAILABLE = -1;
 
     // union type: zero or more of these get populated by your own hooks
-    protected long nodeCount;
-    protected long nodePropertiesWritten;
-    protected long relationshipsWritten;
-
-    public void withPreProcessingMillis(long preProcessingMillis) {
-        this.preProcessingMillis = preProcessingMillis;
-    }
-
-    public void withComputeMillis(long computeMillis) {
-        this.computeMillis = computeMillis;
-    }
-
-    public void withPostProcessingMillis(long postProcessingMillis) {
-        this.postProcessingMillis = postProcessingMillis;
-    }
+    protected long nodeCount = NOT_AVAILABLE;
+    protected long nodePropertiesWritten = NOT_AVAILABLE;
+    protected long relationshipsWritten = NOT_AVAILABLE;
 
     public void withNodeCount(long nodeCount) {
         this.nodeCount = nodeCount;
@@ -78,6 +64,7 @@ public void withRelationshipsWritten(long relationshipsWritten) {
     public abstract RESULT_TO_CALLER build(
         Graph graph,
         GraphStore graphStore,
-        Optional<RESULT_FROM_ALGORITHM> result
+        Optional<RESULT_FROM_ALGORITHM> result,
+        AlgorithmProcessingTimings timings
     );
 }
diff --git a/...rc/main/java/org/neo4j/gds/applications/algorithms/pathfinding/ShortestPathWriteStep.java b/...rc/main/java/org/neo4j/gds/applications/algorithms/pathfinding/ShortestPathWriteStep.java
@@ -25,7 +25,6 @@
 import org.neo4j.gds.api.IdMap;
 import org.neo4j.gds.api.nodeproperties.ValueType;
 import org.neo4j.gds.config.WriteRelationshipConfig;
-import org.neo4j.gds.core.utils.ProgressTimer;
 import org.neo4j.gds.core.utils.progress.TaskRegistryFactory;
 import org.neo4j.gds.core.utils.progress.tasks.TaskProgressTracker;
 import org.neo4j.gds.core.write.ImmutableExportedRelationship;
@@ -124,22 +123,20 @@ public <RESULT_TO_CALLER> void execute(
             var writeRelationshipType = configuration.writeRelationshipType();
 
             /*
-             * The actual export, with timing.
+             * The actual export.
              * Notice that originally we had a CloseableResourceRegistry thing going on here - no longer.
              * Because all we are doing is, processing a stream using the exporter, synchronously.
              * We are not handing it out to upper layers for sporadic consumption.
              * It is done right here, and when we complete, the stream is exhausted.
              * We still explicitly close the stream tho because, yeah, confusion I guess.
              */
-            try (ProgressTimer ignored = ProgressTimer.start(resultBuilder::withPostProcessingMillis)) {
-                var keys = createKeys(writeNodeIds, writeCosts);
-                var types = createTypes(writeNodeIds, writeCosts);
+            var keys = createKeys(writeNodeIds, writeCosts);
+            var types = createTypes(writeNodeIds, writeCosts);
 
-                var relationshipsWritten = relationshipStreamExporter.write(writeRelationshipType, keys, types);
+            var relationshipsWritten = relationshipStreamExporter.write(writeRelationshipType, keys, types);
 
-                // the final result is the side effect of writing to the database, plus this metadata
-                resultBuilder.withRelationshipsWritten(relationshipsWritten);
-            }
+            // the final result is the side effect of writing to the database, plus this metadata
+            resultBuilder.withRelationshipsWritten(relationshipsWritten);
         }
     }
 

diff --git a/...neo4j/gds/applications/algorithms/pathfinding/DefaultAlgorithmProcessingTemplateTest.java b/...neo4j/gds/applications/algorithms/pathfinding/DefaultAlgorithmProcessingTemplateTest.java
@@ -83,10 +83,11 @@ void shouldProcessStreamAlgorithm() {
             public Stream<String> build(
                 Graph graph,
                 GraphStore graphStore,
-                Optional<PathFindingResult> pathFindingResult
+                Optional<PathFindingResult> pathFindingResult,
+                AlgorithmProcessingTimings timings
             ) {
                 // we skip timings when no side effects requested
-                assertThat(postProcessingMillis).isEqualTo(-1);
+                assertThat(timings.postProcessingMillis).isEqualTo(-1);
 
                 return Stream.of(
                     "Huey",
@@ -150,7 +151,8 @@ void shouldProcessMutateOrWriteAlgorithm() {
             public Long build(
                 Graph actualGraph,
                 GraphStore actualGraphStore,
-                Optional<PathFindingResult> actualResult
+                Optional<PathFindingResult> actualResult,
+                AlgorithmProcessingTimings timings
             ) {
                 assertThat(actualGraph).isEqualTo(graph);
                 assertThat(actualGraphStore).isEqualTo(graphStore);
@@ -204,34 +206,37 @@ void shouldDoTimings() {
         ) {
             @Override
             <CONFIGURATION extends AlgoBaseConfig & RelationshipWeightConfig, RESULT_TO_CALLER, RESULT_FROM_ALGORITHM> Pair<Graph, GraphStore> graphLoadAndValidationWithTiming(
+                AlgorithmProcessingTimingsBuilder timingsBuilder,
                 GraphName graphName,
                 CONFIGURATION configuration,
                 ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder
             ) {
-                resultBuilder.withPreProcessingMillis(23);
+                timingsBuilder.withPreProcessingMillis(23);
                 return Pair.of(mock(Graph.class), null);
             }
 
             @Override
             <RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> RESULT_FROM_ALGORITHM computeWithTiming(
+                AlgorithmProcessingTimingsBuilder timingsBuilder,
                 String humanReadableAlgorithmName,
                 AlgorithmComputation<RESULT_FROM_ALGORITHM> algorithmComputation,
                 ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder,
                 Graph graph
             ) {
-                resultBuilder.withComputeMillis(117);
+                timingsBuilder.withComputeMillis(117);
                 return null;
             }
 
             @Override
             <RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> void mutateOrWriteWithTiming(
                 Optional<MutateOrWriteStep<RESULT_FROM_ALGORITHM>> mutateOrWriteStep,
-                ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder,
+                AlgorithmProcessingTimingsBuilder timingsBuilder,
                 Graph graph,
                 GraphStore graphStore,
-                RESULT_FROM_ALGORITHM resultFromAlgorithm
+                RESULT_FROM_ALGORITHM resultFromAlgorithm,
+                ResultBuilder<RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> resultBuilder
             ) {
-                resultBuilder.withPostProcessingMillis(87);
+                timingsBuilder.withPostProcessingMillis(87);
             }
         };
 
@@ -240,12 +245,13 @@ <RESULT_FROM_ALGORITHM, RESULT_TO_CALLER> void mutateOrWriteWithTiming(
             public Map<String, Long> build(
                 Graph graph,
                 GraphStore graphStore,
-                Optional<Void> unused
+                Optional<Void> unused,
+                AlgorithmProcessingTimings timings
             ) {
                 return Map.of(
-                    "preProcessingMillis", preProcessingMillis,
-                    "computeMillis", computeMillis,
-                    "postProcessingMillis", postProcessingMillis
+                    "preProcessingMillis", timings.preProcessingMillis,
+                    "computeMillis", timings.computeMillis,
+                    "postProcessingMillis", timings.postProcessingMillis
                 );
             }
         };

diff --git a/...main/java/org/neo4j/gds/procedures/pathfinding/PathFindingResultBuilderForMutateMode.java b/...main/java/org/neo4j/gds/procedures/pathfinding/PathFindingResultBuilderForMutateMode.java
@@ -21,6 +21,7 @@
 
 import org.neo4j.gds.api.Graph;
 import org.neo4j.gds.api.GraphStore;
+import org.neo4j.gds.applications.algorithms.pathfinding.AlgorithmProcessingTimings;
 import org.neo4j.gds.applications.algorithms.pathfinding.ResultBuilder;
 import org.neo4j.gds.config.ToMapConvertible;
 import org.neo4j.gds.paths.dijkstra.PathFindingResult;
@@ -38,13 +39,14 @@ class PathFindingResultBuilderForMutateMode extends ResultBuilder<PathFindingRes
     public PathFindingMutateResult build(
         Graph graph,
         GraphStore graphStore,
-        Optional<PathFindingResult> pathFindingResult
+        Optional<PathFindingResult> pathFindingResult,
+        AlgorithmProcessingTimings timings
     ) {
         return new PathFindingMutateResult(
-            preProcessingMillis,
-            computeMillis,
+            timings.preProcessingMillis,
+            timings.computeMillis,
             0, // yeah, I don't understand it either :shrug:
-            postProcessingMillis,
+            timings.postProcessingMillis,
             relationshipsWritten,
             configuration.toMap()
         );

diff --git a/...main/java/org/neo4j/gds/procedures/pathfinding/PathFindingResultBuilderForStreamMode.java b/...main/java/org/neo4j/gds/procedures/pathfinding/PathFindingResultBuilderForStreamMode.java
@@ -22,6 +22,7 @@
 import org.neo4j.gds.api.Graph;
 import org.neo4j.gds.api.GraphStore;
 import org.neo4j.gds.api.NodeLookup;
+import org.neo4j.gds.applications.algorithms.pathfinding.AlgorithmProcessingTimings;
 import org.neo4j.gds.applications.algorithms.pathfinding.ResultBuilder;
 import org.neo4j.gds.paths.dijkstra.PathFindingResult;
 import org.neo4j.graphdb.Path;
@@ -50,7 +51,8 @@ public class PathFindingResultBuilderForStreamMode extends ResultBuilder<PathFin
     public Stream<PathFindingStreamResult> build(
         Graph graph,
         GraphStore graphStore,
-        Optional<PathFindingResult> result
+        Optional<PathFindingResult> result,
+        AlgorithmProcessingTimings timings
     ) {
         if (result.isEmpty()) return Stream.of();