Skip to content

Commit

Permalink
Make PlanOptimizers stateless
Browse files Browse the repository at this point in the history
The change in f3dda9c caused mbeans
to be exported unconditionally the first time PlanOptimizers.get() is
called. LocalQueryRunner.getPlanOptimizers creates a new instance
every time, so this causes the mbeans to be exported on every
invocation and results in tests spending the majority of their time
in mbean export.

This change moves the responsibility of exporting/unexporting the
optimizer mbeans to a separate class.
  • Loading branch information
martint committed Apr 30, 2021
1 parent d69b6c9 commit 9777b46
Show file tree
Hide file tree
Showing 8 changed files with 133 additions and 116 deletions.
Expand Up @@ -81,10 +81,10 @@
import io.trino.spi.memory.ClusterMemoryPoolManager;
import io.trino.spi.security.SelectedRole;
import io.trino.sql.analyzer.QueryExplainer;
import io.trino.sql.planner.OptimizerStatsMBeanExporter;
import io.trino.sql.planner.PlanFragmenter;
import io.trino.sql.planner.PlanOptimizers;
import io.trino.sql.planner.PlanOptimizersFactory;
import io.trino.sql.planner.RuleStatsRecorder;
import io.trino.transaction.ForTransactionManager;
import io.trino.transaction.InMemoryTransactionManager;
import io.trino.transaction.TransactionManager;
Expand Down Expand Up @@ -216,8 +216,8 @@ protected void setup(Binder binder)
newOptionalBinder(binder, PlanOptimizersFactory.class)
.setDefault().to(PlanOptimizers.class).in(Scopes.SINGLETON);

// Rule Stats Recorder
binder.bind(RuleStatsRecorder.class).in(Scopes.SINGLETON);
// Optimizer/Rule Stats exporter
binder.bind(OptimizerStatsMBeanExporter.class).in(Scopes.SINGLETON);

// query explainer
binder.bind(QueryExplainer.class).in(Scopes.SINGLETON);
Expand Down
@@ -0,0 +1,105 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.trino.sql.planner;

import com.google.common.collect.ImmutableMap;
import io.trino.sql.planner.iterative.IterativeOptimizer;
import io.trino.sql.planner.iterative.RuleStats;
import io.trino.sql.planner.optimizations.OptimizerStats;
import io.trino.sql.planner.optimizations.PlanOptimizer;
import org.weakref.jmx.MBeanExport;
import org.weakref.jmx.MBeanExporter;
import org.weakref.jmx.ObjectNames;

import javax.annotation.PostConstruct;
import javax.annotation.PreDestroy;
import javax.annotation.concurrent.GuardedBy;
import javax.inject.Inject;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Verify.verify;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class OptimizerStatsMBeanExporter
{
@GuardedBy("this")
private final List<MBeanExport> mbeanExports = new ArrayList<>();

private final MBeanExporter exporter;
private final Map<Class<?>, OptimizerStats> optimizerStats;
private final Map<Class<?>, RuleStats> ruleStats;

@Inject
public OptimizerStatsMBeanExporter(MBeanExporter exporter, PlanOptimizersFactory optimizers)
{
requireNonNull(optimizers, "optimizers is null");
optimizerStats = optimizers.getOptimizerStats();
ruleStats = optimizers.getRuleStats();

this.exporter = requireNonNull(exporter, "exporter is null");
}

@PostConstruct
public synchronized void export()
{
checkState(mbeanExports.isEmpty(), "MBeans already exported");

for (Map.Entry<Class<?>, OptimizerStats> entry : optimizerStats.entrySet()) {
verify(!entry.getKey().getSimpleName().isEmpty());
try {
mbeanExports.add(exporter.exportWithGeneratedName(entry.getValue(), PlanOptimizer.class, ImmutableMap.<String, String>builder()
.put("name", PlanOptimizer.class.getSimpleName())
.put("optimizer", entry.getKey().getSimpleName())
.build()));
}
catch (RuntimeException e) {
throw new RuntimeException(format("Failed to export MBean with name '%s'", getName(entry.getKey())), e);
}
}

for (Map.Entry<Class<?>, RuleStats> entry : ruleStats.entrySet()) {
verify(!entry.getKey().getSimpleName().isEmpty());
try {
mbeanExports.add(exporter.exportWithGeneratedName(entry.getValue(), IterativeOptimizer.class, ImmutableMap.<String, String>builder()
.put("name", IterativeOptimizer.class.getSimpleName())
.put("rule", entry.getKey().getSimpleName())
.build()));
}
catch (RuntimeException e) {
throw new RuntimeException(format("Failed to export MBean with for rule '%s'", entry.getKey().getSimpleName()), e);
}
}
}

@PreDestroy
public synchronized void unexport()
{
for (MBeanExport mbeanExport : mbeanExports) {
mbeanExport.unexport();
}
mbeanExports.clear();
}

private String getName(Class<?> key)
{
return ObjectNames.builder(PlanOptimizer.class)
.withProperty("optimizer", key.getSimpleName())
.build();
}
}
Expand Up @@ -13,40 +13,32 @@
*/
package io.trino.sql.planner;

