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

Jump to bottom

Aggressively prune query info after completion #23257

Merged

tdcmeehan merged 1 commit into prestodb:master from ZacBlanco:upstream-prune-query-info

Aug 19, 2024

Contributor

ZacBlanco commented Jul 19, 2024 •

edited

Loading

Description

This change aggressively prunes query plan metadata before query expiry. It is mostly limited to query plan statistics.

With this change and with histograms enabled in #22365, this cuts the coordinator heap utilization by anywhere between 50% and 75% after running a portion of the TPC-DS benchmark (3-4GiB -> 600-800MiB)

Motivation and Context

Previously, we only pruned information from old queries after the expiration queue filled. With the addition of histograms query statistics can take two orders of magnitude more memory than previous statistics. Without pruning statistics we will much more quickly hit memory limits of the JVM.

Specifically, these are the areas where we try to clear references to save memory:

Session: planNodeStatsMap and planCostMapMap
QueryStateMachine: clear statistics from inputs and planStatsAndCosts
StateMachine: Once final queryInfo reaches a terminal state, we should clear the event listeners to release references to local variables which maintain the SqlQuerySchedule and Stage execution information
Prune all histograms from finalQueryInfo's planStatsAndCosts and from the entire StageInfo graph.

Fixes #23254

Impact

Results from the QueryInfo API after execution finishes will always return column statistics with empty histograms

Test Plan

Added a new test which verifies that a query completed event is generated before all queryInfo pruning is done

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== NO RELEASE NOTE ==

ZacBlanco changed the title ~~Aggressively prune statistics from query info after completion~~ Aggressively prunequery info after completion

ZacBlanco changed the title ~~Aggressively prunequery info after completion~~ Aggressively prune query info after completion

ZacBlanco force-pushed the upstream-prune-query-info branch 2 times, most recently from 9165744 to 6cfe291 Compare

July 19, 2024 07:00

ZacBlanco marked this pull request as ready for review

July 19, 2024 20:53

ZacBlanco requested a review from a team as a code owner

July 19, 2024 20:53

ZacBlanco requested a review from presto-oss

July 19, 2024 20:53

aaneja reviewed

View reviewed changes

Contributor

aaneja left a comment

Can you check (and maybe add/link me to a test that asserts) that the queryCompletedEvent is fired before the query info pruning is done ?

ZacBlanco force-pushed the upstream-prune-query-info branch from 6cfe291 to 0ebbd10 Compare

July 24, 2024 23:27

ZacBlanco requested review from jaystarshot, feilong-liu, elharo and ClarenceThreepwood as code owners

July 24, 2024 23:27

ZacBlanco commented

View reviewed changes

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java Show resolved Hide resolved

ZacBlanco force-pushed the upstream-prune-query-info branch from 0ebbd10 to 20af877 Compare

July 24, 2024 23:52

tdcmeehan self-assigned this

ZacBlanco force-pushed the upstream-prune-query-info branch from 20af877 to 735877a Compare

July 25, 2024 01:35

aaneja previously approved these changes

View reviewed changes

hantangwangd previously approved these changes

View reviewed changes

Member

hantangwangd left a comment

LGTM, just one nit.

presto-main/src/main/java/com/facebook/presto/execution/QueryStateMachine.java Outdated Show resolved Hide resolved

elharo reviewed

View reviewed changes

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

+                                      new RuntimeStats()),
+                              Optional.empty(),
+                              Optional.empty(),
+                              ImmutableMap.of(),

Contributor

elharo Jul 25, 2024

Tangential and non-blocking, but I'd like to get in the habit of using Collections.emptyMap instead of ImmutableList.of to reduce our dependence on and vulnerability to third party libraries. ImmutableMap.of hasn't been needed for years now.

Contributor Author

ZacBlanco Jul 25, 2024 •

edited

Loading

If this is the case, then we should update the contributing guide to state that:

presto/CONTRIBUTING.md

Line 199 in 9a3c695

    
           * Prefer Immutable collections in Guava when possible. For example, instead of using

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

+                              Optional.empty(),
+                              Optional.empty(),
+                              ImmutableMap.of(),
+                              ImmutableSet.of(),

Contributor

elharo Jul 25, 2024

Collections.emptySet

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

+                              ImmutableSet.of(),
+                              StatsAndCosts.empty(),
+                              ImmutableList.of(),
+                              ImmutableList.of(),

Contributor

elharo Jul 25, 2024

Collections.emptyList

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

+                              Optional.empty(),
+                              null,
+                              EXCEEDED_GLOBAL_MEMORY_LIMIT.toErrorCode(),
+                              ImmutableList.of(

Contributor

elharo Jul 25, 2024

here you can use Arrays.asList
It's just a test so immutability doesn't help

jaystarshot reviewed

View reviewed changes

presto-main/src/main/java/com/facebook/presto/execution/QueryStateMachine.java Outdated Show resolved Hide resolved

ZacBlanco dismissed stale reviews from hantangwangd and aaneja via

394ba9f

July 25, 2024 19:39

ZacBlanco force-pushed the upstream-prune-query-info branch from 735877a to 394ba9f Compare

July 25, 2024 19:39

hantangwangd previously approved these changes

View reviewed changes

Member

hantangwangd left a comment

Thanks for the fix.

ZacBlanco requested review from tdcmeehan and jaystarshot

August 13, 2024 19:30

tdcmeehan reviewed

View reviewed changes

presto-main/src/main/java/com/facebook/presto/execution/QueryTracker.java Outdated Show resolved Hide resolved

ZacBlanco force-pushed the upstream-prune-query-info branch from 394ba9f to fab2478 Compare

August 15, 2024 22:36

ZacBlanco dismissed hantangwangd’s stale review via

d597d84

August 16, 2024 00:36

ZacBlanco force-pushed the upstream-prune-query-info branch 2 times, most recently from d597d84 to e3d09ad Compare

August 16, 2024 14:31

tdcmeehan reviewed

View reviewed changes

presto-main/src/main/java/com/facebook/presto/dispatcher/FailedDispatchQuery.java Outdated Show resolved Hide resolved

ZacBlanco force-pushed the upstream-prune-query-info branch 2 times, most recently from 1927346 to e2b9965 Compare

August 16, 2024 14:40


          Aggressively prune statistics from query info after completion

8d27c93

Previously, we only pruned information from old queries after
the expiration queue filled. With more the addition of histograms
query statistics can take two orders of magnitude more memory
than previous statistics. Without pruning statistics we will
much more quickly hit memory limits of the JVM.

This change aggressively prunes query plan metadata before query
expiry. This is mostly limited to query plan statistics.

ZacBlanco force-pushed the upstream-prune-query-info branch from e2b9965 to 8d27c93 Compare

August 16, 2024 14:41

tdcmeehan approved these changes

View reviewed changes

tdcmeehan merged commit 56223eb into prestodb:master

56 checks passed

This was referenced Aug 19, 2024

Flakiness in TestIcebergDistributedHive.testLargeQuery #23430

Open

[Iceberg]Refactor test classes to avoid them growing too large #23466

Merged

ZacBlanco mentioned this pull request

Fix NPE when pruning info from query plan #23544

Merged

6 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

elharo elharo left review comments

tdcmeehan tdcmeehan approved these changes

aaneja aaneja left review comments

hantangwangd hantangwangd left review comments

presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

Labels

None yet