Skip to content

Skip history optimizer query registration if previous registration timeout #21308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
feilong-liu merged 2 commits into from
Nov 9, 2023
Merged

Skip history optimizer query registration if previous registration timeout #21308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2ab440
Remove invalid query ID cache from HBO cache manager
feilong-liu Nov 3, 2023
16eee6d
Skip HBO registration if previous registration timeout
feilong-liu Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,15 @@ public PlanNodeStatsEstimate calculateStats(PlanNode node, StatsProvider sourceS
@Override
public boolean registerPlan(PlanNode root, Session session, long startTimeInNano, long timeoutInMilliseconds)
{
// If previous registration timeout for this query, this run is likely to timeout too, return false.
if (historyBasedStatisticsCacheManager.historyBasedQueryRegistrationTimeout(session.getQueryId())) {
return false;
}
ImmutableList.Builder<PlanNodeWithHash> planNodesWithHash = ImmutableList.builder();
Iterable<PlanNode> planNodeIterable = forTree(PlanNode::getSources).depthFirstPreOrder(root);
for (PlanNode plan : planNodeIterable) {
if (checkTimeOut(startTimeInNano, timeoutInMilliseconds)) {
historyBasedStatisticsCacheManager.setHistoryBasedQueryRegistrationTimeout(session.getQueryId());
return false;
}
if (plan.getStatsEquivalentPlanNode().isPresent()) {
Expand All @@ -95,6 +100,10 @@ public boolean registerPlan(PlanNode root, Session session, long startTimeInNano
catch (ExecutionException e) {
throw new RuntimeException("Unable to register plan: ", e.getCause());
}

if (checkTimeOut(startTimeInNano, timeoutInMilliseconds)) {
historyBasedStatisticsCacheManager.setHistoryBasedQueryRegistrationTimeout(session.getQueryId());
}
// Return true even if get empty history statistics, so that HistoricalStatisticsEquivalentPlanMarkingOptimizer still return the plan with StatsEquivalentPlanNode which
// will be used in populating history statistics
return true;
Expand Down Expand Up @@ -142,7 +151,7 @@ private Map<PlanCanonicalizationStrategy, PlanNodeWithHash> getPlanNodeHashes(Pl

private PlanNodeStatsEstimate getStatistics(PlanNode planNode, Session session, Lookup lookup, PlanNodeStatsEstimate delegateStats)
{
if (!useHistoryBasedPlanStatisticsEnabled(session) || historyBasedStatisticsCacheManager.loadHistoryFailed(session.getQueryId())) {
Copy link
Contributor Author

@feilong-liu feilong-liu Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadHistoryFailed is a macro optimization for HBO latency added in #19933. For queries which do not load any history statistics, we will skip attempt to read history stats. However, by the design, all statistics are cached during query registration, and the read here will be reading from cache, hence the saving is small. On the other hand, we run HBO optimizer twice, having one run reading empty does not mean the second time will still be empty. Hence remove this macro optimization here.

if (!useHistoryBasedPlanStatisticsEnabled(session)) {
return delegateStats;
}

Expand Down
26 changes: 10 additions & 16 deletions presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedStatisticsCacheManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ public class HistoryBasedStatisticsCacheManager

private final Map<QueryId, Map<CachingPlanCanonicalInfoProvider.InputTableCacheKey, PlanStatistics>> inputTableStatistics = new ConcurrentHashMap<>();

// Whether the history is loaded successfully
private final Set<QueryId> invalidQueryId = ConcurrentHashMap.newKeySet();
// Stores query IDs which timeout during history optimizer registration
private final Set<QueryId> queryIdsRegistrationTimeOut = ConcurrentHashMap.newKeySet();

public HistoryBasedStatisticsCacheManager() {}

Expand All @@ -59,24 +59,13 @@ public LoadingCache<PlanNodeWithHash, HistoricalPlanStatistics> getStatisticsCac
@Override
public HistoricalPlanStatistics load(PlanNodeWithHash key)
{
if (invalidQueryId.contains(queryId)) {
return empty();
}
return loadAll(Collections.singleton(key)).values().stream().findAny().orElseGet(HistoricalPlanStatistics::empty);
}

@Override
public Map<PlanNodeWithHash, HistoricalPlanStatistics> loadAll(Iterable<? extends PlanNodeWithHash> keys)
{
if (invalidQueryId.contains(queryId)) {
Map<PlanNodeWithHash, HistoricalPlanStatistics> emptyResult = new HashMap<>();
keys.forEach(x -> emptyResult.put(x, empty()));
return emptyResult;
}
Map<PlanNodeWithHash, HistoricalPlanStatistics> statistics = new HashMap<>(historyBasedPlanStatisticsProvider.get().getStats(ImmutableList.copyOf(keys), timeoutInMilliSeconds));
if (statistics.isEmpty()) {
invalidQueryId.add(queryId);
}
// loadAll excepts all keys to be written
for (PlanNodeWithHash key : keys) {
statistics.putIfAbsent(key, empty());
Expand All @@ -101,7 +90,7 @@ public void invalidate(QueryId queryId)
statisticsCache.remove(queryId);
canonicalInfoCache.remove(queryId);
inputTableStatistics.remove(queryId);
invalidQueryId.remove(queryId);
queryIdsRegistrationTimeOut.remove(queryId);
}

@VisibleForTesting
Expand All @@ -110,8 +99,13 @@ public Map<QueryId, Map<CachingPlanCanonicalInfoProvider.CacheKey, PlanNodeCanon
return canonicalInfoCache;
}

public boolean loadHistoryFailed(QueryId queryId)
public boolean historyBasedQueryRegistrationTimeout(QueryId queryId)
{
return queryIdsRegistrationTimeOut.contains(queryId);
}

public void setHistoryBasedQueryRegistrationTimeout(QueryId queryId)
{
return invalidQueryId.contains(queryId);
queryIdsRegistrationTimeOut.add(queryId);
}
}