Implement Spilling Strategies #15173

sachdevs · 2020-09-15T15:47:27Z

This PR adds multiple different spilling strategies that can be swapped between using the config experimental.spiller.task-spilling-strategy.

ORDER_BY_CREATE_TIME - current default strategy. Watch memory pools for revocable memory exceeding threshold, sort tasks by create time, revoke individual operators until we reach the lower threshold.

ORDER_BY_REVOCABLE_BYTES - NEW. Watch memory pools for revocable memory exceeding threshold, sort tasks by most allocated revocable bytes, revoke individual operators until we reach the lower threshold.

PER_TASK_MEMORY_THRESHOLD - NEW. Watch revocable memory pool for memory exceeding per task memory threshold defined by experimental.spiller.max-revocable-task-memory. Spill operators in task until it lowers to this threshold.

TODO

~~finish integration tests~~
~~better commit message~~
~~Release note~~

== RELEASE NOTES ==

General Changes
* Add config `experimental.spiller.task-spilling-strategy` for choosing different spilling strategy to use.

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

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

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

sachdevs · 2020-09-16T18:39:06Z

I'll actually update this with PER_TASK_MEMORY_THRESHOLD as it's own class. Right now MemoryRevokingScheduler is doing too much conditional logic.

sachdevs · 2020-09-24T03:53:04Z

Still finishing up integration tests, updating revocable memory at the task level without having an SqlTaskManager has been a bit of a challenge. Everything else is complete however.

wenleix · 2020-09-25T22:12:36Z

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

+import static java.util.Objects.requireNonNull;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+public class TaskThresholdMemoryRevokingScheduler


Sorry if it's a dumb question. I just realized memory revoking scheduler (and MemoryRevokingScheduler) doesn't implement any interface. Curious how are they get called? I assume it's related to the @PostConstruct and @PreDestroy annotation?

Yup you got it. They run as a separate thread and are created at injection time. On creation (@PostConstruct), we register a periodically executing method to check if revoking is necessary (based on whichever spilling strategy we are using). As for which scheduler is created, that is determined in ServerMainModule, using installModuleIf. I was wondering too if it's worth having an interface but figured there wasn't enough reason to abstract it yet.

@sachdevs : I see. Is this a mechanism from Guice? (e.g. automatically call methods annotated with @PostConstruct)

Javax annotation: https://docs.oracle.com/javaee/7/api/javax/annotation/PostConstruct.html.

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

aweisberg · 2020-09-28T19:09:25Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

