Optimize how we merge multiple operatorStats by shangm2 · Pull Request #24414 · prestodb/presto

shangm2 · 2025-01-22T04:59:01Z

Description

Thanks to @arhimondr who observed that GC could take too much cpu to clean up memory during heavy load

This pr will be the first of a series of optimization to improve how objects are being created along the critical path.
This pr optimizes how we merge multiple operatorStats without creating temporary/intermediate objects

Motivation and Context

The original code will create temporary objects every time we add two OperatorStats together (with same id) using v.add(operatorStats) and this intermediate object will be discarded when it is used to merge with next OperatorStats object, This PR groups all operatorStats by their id and then merge them together in one go.
Refactoring the code by moving local variables into a dedicated class so that we can easily use one loop within the create method to aggregate all necessary metrics.

Impact

Test Plan

local hiveQueryRunner works fine
Internal verifier tests passed

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Optimizations
* Improve how we merge multiple operator stats together. :pr:`24414`
* Improve metrics creation by refactoring local variables to a dedicated class. :pr:`24414`

presto-main/src/main/java/com/facebook/presto/execution/StageExecutionInfo.java

steveburnett · 2025-01-24T15:23:20Z

Thanks for the release note! Rephrasing suggestions to follow the Order of changes in the Release Notes Guidelines:

== RELEASE NOTES ==

General Changes
* Improve how we merge multiple operator stats together. :pr:`24414`
* Improve metrics creation by refactoring local variables to a dedicated class. :pr:`24414`

shangm2 · 2025-01-24T17:21:12Z

@arhimondr feel free to take another look. Thank you so much for all the awesome suggestionsl

arhimondr

Thank you for following up. Looks good to me % comments

presto-main/src/main/java/com/facebook/presto/execution/StageExecutionInfo.java

presto-main/src/main/java/com/facebook/presto/operator/OperatorStats.java

arhimondr · 2025-01-27T21:45:04Z

presto-main/src/main/java/com/facebook/presto/operator/PipelineContext.java

-        for (OperatorStats operator : operators) {
-            operatorSummaries.compute(operator.getOperatorId(), (operatorId, summaryStats) -> summaryStats == null ? operator : summaryStats.add(operator));
-        }
+        operators.stream().collect(Collectors.groupingBy(OperatorStats::getOperatorId))


This will only have one OperatorStats per operator. Probably we can keep the loop and avoid groupingBy.

Yep. I should have double checked on that. Thanks for catching it.

arhimondr · 2025-01-27T21:45:25Z

presto-main/src/main/java/com/facebook/presto/operator/PipelineContext.java