import com.google.common.collect.ImmutableMap;
import io.trino.sql.planner.optimizations.OptimizerStats;
import io.trino.sql.planner.optimizations.PlanOptimizer;
import org.weakref.jmx.MBeanExport;
import org.weakref.jmx.MBeanExporter;
import org.weakref.jmx.ObjectNames;

import javax.annotation.concurrent.GuardedBy;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Verify.verify;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class OptimizerStatsRecorder
{
private final Map<Class<?>, OptimizerStats> stats = new HashMap<>();

@GuardedBy("this")
private final List<MBeanExport> mbeanExports = new ArrayList<>();

public void register(PlanOptimizer optimizer)
{
requireNonNull(optimizer, "optimizer is null");
checkArgument(!optimizer.getClass().isAnonymousClass());
stats.put(optimizer.getClass(), new OptimizerStats());
}

public Map<Class<?>, OptimizerStats> getStats()
{
return Collections.unmodifiableMap(stats);
}

public void record(PlanOptimizer optimizer, long nanos)
{
requireNonNull(optimizer, "optimizer is null");
Expand All @@ -60,36 +52,4 @@ public void recordFailure(PlanOptimizer optimizer)
OptimizerStats optimizerStats = requireNonNull(stats.get(optimizer.getClass()), "optimizer is not registered");
optimizerStats.recordFailure();
}

public synchronized void export(MBeanExporter exporter)
{
checkState(mbeanExports.isEmpty(), "MBeans already exported");
for (Map.Entry<Class<?>, OptimizerStats> entry : stats.entrySet()) {
verify(!entry.getKey().getSimpleName().isEmpty());
try {
mbeanExports.add(exporter.exportWithGeneratedName(entry.getValue(), PlanOptimizer.class, ImmutableMap.<String, String>builder()
.put("name", PlanOptimizer.class.getSimpleName())
.put("optimizer", entry.getKey().getSimpleName())
.build()));
}
catch (RuntimeException e) {
throw new RuntimeException(format("Failed to export MBean with name '%s'", getName(entry.getKey())), e);
}
}
}

public synchronized void unexport(MBeanExporter exporter)
{
for (MBeanExport mbeanExport : mbeanExports) {
mbeanExport.unexport();
}
mbeanExports.clear();
}

private String getName(Class<?> key)
{
return ObjectNames.builder(PlanOptimizer.class)
.withProperty("optimizer", key.getSimpleName())
.build();
}
}
Expand Up @@ -28,6 +28,7 @@
import io.trino.split.SplitManager;
import io.trino.sql.planner.iterative.IterativeOptimizer;
import io.trino.sql.planner.iterative.Rule;
import io.trino.sql.planner.iterative.RuleStats;
import io.trino.sql.planner.iterative.rule.AddExchangesBelowPartialAggregationOverGroupIdRuleSet;
import io.trino.sql.planner.iterative.rule.AddIntermediateAggregations;
import io.trino.sql.planner.iterative.rule.ApplyTableScanRedirection;
Expand Down Expand Up @@ -218,6 +219,7 @@
import io.trino.sql.planner.optimizations.LimitPushDown;
import io.trino.sql.planner.optimizations.MetadataQueryOptimizer;
import io.trino.sql.planner.optimizations.OptimizeMixedDistinctAggregations;
import io.trino.sql.planner.optimizations.OptimizerStats;
import io.trino.sql.planner.optimizations.PlanOptimizer;
import io.trino.sql.planner.optimizations.PredicatePushDown;
import io.trino.sql.planner.optimizations.PruneUnreferencedOutputs;
Expand All @@ -229,10 +231,10 @@
import io.trino.sql.planner.optimizations.WindowFilterPushDown;
import org.weakref.jmx.MBeanExporter;

