Optimize MemoryPool synchronization by viczhang861 · Pull Request #14117 · prestodb/presto

viczhang861 · 2020-02-18T23:07:32Z

Contention of lock for MemoryPool class is observed under high load.

== RELEASE NOTES ==

General Changes
*

wenleix · 2020-02-19T20:45:55Z

Do we know today when are we using tagged memory? :)

Threads could be blocked by waiting for lock for function updateTaggedMemoryAllocations.

@viczhang861 : That's a nice finding! I am wondering if you have any profiling numbers to share? (e.g. asyncprofiler supports profiling lock contention by using -e lock: https://github.com/jvm-profiling-tools/async-profiler

wenleix · 2020-02-19T20:53:57Z

A different approach would be avoid synchronize the whole updateTaggedMemoryAllocations method by using things like ConcurrentHashMap and AtomicLong, etc. Since tagged memory might be useful when query run out of task memory? (it shows top memory consumption for operators).

Alternatively, we can make this being able to enable/disable at a per-query basis, so we can make it default to false and only enable it when we want to debug? -- This might be easier to implement. What do you think? @arhimondr , @aweisberg , @viczhang861

viczhang861 · 2020-02-19T21:17:30Z

Do we know today when are we using tagged memory? :)

/v1/cluster/memory will list memory allocation for each tag

aweisberg · 2020-02-19T21:47:14Z

The lock is already held from all the contexts where tagged memory is updated. So the # of lock acquisitions isn't changed although the duration is probably changed because it's two less map updates.

Since the lock is still being acquired does removing tagged allocations improve the situation much?

I think this can be made faster in better and still simple ways.

queryMemoryReservations and taggedMemoryReservations both share a key. So there could be a single map with an object there containing the values. This would also save on boxing/unboxing overhead inside the lock when modifying the counters.

We could roll revocable reservations into the same thing.

We also should use mutable long wrappers instead of Long so we don't have to keep allocating under the lock when updating the tagged memory allocations.

The next thing would be could we update tagged memory allocations outside the lock? How many things there can we push outside the lock? Need to spend more time looking at it to determine that.

aweisberg · 2020-02-19T21:48:10Z

Also RE eliminating the lock entirely, not sure. It's a pretty complicated set of interactions between moving memory between pools and managing the future that notifies waiters that memory is available.

viczhang861 · 2020-02-19T22:03:45Z

The lock is already held from all the contexts where tagged memory is updated. So the # of lock acquisitions isn't changed although the duration is probably changed because it's two less map updates.

Yes, it is mainly about making the function run faster with less duration

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

aweisberg

I think free() is not safe in this formulation. I also don't think move Is safe even though it is synchronized, but not synchronized with the rest of the code manipulating these maps.

Read write locks are generally kind of slow but it might work to read lock for your basic free and reserve. Write lock for ones when you want to remove an entries from the map, and move between threadpools.

From a correctness standpoint I think that is the simplest path forward because it allows us to separate things we think can be correct when done concurrently from things where we require full isolation.

We already acquire a lock per operation and aren't making it truly lock free so the RW lock should be fine. I don't recall if RW locks perform the same as regular locks.

The truly lock free solution would be to immutably CAS from one state to another on a per query basis, but there are several fields we are updating at once including large nested maps so I don't think it's viable here.

aweisberg · 2020-02-21T22:45:47Z

free also does a blind put into the map https://github.com/prestodb/presto/blob/e2102cb60232dd6ceddfcaa2515f6a5fb7b74aeb/presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java#L204

viczhang861 · 2020-02-22T01:39:54Z

Generally the approach to removing the memory reservation on free is fragile. We should remove the reservation information when the query itself is cleaned up/removed from the node.

I agree. I rewrite previous logics to make free() thread safe without synchronizing queryMemoryReservations for every free() operation. Thanks for review again.

Move query and revocable memory is not likely to cause contention since they are rarely called, I can keep them unchanged and focus on taggedMemoryReservation related lock. I guess free() is also called much less often than reserve().

aweisberg · 2020-02-25T17:39:31Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

Can you update this to be like reserve? reserving revocable shouldn't be slow when we improved regular reserve. We have the knowledge and experience right now to do it correctly and it will be harder and more confusing for someone coming in later to try and figure out why it's different and how to safely improve it.

aweisberg · 2020-02-25T17:46:34Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

Don't call containsKey, switch between merge vs computeIfPresent. Avoids repeating the map lookup.

Optimized with calling get(key) at most once.

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

aweisberg · 2020-02-25T19:51:33Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

This still doesn't match how regular free works. This unconditional put overwrites the value another thread has put in. I think it needs to basically match what we did in free in terms of merging in the initial -bytes. Then you can remove the "else".

I removed old read and then remove, and use the strategy in free

aweisberg

YOLO LGTM

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

- Use ConcurrentHashMap - Remove unnecessary synchornization of instance method - Synchronization is needed for updating reservedBytes - Synchronization is needed for adding/removing queryId

wenleix · 2020-03-03T05:10:29Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

-    private long reservedBytes;
-    @GuardedBy("this")
-    private long reservedRevocableBytes;
+    private volatile long reservedBytes;


Does it make sense to mark it with @GuardedBy("this")? cc @aweisberg , @arhimondr

wenleix

LGTM % a minor question.

wenleix · 2020-03-03T05:14:06Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

+                }
            }
