Revert "Introduce new timers for the different phases of the Cascades planner"

This reverts commit 7e8cc8e.

Author: hazefully

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

@@ -26,7 +26,6 @@
 import com.apple.foundationdb.record.RecordStoreState;
 import com.apple.foundationdb.record.logging.KeyValueLogMessage;
 import com.apple.foundationdb.record.logging.LogMessageKeys;
-import com.apple.foundationdb.record.provider.common.StoreTimer;
 import com.apple.foundationdb.record.query.IndexQueryabilityFilter;
 import com.apple.foundationdb.record.query.ParameterRelationshipGraph;
 import com.apple.foundationdb.record.query.RecordQuery;
@@ -56,7 +55,6 @@
 import org.slf4j.LoggerFactory;
 
 import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
 import java.util.ArrayDeque;
 import java.util.Collection;
 import java.util.Deque;
@@ -214,8 +212,6 @@ public class CascadesPlanner implements QueryPlanner {
     private final RecordMetaData metaData;
     @Nonnull
     private final RecordStoreState recordStoreState;
-    @Nullable
-    private final StoreTimer storeTimer;
     @Nonnull
     private Reference currentRoot;
     @Nonnull
@@ -232,14 +228,9 @@ public class CascadesPlanner implements QueryPlanner {
     private int maxQueueSize;
 
     public CascadesPlanner(@Nonnull RecordMetaData metaData, @Nonnull RecordStoreState recordStoreState) {
-        this(metaData, recordStoreState, null);
-    }
-
-    public CascadesPlanner(@Nonnull RecordMetaData metaData, @Nonnull RecordStoreState recordStoreState, @Nullable StoreTimer storeTimer) {
         this.configuration = RecordQueryPlannerConfiguration.builder().build();
         this.metaData = metaData;
         this.recordStoreState = recordStoreState;
-        this.storeTimer = storeTimer;
         // Placeholders until we get a query.
         this.currentRoot = Reference.empty();
         this.planContext = PlanContext.emptyContext();
@@ -533,41 +524,6 @@ public interface Task {
         Debugger.Event toTaskEvent(Location location);
     }
 
-    private class FinalizePlannerPhase implements Task {
-        @Nonnull
-        private final PlannerPhase plannerPhase;
-        private final long phaseStartTime;
-
-        public FinalizePlannerPhase(@Nonnull final PlannerPhase plannerPhase, long phaseStartTime) {
-            this.plannerPhase = plannerPhase;
-            this.phaseStartTime = phaseStartTime;
-        }
-
-        @Override
-        @Nonnull
-        public PlannerPhase getPlannerPhase() {
-            return plannerPhase;
-        }
-
-        @Override
-        public void execute() {
-            if (storeTimer != null) {
-                storeTimer.record(plannerPhase.getPhaseCompletionTimerEvent(), System.nanoTime() - phaseStartTime);
-            }
-        }
-
-        @Override
-        @Nonnull
-        public Debugger.Event toTaskEvent(final Location location) {
-            return new Debugger.FinalizePlannerPhaseEvent(plannerPhase, currentRoot, taskStack, location);
-        }
-
-        @Override
-        public String toString() {
-            return "FinalizePlannerPhase(" + plannerPhase.name() + ")";
-        }
-    }
-
     /**
      * Globally initiate a new planner phase.
      * Simplified push/execute overview:
@@ -596,12 +552,10 @@ public PlannerPhase getPlannerPhase() {
 
         @Override
         public void execute() {
-            var phaseStartTime = System.nanoTime();
             if (plannerPhase.hasNextPhase()) {
                 // if there is another phase push it first so it gets executed at the very end
                 taskStack.push(new InitiatePlannerPhase(plannerPhase.getNextPhase()));
             }
-            taskStack.push(new FinalizePlannerPhase(plannerPhase, phaseStartTime));
             taskStack.push(new OptimizeGroup(plannerPhase, currentRoot));
             taskStack.push(new ExploreGroup(plannerPhase, currentRoot));
         }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/PlannerPhase.java

@@ -35,8 +35,8 @@
  */
 public enum PlannerPhase {
     // note that the phase are declared in a counterintuitive inverse way since a phase has to specify the next phase
-    PLANNING(PlanningRuleSet.getDefault(), PlannerStage.PLANNED, PlanningCostModel::new, PlannerTimerEvents.PLANNING_PHASE_COMPLETE),
-    REWRITING(RewritingRuleSet.getDefault(), PlannerStage.CANONICAL, RewritingCostModel::new, PlannerTimerEvents.REWRITING_PHASE_COMPLETE, PLANNING);
+    PLANNING(PlanningRuleSet.getDefault(), PlannerStage.PLANNED, PlanningCostModel::new),
+    REWRITING(RewritingRuleSet.getDefault(), PlannerStage.CANONICAL, RewritingCostModel::new, PLANNING);
 
     @Nonnull
     private final CascadesRuleSet ruleSet;
@@ -46,26 +46,21 @@ public enum PlannerPhase {
     private final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator;
     @Nullable
     private final PlannerPhase nextPhase;
-    @Nonnull
-    private final PlannerTimerEvents phaseCompletionTimerEvent;
 
     PlannerPhase(@Nonnull final CascadesRuleSet ruleSet,
                  @Nonnull final PlannerStage targetStage,
-                 @Nonnull final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator,
-                 @Nonnull final PlannerTimerEvents phaseCompletionTimerEvent) {
-        this(ruleSet, targetStage, costModelCreator, phaseCompletionTimerEvent, null);
+                 @Nonnull final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator) {
+        this(ruleSet, targetStage, costModelCreator, null);
     }
 
     PlannerPhase(@Nonnull final CascadesRuleSet ruleSet,
                  @Nonnull final PlannerStage targetStage,
                  @Nonnull final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator,
-                 @Nonnull final PlannerTimerEvents phaseCompletionTimerEvent,
                  @Nullable final PlannerPhase nextPhase) {
         this.ruleSet = ruleSet;
         this.targetStage = targetStage;
         this.costModelCreator = costModelCreator;
         this.nextPhase = nextPhase;
-        this.phaseCompletionTimerEvent = phaseCompletionTimerEvent;
     }
 
     @Nonnull
@@ -92,11 +87,6 @@ public boolean hasNextPhase() {
         return nextPhase != null;
     }
 
-    @Nonnull
-    public PlannerTimerEvents getPhaseCompletionTimerEvent() {
-        return phaseCompletionTimerEvent;
-    }
-
     @Nonnull
     public PPlannerPhase toProto() {
         switch (this) {

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/PlannerTimerEvents.java

@@ -36,7 +36,6 @@
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PExecutingTaskEvent;
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PExploreExpressionEvent;
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PExploreGroupEvent;
-import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PFinalizePlannerPhaseEvent;
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PInitiatePlannerPhaseEvent;
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PInsertIntoMemoEvent;
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PMatchPartition;
@@ -261,8 +260,7 @@ enum Shorthand {
         TRANSFORM,
         INSERT_INTO_MEMO,
         TRANSLATE_CORRELATIONS,
-        INITPHASE,
-        FINALIZEPHASE
+        INITPHASE
     }
 
     /**
@@ -531,42 +529,6 @@ public PEvent.Builder toEventBuilder() {
         }
     }
 
-    class FinalizePlannerPhaseEvent extends AbstractEventWithState {
-        public FinalizePlannerPhaseEvent(@Nonnull final PlannerPhase plannerPhase,
-                                         @Nonnull final Reference rootReference,
-                                         @Nonnull final Deque<Task> taskStack,
-                                         @Nonnull final Location location) {
-            super(plannerPhase, rootReference, taskStack, location);
-        }
-
-        @Override
-        @Nonnull
-        public String getDescription() {
-            return "finalizing planner phase " + getPlannerPhase().name();
-        }
-
-        @Override
-        @Nonnull
-        public Shorthand getShorthand() {
-            return Shorthand.FINALIZEPHASE;
-        }
-
-        @Nonnull
-        @Override
-        public PFinalizePlannerPhaseEvent toProto() {
-            return PFinalizePlannerPhaseEvent.newBuilder()
-                    .setSuper(toAbstractEventWithStateProto())
-                    .build();
-        }
-
-        @Nonnull
-        @Override
-        public PEvent.Builder toEventBuilder() {
-            return PEvent.newBuilder()
-                    .setFinalizePlannerPhaseEvent(toProto());
-        }
-    }
-
     /**
      * Events of this class are generated every time the planner executes a task.
      */

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/debug/Debugger.java

@@ -41,7 +41,6 @@ message PEvent {
     PInsertIntoMemoEvent insert_into_memo_event = 11;
     PTranslateCorrelationsEvent translate_correlations_event = 12;
     PInitiatePlannerPhaseEvent initiate_planner_phase_event = 13;
-    PFinalizePlannerPhaseEvent finalize_planner_phase_event = 14;
   }
 }
 
@@ -106,10 +105,6 @@ message PInitiatePlannerPhaseEvent {
   optional PAbstractEventWithState super = 1;
 }
 
-message PFinalizePlannerPhaseEvent {
-  optional PAbstractEventWithState super = 1;
-}
-
 message PInsertIntoMemoEvent {
   optional string location = 1;
   optional PRegisteredRelationalExpression expression = 2;

fdb-record-layer-core/src/main/proto/planner_debugger.proto

@@ -168,7 +168,7 @@ public QueryPlanner planner(@Nonnull BenchmarkTimer timer, boolean cascades) {
         final RecordMetaData recordMetaData = recordStoreBuilder.getMetaDataProvider().getRecordMetaData();
         final RecordStoreState recordStoreState = new RecordStoreState(null, null);
         if (cascades) {
-            return new CascadesPlanner(recordMetaData, recordStoreState, timer.getTimer());
+            return new CascadesPlanner(recordMetaData, recordStoreState);
         } else {
             return new RecordQueryPlanner(recordMetaData, recordStoreState, timer.getTimer());
         }

fdb-record-layer-jmh/src/jmh/java/com/apple/foundationdb/record/benchmark/BenchmarkRecordStore.java

@@ -20,15 +20,12 @@
 
 package com.apple.foundationdb.relational.api.metrics;
 
-import com.apple.foundationdb.record.provider.common.StoreTimer;
 import com.apple.foundationdb.relational.api.exceptions.ErrorCode;
 import com.apple.foundationdb.relational.api.exceptions.RelationalException;
 import com.apple.foundationdb.relational.util.Supplier;
 
 import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
 import java.util.Locale;
-import java.util.Optional;
 
 /**
  * MetricCollector provides a utility API to register events and counts while performing operations in the Relational
@@ -99,15 +96,4 @@ default boolean hasCounter(@Nonnull RelationalMetric.RelationalCount count) {
      * */
     default void flush() {
     }
-
-    /**
-     * Return an instance of {@link com.apple.foundationdb.record.provider.common.StoreTimer} that corresponds to
-     * this metric collector, if possible.
-     *
-     * @return an instance of {@link com.apple.foundationdb.record.provider.common.StoreTimer} or an empty {@link Optional}
-     */
-    @Nullable
-    default StoreTimer getRecordLayerStoreTimer() {
-        return null;
-    }
 }

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/api/metrics/MetricCollector.java

@@ -31,6 +31,8 @@
 import com.apple.foundationdb.relational.util.Assert;
 import com.apple.foundationdb.relational.util.Supplier;
 
+import com.google.common.annotations.VisibleForTesting;
+
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import java.util.Objects;
@@ -96,12 +98,12 @@ public boolean hasCounter(@Nonnull RelationalMetric.RelationalCount count) {
 
     @Nullable
     private StoreTimer.Counter getCounter(@Nonnull RelationalMetric.RelationalCount count) {
-        return Objects.requireNonNull(getRecordLayerStoreTimer()).getCounter(count);
+        return getUnderlyingStoreTimer().getCounter(count);
     }
 
-    @Override
-    @Nullable
-    public StoreTimer getRecordLayerStoreTimer() {
-        return context.getTimer();
+    @Nonnull
+    @VisibleForTesting
+    public StoreTimer getUnderlyingStoreTimer() {
+        return Objects.requireNonNull(context.getTimer());
     }
 }

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/metric/RecordLayerMetricCollector.java

@@ -126,7 +126,7 @@ public Plan<?> getPlan(@Nonnull final String query) throws RelationalException {
             exception = e;
             throw e;
         } finally {
-            RelationalLoggingUtil.publishPlanGenerationLogs(logger, message, plan, exception, planContext.getMetricsCollector().getRecordLayerStoreTimer(), totalTimeMicros(), options);
+            RelationalLoggingUtil.publishPlanGenerationLogs(logger, message, plan, exception, totalTimeMicros(), options);
         }
         return plan;
     }
@@ -390,10 +390,7 @@ private static boolean shouldNotCache(@Nonnull final Set<AstNormalizer.Normaliza
     private static CascadesPlanner createPlanner(@Nonnull final RecordMetaData metaData,
                                                  @Nonnull final RecordStoreState recordStoreState,
                                                  @Nonnull final PlanContext planContext) {
-        final var planner = new CascadesPlanner(
-                metaData,
-                recordStoreState,
-                planContext.getMetricsCollector().getRecordLayerStoreTimer());
+        final var planner = new CascadesPlanner(metaData, recordStoreState);
         planner.setConfiguration(planContext.getRecordQueryPlannerConfiguration());
         return planner;
     }