import javax.annotation.PreDestroy;
import javax.inject.Inject;

import java.util.List;
import java.util.Map;
import java.util.Set;

import static io.trino.SystemSessionProperties.isIterativeRuleBasedColumnPruning;
Expand All @@ -241,11 +243,8 @@ public class PlanOptimizers
implements PlanOptimizersFactory
{
private final List<PlanOptimizer> optimizers;
private final RuleStatsRecorder ruleStats;
private final RuleStatsRecorder ruleStats = new RuleStatsRecorder();
private final OptimizerStatsRecorder optimizerStats = new OptimizerStatsRecorder();
private final MBeanExporter exporter;

private boolean initialized;

@Inject
public PlanOptimizers(
Expand All @@ -261,7 +260,6 @@ public PlanOptimizers(
@EstimatedExchanges CostCalculator estimatedExchangesCostCalculator,
CostComparator costComparator,
TaskCountEstimator taskCountEstimator,
RuleStatsRecorder ruleStats,
NodePartitioningManager nodePartitioningManager)
{
this(metadata,
Expand All @@ -277,17 +275,9 @@ public PlanOptimizers(
estimatedExchangesCostCalculator,
costComparator,
taskCountEstimator,
ruleStats,
nodePartitioningManager);
}

@PreDestroy
public void destroy()
{
ruleStats.unexport(exporter);
optimizerStats.unexport(exporter);
}

public PlanOptimizers(
Metadata metadata,
TypeOperators typeOperators,
Expand All @@ -302,11 +292,8 @@ public PlanOptimizers(
CostCalculator estimatedExchangesCostCalculator,
CostComparator costComparator,
TaskCountEstimator taskCountEstimator,
RuleStatsRecorder ruleStats,
NodePartitioningManager nodePartitioningManager)
{
this.ruleStats = ruleStats;
this.exporter = exporter;
ImmutableList.Builder<PlanOptimizer> builder = ImmutableList.builder();

Set<Rule<?>> columnPruningRules = ImmutableSet.of(
Expand Down Expand Up @@ -894,18 +881,18 @@ public PlanOptimizers(
@Override
public List<PlanOptimizer> get()
{
initialize();
return optimizers;
}

private synchronized void initialize()
@Override
public Map<Class<?>, OptimizerStats> getOptimizerStats()
{
if (initialized) {
return;
}
return optimizerStats.getStats();
}

ruleStats.export(exporter);
optimizerStats.export(exporter);
initialized = true;
@Override
public Map<Class<?>, RuleStats> getRuleStats()
{
return ruleStats.getStats();
}
}
Expand Up @@ -13,11 +13,18 @@
*/
package io.trino.sql.planner;

import io.trino.sql.planner.iterative.RuleStats;
import io.trino.sql.planner.optimizations.OptimizerStats;
import io.trino.sql.planner.optimizations.PlanOptimizer;

import java.util.List;
import java.util.Map;

public interface PlanOptimizersFactory
{
List<PlanOptimizer> get();

Map<Class<?>, OptimizerStats> getOptimizerStats();

Map<Class<?>, RuleStats> getRuleStats();
}

0 comments on commit 9777b46

Please sign in to comment.