houqp commented on a change in pull request #1691:
URL: https://github.com/apache/arrow-datafusion/pull/1691#discussion_r794984468
##########
File path: datafusion/src/execution/memory_manager.rs
##########
@@ -245,10 +245,10 @@ The memory management architecture is the following:
/// Manage memory usage during physical plan execution
#[derive(Debug)]
pub struct MemoryManager {
- requesters: Arc<Mutex<HashMap<MemoryConsumerId, Weak<dyn
MemoryConsumer>>>>,
- trackers: Arc<Mutex<HashMap<MemoryConsumerId, Weak<dyn MemoryConsumer>>>>,
+ requesters: Arc<Mutex<HashSet<MemoryConsumerId>>>,
pool_size: usize,
requesters_total: Arc<Mutex<usize>>,
Review comment:
this lock only guarantees the two operations updating requesters_total
and calling cv.notify_all will be performed atomically, but it looks like this
doesn't really buy us anything? The waiter on self.cv can wake up and get
preempted right away by other threads that might update requesters_total. I am
curious from your point of view what benefit this critical region provides.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