+        }
+        synchronized (this) {


why this now needs to be synchronized?

The idea is to use synchronization whenever write operation is performed, and let read operation become lock free. Thus, reading is not @GuardedBy("this")

wenleix · 2020-03-03T05:17:49Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

-                updateTaggedMemoryAllocations(queryId, allocationTag, bytes);
+        }
+        if (bytes != 0) {
+            while (true) {


i understand what this tries to do (basically either one of the following operation succeed, we are good). But it's a bit mind-twisting :). is this a common practice Java concurrency programming? ;) cc @aweisberg , @arhimondr

arhimondr · 2020-03-03T14:45:20Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

-    @GuardedBy("this")
    // TODO: It would be better if we just tracked QueryContexts, but their lifecycle is managed by a weak reference, so we can't do that
-    private final Map<QueryId, Long> queryMemoryReservations = new HashMap<>();
+    private final Map<QueryId, Long> queryMemoryReservations = new ConcurrentHashMap<>();


All three maps (queryMemoryReservations, taggedMemoryAllocations, queryMemoryRevocableReservations) are here to track memory reservations per query. These maps are quite orthogonal to the main function of the MemoryPool class. Instead of trying to optimize locking here by applying various "ninja" techniques i would strongly recommend moving the "per query" related memory tracking to the QueryContext. QueryContext is synchronized on the per query basis, thus the locking impact should be lower. The MemoryPool class should remain only with two long fields that have to be updated under a lock (reservedBytes, reservedRevocableBytes). Updating a long under a global lock should be very lightweight operation, and thus shouldn't cause significant locking contention.

CC @nezihyigitbasi

@arhimondr : This makes sense. But will this be a lot of additional work? -- maybe we can merge this as it is (since it brings some incremental value) and leave the query level syntonization as future work? :)

Given the high risk nature of this change and very limited improvement it brings I would rather postpone it, and do the right refactoring once we have more time.

stale · 2020-09-02T17:43:16Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

viczhang861 requested review from arhimondr, aweisberg, nezihyigitbasi and wenleix February 19, 2020 02:32

viczhang861 changed the title ~~Configure taggedMemoryAllocations update in MemoryPool~~ [WIP]Configure taggedMemoryAllocations update in MemoryPool Feb 20, 2020

viczhang861 self-assigned this Feb 20, 2020

viczhang861 force-pushed the tagged_memory branch from c75c0e5 to e2102cb Compare February 21, 2020 20:45

viczhang861 changed the title ~~[WIP]Configure taggedMemoryAllocations update in MemoryPool~~ [WIP]Optimize MemoryPool synchronization Feb 21, 2020

aweisberg reviewed Feb 21, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java Outdated Show resolved Hide resolved

aweisberg suggested changes Feb 21, 2020

View reviewed changes

viczhang861 force-pushed the tagged_memory branch from e2102cb to 005032a Compare February 22, 2020 01:32

viczhang861 force-pushed the tagged_memory branch 3 times, most recently from 656ed0d to 8bd336e Compare February 25, 2020 00:08

viczhang861 changed the title ~~[WIP]Optimize MemoryPool synchronization~~ Optimize MemoryPool synchronization Feb 25, 2020

aweisberg suggested changes Feb 25, 2020

View reviewed changes

viczhang861 force-pushed the tagged_memory branch from 8bd336e to 6888976 Compare February 25, 2020 19:23

aweisberg reviewed Feb 25, 2020

View reviewed changes

viczhang861 force-pushed the tagged_memory branch from 6888976 to 538322f Compare February 25, 2020 20:23

aweisberg approved these changes Feb 25, 2020

View reviewed changes

viczhang861 force-pushed the tagged_memory branch from 538322f to 2b679e5 Compare February 26, 2020 21:24

aweisberg reviewed Feb 26, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java Outdated Show resolved Hide resolved

Optimize MemoryPool synchronization

469bf7d

- Use ConcurrentHashMap - Remove unnecessary synchornization of instance method - Synchronization is needed for updating reservedBytes - Synchronization is needed for adding/removing queryId

viczhang861 force-pushed the tagged_memory branch from 2b679e5 to 469bf7d Compare February 26, 2020 22:35

wenleix mentioned this pull request Mar 2, 2020

Add tracking of memory allocation per operator #14191

Merged

wenleix reviewed Mar 3, 2020

View reviewed changes

wenleix approved these changes Mar 3, 2020

View reviewed changes

arhimondr requested changes Mar 3, 2020

View reviewed changes

stale bot added the stale label Sep 2, 2020

stale bot closed this Sep 10, 2020

Conversation

viczhang861 commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenleix commented Feb 19, 2020

Uh oh!

wenleix commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viczhang861 commented Feb 19, 2020

Uh oh!

aweisberg commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg commented Feb 19, 2020

Uh oh!

viczhang861 commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

aweisberg commented Feb 21, 2020

Uh oh!

viczhang861 commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg Feb 25, 2020 • 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!

Uh oh!

aweisberg Feb 25, 2020 • 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!

viczhang861 Feb 25, 2020 • 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!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenleix 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!

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!

stale bot commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viczhang861 commented Feb 18, 2020 •

edited

Loading

wenleix commented Feb 19, 2020 •

edited

Loading

aweisberg commented Feb 19, 2020 •

edited

Loading

viczhang861 commented Feb 19, 2020 •

edited

Loading

viczhang861 commented Feb 22, 2020 •

edited

Loading

aweisberg Feb 25, 2020 •

edited

Loading

aweisberg Feb 25, 2020 •

edited

Loading

viczhang861 Feb 25, 2020 •

edited

Loading