[GitHub] [beam] pabloem commented on a change in pull request #11590: [BEAM-8944] Improve UnboundedThreadPoolExecutor performance

[GitBox <gi...@apache.org>]

pabloem commented on a change in pull request #11590:
URL: https://github.com/apache/beam/pull/11590#discussion_r420468171



##########
File path: sdks/python/apache_beam/utils/thread_pool_executor.py
##########
@@ -137,35 +101,33 @@ def submit(self, fn, *args, **kwargs):
     """
     future = _base.Future()
     work_item = _WorkItem(future, fn, args, kwargs)
-    try:
-      # Keep trying to get an idle worker from the queue until we find one
-      # that accepts the work.
-      while not self._idle_worker_queue.get(
-          block=False).accepted_work(work_item):
-        pass
-      return future
-    except queue.Empty:
-      with self._lock:
-        if self._shutdown:
-          raise RuntimeError(
-              'Cannot schedule new tasks after thread pool '
-              'has been shutdown.')
-
-        worker = _Worker(
-            self._idle_worker_queue,
-            self._permitted_thread_age_in_seconds,
-            work_item)
+    with self._lock:
+      if self._shutdown:
+        raise RuntimeError(
+            'Cannot schedule new tasks after thread pool has been shutdown.')
+      try:
+        self._idle_worker_queue.get(block=False).assign_work(work_item)
+
+        # If we have more idle threads then the max allowed, shutdown a thread.
+        if self._idle_worker_queue.qsize() > self._max_idle_threads:
+          try:
+            self._idle_worker_queue.get(block=False).shutdown()

Review comment:
       should we remove a total of `self._max_idle_threads - self._idle_worker_queue.qsize()` workers rather than just one?
   IIUC, this is the only point (besides shutdown) where workers are removed, so maybe yes?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org