-            }
-            operatorSummaries.put(operatorId, combined);
-        }
+        runningOperators.asMap().forEach((operatorId, runningStats) -> {


Consider using the same approach with merge + toImmutableList)

Good point. Did not realize this part is rather similar to StageExecutionStats

shangm2 · 2025-01-28T02:10:44Z

presto-main/src/main/java/com/facebook/presto/operator/OperatorStats.java

+        int stageExecutionId = first.getStageExecutionId();
+        int pipelineId = first.getPipelineId();
+        PlanNodeId planNodeId = first.getPlanNodeId();
+        String operatorType = first.getOperatorType();


@arhimondr one issue with "0" based initial value is we dont have a starting value for those ids but need to grab them from the first item of the collections passed in. This is safe since we check its emptiness right above. Let me know what you think.

arhimondr

Very good. Thank you for the follow up.

Looks great to me % two small questions

arhimondr · 2025-01-28T16:05:23Z

presto-main/src/main/java/com/facebook/presto/operator/PipelineContext.java

-        ListMultimap<Integer, OperatorStats> runningOperators = ArrayListMultimap.create();
        ImmutableList.Builder<DriverStats> drivers = ImmutableList.builderWithExpectedSize(driverContexts.size());
+        // Make deep copy of each list
+        ConcurrentMap<Integer, List<OperatorStats>> operatorStatsById = this.operatorStatsById.entrySet().stream()


Does it have to be ConcurrentMap? Have you considered a simple Map and the toMap collector?

You are absolutely correct. It does not need to be concurrentMap. Regular map will do the job.

arhimondr · 2025-01-28T16:06:16Z

presto-main/src/main/java/com/facebook/presto/operator/PipelineContext.java

        ImmutableList.Builder<DriverStats> drivers = ImmutableList.builderWithExpectedSize(driverContexts.size());
+        // Make deep copy of each list
+        ConcurrentMap<Integer, List<OperatorStats>> operatorStatsById = this.operatorStatsById.entrySet().stream()
+                .collect(toConcurrentMap(Map.Entry::getKey, e -> new ArrayList<>(Arrays.asList(e.getValue()))));


Do you need to have an extra new ArrayList<>(...)? Would Arrays.asList(e.getValue()) alone work?

Yeah, it needs to be new ArrayList<> since Arrays.asList(e.getValue()) will create a ~~immutable~~ resizable list but we need it be mutable so we can add item to it later on here

for (OperatorStats operatorStats : driverStats.getOperatorStats()) { operatorStatsById.computeIfAbsent(operatorStats.getOperatorId(), k -> new ArrayList<>()).add(operatorStats); }

The standard JDK Arrays.asList returns a mutable ArrayList: https://github.com/openjdk/jdk21/blob/master/src/java.base/share/classes/java/util/Arrays.java#L4222

Is this the JDK Arrays.asList used here? Or is there one in Guava?

It is from standard JDK. It gives the following exception because the Arrays.asList call "returns a fixed-size list backed by the specified array." (so basically a view of the array) We can modify existing elements but can not resize the list. I should have said it is mutable by not resizable. Sorry about the confusion and let me know if there is a better way. Really appreciate all the discussion. Learned a lot!

I didn't know that. Thank you for the explanation

## Description 1. this pr re-introduce the #24414 , which cause a sev where written partition was not logged. The bug is a corner case, where while merging only one single non-mergeable operatorInfo, the old code will NOT perform any merge operation (since the add operation will only get invoke when the second operator stats shows up) and give back the operator info itself while #24414 will actually kick off a merge and gives null result. 2. This pr reintroduce #24414 and handles this corner case and also added specific unit tests for this scenario. ## Motivation and Context 1. re-introduce #24414 ## Impact  ## Test Plan 1. verifier runs log written partition correctly: <img width="1469" alt="Screenshot 2025-04-15 at 17 13 08" src="https://github.com/user-attachments/assets/f7c84a8f-7381-411a-95d1-15b075870b83" /> ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] 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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * Improve how we merge multiple operator stats together. * Improve metrics creation by refactoring local variables to a dedicated class. ```

## Description 1. this pr re-introduce the prestodb#24414 , which cause a sev where written partition was not logged. The bug is a corner case, where while merging only one single non-mergeable operatorInfo, the old code will NOT perform any merge operation (since the add operation will only get invoke when the second operator stats shows up) and give back the operator info itself while prestodb#24414 will actually kick off a merge and gives null result. 2. This pr reintroduce prestodb#24414 and handles this corner case and also added specific unit tests for this scenario. ## Motivation and Context 1. re-introduce prestodb#24414 ## Impact  ## Test Plan 1. verifier runs log written partition correctly: <img width="1469" alt="Screenshot 2025-04-15 at 17 13 08" src="https://github.com/user-attachments/assets/f7c84a8f-7381-411a-95d1-15b075870b83" /> ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] 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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * Improve how we merge multiple operator stats together. * Improve metrics creation by refactoring local variables to a dedicated class. ```

shangm2 requested a review from a team as a code owner January 22, 2025 04:59

shangm2 requested a review from presto-oss January 22, 2025 04:59

prestodb-ci added the from:Meta PR from Meta label Jan 22, 2025

shangm2 force-pushed the optimize_merging_operator_stats branch from dfcc4ca to a8ecf10 Compare January 22, 2025 05:04

arhimondr reviewed Jan 22, 2025

View reviewed changes

shangm2 force-pushed the optimize_merging_operator_stats branch 2 times, most recently from 727cf06 to 6808005 Compare January 24, 2025 07:38

arhimondr reviewed Jan 27, 2025

View reviewed changes

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

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

shangm2 force-pushed the optimize_merging_operator_stats branch 2 times, most recently from 1d78017 to a3c6221 Compare January 27, 2025 21:41

arhimondr reviewed Jan 27, 2025

View reviewed changes

shangm2 force-pushed the optimize_merging_operator_stats branch 2 times, most recently from 1cedaea to 749d60b Compare January 28, 2025 02:07

shangm2 commented Jan 28, 2025

View reviewed changes

shangm2 force-pushed the optimize_merging_operator_stats branch 3 times, most recently from 6ea95c3 to cec07cd Compare January 28, 2025 04:51

arhimondr previously approved these changes Jan 28, 2025

View reviewed changes

Improve the merging of operator stats

35c51ba

shangm2 dismissed arhimondr’s stale review via 35c51ba January 28, 2025 19:07

shangm2 force-pushed the optimize_merging_operator_stats branch from cec07cd to 35c51ba Compare January 28, 2025 19:07

arhimondr approved these changes Jan 28, 2025

View reviewed changes

arhimondr merged commit 510024d into prestodb:master Jan 28, 2025
52 checks passed

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

kewang1024 mentioned this pull request Apr 1, 2025

OperatorStats overflow fix #24843

Merged

7 tasks

shangm2 mentioned this pull request Apr 16, 2025

Re-introduce improving the merging of operator stats #24921

Merged

6 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

Conversation

shangm2 commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveburnett commented Jan 24, 2025

Uh oh!

shangm2 commented Jan 24, 2025

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhimondr Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhimondr Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangm2 Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangm2 Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shangm2 commented Jan 22, 2025 •

edited

Loading

arhimondr Jan 27, 2025 •

edited

Loading

arhimondr Jan 27, 2025 •

edited

Loading

shangm2 Jan 28, 2025 •

edited

Loading

shangm2 Jan 28, 2025 •

edited

Loading