+    public enum TaskSpillingStrategy
+    {
+        ORDER_BY_CREATE_TIME, // When spilling is triggered, revoke tasks in order of oldest to newest
+        ORDER_BY_REVOCABLE_BYTES, // When spilling is triggered, revoke tasks by most allocated revocable memory to least allocated revocable memory


Should we treat join and aggregation spill the same way? The cost of spilling them is not uniform.

With join if you spill it then the entire probe side has to be spilled incurring a 3x IO cost.

With aggregation if you spill it will have to merge in one more run, but it can continue to use memory to avoid creating more sorted runs.

In practice whether this distinction matters often???

There are two levels of spilling strategy:

Prioritizing which operator to spill within a given task.
This would be easy to add on top of the existing implementation. Modifying the VoidTraversingQueryContextVisitor to make a list of operators and then rank them by priority would accomplish this. This refers to how we choose to spill operators within a task. So far, these implementations do not try to distinguish between operators when choosing to spill which isn't ideal. I can look into adding this in as we see fit depending on how spilling works in production for our workload as we start rolling out soon.

Prioritizing which task to spill in a list of currently running tasks.
As for distinguishing between revocable bytes allocated by join operator vs agg operator and using that to prioritize which tasks to spill, that would be a bit more work. Let's circle back on this if we see issues with this in during shadow.

highker

minor comments only. Can @wenleix also help to take a look?

presto-main/src/test/java/com/facebook/presto/execution/TestMemoryRevokingScheduler.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

highker · 2020-09-29T04:18:15Z

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

+    private final long maxRevocableMemoryPerTask;
+
+    @Nullable
+    private ScheduledFuture<?> scheduledFuture;


I guess this variable is not thread-safe? Maybe have synchronized methods/blocks + "GuardedBy("this")"

Looks like it's just copied from existing MemoryRevokingScheduler 😂

See comment on lines 95-98

highker · 2020-09-29T04:21:57Z

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

+        if (scheduledFuture != null) {
+            scheduledFuture.cancel(true);
+            scheduledFuture = null;
+        }


This is not thread-safe

Since this class is injected as a SINGLETON + this part is only called on @PreDestroy, this method won't be called until this class is destroyed (We never call stop() manually). This won't happen until Presto shuts down? I can't imagine any other case in which we will destroy these schedulers. This method only exists to stop leaking this future for tests I'm guessing (since a similar method is also present in MemoryRevokingScheduler). Let me know if we still need thread safety for this variable because of this context.

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

wenleix

Publish some comments since there are changes to the commit

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

wenleix · 2020-09-29T15:57:59Z

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

+    private final long maxRevocableMemoryPerTask;
+
+    @Nullable
+    private ScheduledFuture<?> scheduledFuture;


Looks like it's just copied from existing MemoryRevokingScheduler 😂

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

wenleix

LGTM.

...o-main/src/main/java/com/facebook/presto/execution/TaskThresholdMemoryRevokingScheduler.java

sachdevs · 2020-09-29T19:12:46Z

Rebase master due to unrelated test fail.

This adds ordering tasks by revocable bytes and per task memory threshold alongside ordering by create time.

sachdevs · 2020-09-29T21:40:01Z

Tests are green, only added comment. Good to go 👍

sachdevs changed the title ~~Prototype different task spilling strategies for performance testing~~ [WIP] Prototype different task spilling strategies for performance testing Sep 15, 2020

sachdevs requested a review from highker September 15, 2020 15:48

sachdevs force-pushed the task-based-spilling branch from 00ac981 to 5347917 Compare September 15, 2020 15:53

highker reviewed Sep 16, 2020

View reviewed changes

sachdevs force-pushed the task-based-spilling branch from 5347917 to 3d0be00 Compare September 24, 2020 03:52

sachdevs requested a review from highker September 24, 2020 03:54

sachdevs changed the title ~~[WIP] Prototype different task spilling strategies for performance testing~~ [WIP] Implement Spilling Strategies Sep 24, 2020

wenleix reviewed Sep 25, 2020

View reviewed changes

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

sachdevs force-pushed the task-based-spilling branch 2 times, most recently from 0b33842 to 46ed059 Compare September 28, 2020 18:48

sachdevs changed the title ~~[WIP] Implement Spilling Strategies~~ Implement Spilling Strategies Sep 28, 2020

aweisberg reviewed Sep 28, 2020

View reviewed changes

sachdevs force-pushed the task-based-spilling branch from 46ed059 to 400f1fb Compare September 28, 2020 19:20

sachdevs requested a review from wenleix September 28, 2020 19:33

highker approved these changes Sep 29, 2020

View reviewed changes

highker self-assigned this Sep 29, 2020

sachdevs force-pushed the task-based-spilling branch 2 times, most recently from c56cc07 to 43a9b80 Compare September 29, 2020 17:10

wenleix reviewed Sep 29, 2020

View reviewed changes

wenleix approved these changes Sep 29, 2020

View reviewed changes

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

sachdevs force-pushed the task-based-spilling branch 2 times, most recently from d6e2c5b to 4be43f5 Compare September 29, 2020 19:12

Introduce TaskSpillingStrategy and multiple spilling strategies

5bf9207

This adds ordering tasks by revocable bytes and per task memory threshold alongside ordering by create time.

sachdevs force-pushed the task-based-spilling branch from 4be43f5 to 5bf9207 Compare September 29, 2020 21:39

highker merged commit 544b5a4 into prestodb:master Sep 29, 2020

This was referenced Oct 6, 2020

Add release notes for 0.242 #15270

Merged

[Test] Add release notes for 0.242 #15291

Closed

[Test-Only] Add release notes for 0.242 #15294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Spilling Strategies #15173

Implement Spilling Strategies #15173

sachdevs commented Sep 15, 2020 •

edited by highker

Loading

sachdevs commented Sep 16, 2020

sachdevs commented Sep 24, 2020

wenleix Sep 25, 2020 •

edited

Loading

sachdevs Sep 28, 2020

wenleix Sep 29, 2020

sachdevs Sep 29, 2020

aweisberg Sep 28, 2020

sachdevs Sep 28, 2020

highker left a comment

highker Sep 29, 2020

wenleix Sep 29, 2020

sachdevs Sep 29, 2020

highker Sep 29, 2020

sachdevs Sep 29, 2020 •

edited

Loading

wenleix left a comment

wenleix Sep 29, 2020

wenleix left a comment

sachdevs commented Sep 29, 2020

sachdevs commented Sep 29, 2020

Implement Spilling Strategies #15173

Implement Spilling Strategies #15173

Conversation

sachdevs commented Sep 15, 2020 • edited by highker Loading

sachdevs commented Sep 16, 2020

sachdevs commented Sep 24, 2020

wenleix Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachdevs Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

sachdevs commented Sep 29, 2020

sachdevs commented Sep 29, 2020

sachdevs commented Sep 15, 2020 •

edited by highker

Loading

wenleix Sep 25, 2020 •

edited

Loading

sachdevs Sep 29, 2020 •

edited

Loading