Skip to content

Commit 260e2b1

Browse files
hazefullyhazefully
committed
Revert "Introduce new timers for the different phases of the Cascades planner"
This reverts commit 7e8cc8e.
1 parent 12b5817 commit 260e2b1

File tree

11 files changed

+16
-203
lines changed

11 files changed

+16
-203
lines changed

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

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import com.apple.foundationdb.record.RecordStoreState;
2727
import com.apple.foundationdb.record.logging.KeyValueLogMessage;
2828
import com.apple.foundationdb.record.logging.LogMessageKeys;
29-
import com.apple.foundationdb.record.provider.common.StoreTimer;
3029
import com.apple.foundationdb.record.query.IndexQueryabilityFilter;
3130
import com.apple.foundationdb.record.query.ParameterRelationshipGraph;
3231
import com.apple.foundationdb.record.query.RecordQuery;
@@ -56,7 +55,6 @@
5655
import org.slf4j.LoggerFactory;
5756

5857
import javax.annotation.Nonnull;
59-
import javax.annotation.Nullable;
6058
import java.util.ArrayDeque;
6159
import java.util.Collection;
6260
import java.util.Deque;
@@ -214,8 +212,6 @@ public class CascadesPlanner implements QueryPlanner {
214212
private final RecordMetaData metaData;
215213
@Nonnull
216214
private final RecordStoreState recordStoreState;
217-
@Nullable
218-
private final StoreTimer storeTimer;
219215
@Nonnull
220216
private Reference currentRoot;
221217
@Nonnull
@@ -232,14 +228,9 @@ public class CascadesPlanner implements QueryPlanner {
232228
private int maxQueueSize;
233229

234230
public CascadesPlanner(@Nonnull RecordMetaData metaData, @Nonnull RecordStoreState recordStoreState) {
235-
this(metaData, recordStoreState, null);
236-
}
237-
238-
public CascadesPlanner(@Nonnull RecordMetaData metaData, @Nonnull RecordStoreState recordStoreState, @Nullable StoreTimer storeTimer) {
239231
this.configuration = RecordQueryPlannerConfiguration.builder().build();
240232
this.metaData = metaData;
241233
this.recordStoreState = recordStoreState;
242-
this.storeTimer = storeTimer;
243234
// Placeholders until we get a query.
244235
this.currentRoot = Reference.empty();
245236
this.planContext = PlanContext.emptyContext();
@@ -533,41 +524,6 @@ public interface Task {
533524
Debugger.Event toTaskEvent(Location location);
534525
}
535526

536-
private class FinalizePlannerPhase implements Task {
537-
@Nonnull
538-
private final PlannerPhase plannerPhase;
539-
private final long phaseStartTime;
540-
541-
public FinalizePlannerPhase(@Nonnull final PlannerPhase plannerPhase, long phaseStartTime) {
542-
this.plannerPhase = plannerPhase;
543-
this.phaseStartTime = phaseStartTime;
544-
}
545-
546-
@Override
547-
@Nonnull
548-
public PlannerPhase getPlannerPhase() {
549-
return plannerPhase;
550-
}
551-
552-
@Override
553-
public void execute() {
554-
if (storeTimer != null) {
555-
storeTimer.record(plannerPhase.getPhaseCompletionTimerEvent(), System.nanoTime() - phaseStartTime);
556-
}
557-
}
558-
559-
@Override
560-
@Nonnull
561-
public Debugger.Event toTaskEvent(final Location location) {
562-
return new Debugger.FinalizePlannerPhaseEvent(plannerPhase, currentRoot, taskStack, location);
563-
}
564-
565-
@Override
566-
public String toString() {
567-
return "FinalizePlannerPhase(" + plannerPhase.name() + ")";
568-
}
569-
}
570-
571527
/**
572528
* Globally initiate a new planner phase.
573529
* Simplified push/execute overview:
@@ -596,12 +552,10 @@ public PlannerPhase getPlannerPhase() {
596552

597553
@Override
598554
public void execute() {
599-
var phaseStartTime = System.nanoTime();
600555
if (plannerPhase.hasNextPhase()) {
601556
// if there is another phase push it first so it gets executed at the very end
602557
taskStack.push(new InitiatePlannerPhase(plannerPhase.getNextPhase()));
603558
}
604-
taskStack.push(new FinalizePlannerPhase(plannerPhase, phaseStartTime));
605559
taskStack.push(new OptimizeGroup(plannerPhase, currentRoot));
606560
taskStack.push(new ExploreGroup(plannerPhase, currentRoot));
607561
}

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

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@
3535
*/
3636
public enum PlannerPhase {
3737
// note that the phase are declared in a counterintuitive inverse way since a phase has to specify the next phase
38-
PLANNING(PlanningRuleSet.getDefault(), PlannerStage.PLANNED, PlanningCostModel::new, PlannerTimerEvents.PLANNING_PHASE_COMPLETE),
39-
REWRITING(RewritingRuleSet.getDefault(), PlannerStage.CANONICAL, RewritingCostModel::new, PlannerTimerEvents.REWRITING_PHASE_COMPLETE, PLANNING);
38+
PLANNING(PlanningRuleSet.getDefault(), PlannerStage.PLANNED, PlanningCostModel::new),
39+
REWRITING(RewritingRuleSet.getDefault(), PlannerStage.CANONICAL, RewritingCostModel::new, PLANNING);
4040

4141
@Nonnull
4242
private final CascadesRuleSet ruleSet;
@@ -46,26 +46,21 @@ public enum PlannerPhase {
4646
private final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator;
4747
@Nullable
4848
private final PlannerPhase nextPhase;
49-
@Nonnull
50-
private final PlannerTimerEvents phaseCompletionTimerEvent;
5149

5250
PlannerPhase(@Nonnull final CascadesRuleSet ruleSet,
5351
@Nonnull final PlannerStage targetStage,
54-
@Nonnull final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator,
55-
@Nonnull final PlannerTimerEvents phaseCompletionTimerEvent) {
56-
this(ruleSet, targetStage, costModelCreator, phaseCompletionTimerEvent, null);
52+
@Nonnull final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator) {
53+
this(ruleSet, targetStage, costModelCreator, null);
5754
}
5855

5956
PlannerPhase(@Nonnull final CascadesRuleSet ruleSet,
6057
@Nonnull final PlannerStage targetStage,
6158
@Nonnull final Function<RecordQueryPlannerConfiguration, CascadesCostModel> costModelCreator,
62-
@Nonnull final PlannerTimerEvents phaseCompletionTimerEvent,
6359
@Nullable final PlannerPhase nextPhase) {
6460
this.ruleSet = ruleSet;
6561
this.targetStage = targetStage;
6662
this.costModelCreator = costModelCreator;
6763
this.nextPhase = nextPhase;
68-
this.phaseCompletionTimerEvent = phaseCompletionTimerEvent;
6964
}
7065

7166
@Nonnull
@@ -92,11 +87,6 @@ public boolean hasNextPhase() {
9287
return nextPhase != null;
9388
}
9489

95-
@Nonnull
96-
public PlannerTimerEvents getPhaseCompletionTimerEvent() {
97-
return phaseCompletionTimerEvent;
98-
}
99-
10090
@Nonnull
10191
public PPlannerPhase toProto() {
10292
switch (this) {

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

Lines changed: 0 additions & 49 deletions
This file was deleted.

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

Lines changed: 1 addition & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PExecutingTaskEvent;
3737
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PExploreExpressionEvent;
3838
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PExploreGroupEvent;
39-
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PFinalizePlannerPhaseEvent;
4039
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PInitiatePlannerPhaseEvent;
4140
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PInsertIntoMemoEvent;
4241
import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PMatchPartition;
@@ -261,8 +260,7 @@ enum Shorthand {
261260
TRANSFORM,
262261
INSERT_INTO_MEMO,
263262
TRANSLATE_CORRELATIONS,
264-
INITPHASE,
265-
FINALIZEPHASE
263+
INITPHASE
266264
}
267265

268266
/**
@@ -531,42 +529,6 @@ public PEvent.Builder toEventBuilder() {
531529
}
532530
}
533531

534-
class FinalizePlannerPhaseEvent extends AbstractEventWithState {
535-
public FinalizePlannerPhaseEvent(@Nonnull final PlannerPhase plannerPhase,
536-
@Nonnull final Reference rootReference,
537-
@Nonnull final Deque<Task> taskStack,
538-
@Nonnull final Location location) {
539-
super(plannerPhase, rootReference, taskStack, location);
540-
}
541-
542-
@Override
543-
@Nonnull
544-
public String getDescription() {
545-
return "finalizing planner phase " + getPlannerPhase().name();
546-
}
547-
548-
@Override
549-
@Nonnull
550-
public Shorthand getShorthand() {
551-
return Shorthand.FINALIZEPHASE;
552-
}
553-
554-
@Nonnull
555-
@Override
556-
public PFinalizePlannerPhaseEvent toProto() {
557-
return PFinalizePlannerPhaseEvent.newBuilder()
558-
.setSuper(toAbstractEventWithStateProto())
559-
.build();
560-
}
561-
562-
@Nonnull
563-
@Override
564-
public PEvent.Builder toEventBuilder() {
565-
return PEvent.newBuilder()
566-
.setFinalizePlannerPhaseEvent(toProto());
567-
}
568-
}
569-
570532
/**
571533
* Events of this class are generated every time the planner executes a task.
572534
*/

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ message PEvent {
4141
PInsertIntoMemoEvent insert_into_memo_event = 11;
4242
PTranslateCorrelationsEvent translate_correlations_event = 12;
4343
PInitiatePlannerPhaseEvent initiate_planner_phase_event = 13;
44-
PFinalizePlannerPhaseEvent finalize_planner_phase_event = 14;
4544
}
4645
}
4746

@@ -106,10 +105,6 @@ message PInitiatePlannerPhaseEvent {
106105
optional PAbstractEventWithState super = 1;
107106
}
108107

109-
message PFinalizePlannerPhaseEvent {
110-
optional PAbstractEventWithState super = 1;
111-
}
112-
113108
message PInsertIntoMemoEvent {
114109
optional string location = 1;
115110
optional PRegisteredRelationalExpression expression = 2;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ public QueryPlanner planner(@Nonnull BenchmarkTimer timer, boolean cascades) {
168168
final RecordMetaData recordMetaData = recordStoreBuilder.getMetaDataProvider().getRecordMetaData();
169169
final RecordStoreState recordStoreState = new RecordStoreState(null, null);
170170
if (cascades) {
171-
return new CascadesPlanner(recordMetaData, recordStoreState, timer.getTimer());
171+
return new CascadesPlanner(recordMetaData, recordStoreState);
172172
} else {
173173
return new RecordQueryPlanner(recordMetaData, recordStoreState, timer.getTimer());
174174
}

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

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,12 @@
2020

2121
package com.apple.foundationdb.relational.api.metrics;
2222

23-
import com.apple.foundationdb.record.provider.common.StoreTimer;
2423
import com.apple.foundationdb.relational.api.exceptions.ErrorCode;
2524
import com.apple.foundationdb.relational.api.exceptions.RelationalException;
2625
import com.apple.foundationdb.relational.util.Supplier;
2726

2827
import javax.annotation.Nonnull;
29-
import javax.annotation.Nullable;
3028
import java.util.Locale;
31-
import java.util.Optional;
3229

3330
/**
3431
* 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) {
9996
* */
10097
default void flush() {
10198
}
102-
103-
/**
104-
* Return an instance of {@link com.apple.foundationdb.record.provider.common.StoreTimer} that corresponds to
105-
* this metric collector, if possible.
106-
*
107-
* @return an instance of {@link com.apple.foundationdb.record.provider.common.StoreTimer} or an empty {@link Optional}
108-
*/
109-
@Nullable
110-
default StoreTimer getRecordLayerStoreTimer() {
111-
return null;
112-
}
11399
}

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
import com.apple.foundationdb.relational.util.Assert;
3232
import com.apple.foundationdb.relational.util.Supplier;
3333

34+
import com.google.common.annotations.VisibleForTesting;
35+
3436
import javax.annotation.Nonnull;
3537
import javax.annotation.Nullable;
3638
import java.util.Objects;
@@ -96,12 +98,12 @@ public boolean hasCounter(@Nonnull RelationalMetric.RelationalCount count) {
9698

9799
@Nullable
98100
private StoreTimer.Counter getCounter(@Nonnull RelationalMetric.RelationalCount count) {
99-
return Objects.requireNonNull(getRecordLayerStoreTimer()).getCounter(count);
101+
return getUnderlyingStoreTimer().getCounter(count);
100102
}
101103

102-
@Override
103-
@Nullable
104-
public StoreTimer getRecordLayerStoreTimer() {
105-
return context.getTimer();
104+
@Nonnull
105+
@VisibleForTesting
106+
public StoreTimer getUnderlyingStoreTimer() {
107+
return Objects.requireNonNull(context.getTimer());
106108
}
107109
}

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/PlanGenerator.java

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public Plan<?> getPlan(@Nonnull final String query) throws RelationalException {
126126
exception = e;
127127
throw e;
128128
} finally {
129-
RelationalLoggingUtil.publishPlanGenerationLogs(logger, message, plan, exception, planContext.getMetricsCollector().getRecordLayerStoreTimer(), totalTimeMicros(), options);
129+
RelationalLoggingUtil.publishPlanGenerationLogs(logger, message, plan, exception, totalTimeMicros(), options);
130130
}
131131
return plan;
132132
}
@@ -390,10 +390,7 @@ private static boolean shouldNotCache(@Nonnull final Set<AstNormalizer.Normaliza
390390
private static CascadesPlanner createPlanner(@Nonnull final RecordMetaData metaData,
391391
@Nonnull final RecordStoreState recordStoreState,
392392
@Nonnull final PlanContext planContext) {
393-
final var planner = new CascadesPlanner(
394-
metaData,
395-
recordStoreState,
396-
planContext.getMetricsCollector().getRecordLayerStoreTimer());
393+
final var planner = new CascadesPlanner(metaData, recordStoreState);
397394
planner.setConfiguration(planContext.getRecordQueryPlannerConfiguration());
398395
return planner;
399396
}

0 commit comments

Comments
 (0)