Do we need StrageyBase._results_lock?

[al...@moreati.org.uk]

ansible.plugins.strategy.StrategyBase._results_lock is acquired to control access to StrategyBase._results and StrategyBase._handler_results. Specifically

- in ansible.plugins.strategy.results_thread_main() before append() ing a result

- in StrategyBase_process_pending_results() before popleft() ing

However StrategyBase._results & _handler_results are both collections.deque objects. From the Python docs

> Deques support thread-safe, memory efficient appends and pops from either side of the deque.

Thus AFAICT the lock isn't needed for the deque operations, and could possibly be removed. Have I missed something? Is the lock providing another protection I've not noticed? Is it there as a belt and braces measure?

If there's no  reason to have it, and interest in removing it, I'll submit a PR. Alternatively maybe there's benefit in keeping the lock and using wait()/notify() instead of some of the polling.

Regards, Alex

[Matt Martz]

I did some basic performance testing here, and the lock as currently implemented provides a decent performance improvement.

On my machine with 100 hosts, 100 debug tasks, and 25 forks, I found that removing the lock increased execution time by close to 60 seconds.

Based on that, I'd be hesitant to remove it.

--
You received this message because you are subscribed to the Google Groups "Ansible Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ansible-deve...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/ansible-devel/3a309938-189d-4b8b-b9c4-539afea81984n%40googlegroups.com.

--

Matt Martz
@sivel
sivel.net

[Alex Willmer]

Huh, I would not have guessed that. Thanks, I'll try that next time I can't explain the presence of some code.

--

Alex Willmer <al...@moreati.org.uk>

[Matt Martz]

I'll honestly say I was surprised too. I expected the opposite. These are questions that chew away at my days; Running endless performance profiling.

[Alex Willmer]

Matt,

Could you share your benchmark. I'm seeing barely any difference

With lock:

± hyperfine -- "ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml"
Benchmark #1: ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml
  Time (mean ± σ):      6.744 s ±  0.107 s    [User: 27.194 s, System: 1.963 s]
  Range (min … max):    6.537 s …  6.829 s    10 runs

Without lock

$ hyperfine -- "ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml"
Benchmark #1: ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml
  Time (mean ± σ):      6.675 s ±  0.090 s    [User: 27.160 s, System: 1.936 s]
  Range (min … max):    6.499 s …  6.748 s    10 runs

$ git rev-parse HEAD
c4e211a4291162c17054f44ad5381856ffff802f
$ git diff | cat
diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py
index 70dea1ea6f..31e0f58eb5 100644
--- a/lib/ansible/plugins/strategy/__init__.py
+++ b/lib/ansible/plugins/strategy/__init__.py
@@ -101,14 +101,13 @@ def results_thread_main(strategy):
                 strategy._tqm.send_callback(result.method_name, *result.args, **result.kwargs)
             elif isinstance(result, TaskResult):
                 strategy.normalize_task_result(result)
-                with strategy._results_lock:
-                    # only handlers have the listen attr, so this must be a handler
-                    # we split up the results into two queues here to make sure
-                    # handler and regular result processing don't cross wires
-                    if 'listen' in result._task_fields:
-                        strategy._handler_results.append(result)
-                    else:
-                        strategy._results.append(result)
+                # only handlers have the listen attr, so this must be a handler
+                # we split up the results into two queues here to make sure
+                # handler and regular result processing don't cross wires
+                if 'listen' in result._task_fields:
+                    strategy._handler_results.append(result)
+                else:
+                    strategy._results.append(result)
             else:
                 display.warning('Received an invalid object (%s) in the result queue: %r' % (type(result), result))
         except (IOError, EOFError):
@@ -223,7 +222,6 @@ class StrategyBase:
 
         self._results = deque()
         self._handler_results = deque()
-        self._results_lock = threading.Condition(threading.Lock())
 
         # create the result processing thread for reading results in the background
         self._results_thread = threading.Thread(target=results_thread_main, args=(self,))
@@ -524,15 +522,12 @@ class StrategyBase:
         cur_pass = 0
         while True:
             try:
-                self._results_lock.acquire()
                 if do_handlers:
                     task_result = self._handler_results.popleft()
                 else:
                     task_result = self._results.popleft()
             except IndexError:
                 break
-            finally:
-                self._results_lock.release()
 
             original_host = task_result._host
             original_task = task_result._task

$ cat 100_hosts.yml
all:
  hosts:
    local[001:100]:
  vars:
    ansible_connection: local

$ cat 100_debugs.yml
- hosts: all
  gather_facts: false
  tasks:
    - debug:
      loop: "{{ range(100) }}"

--

Alex Willmer <al...@moreati.org.uk>

[Matt Martz]

I won't have time until sometime later, but note that I had 100 actual debug tasks, not a loop of 100 items.

[Brian Coca]

note that loops are within the same subprocess, while tasks each
require their own, so there are very different performance profiles

On Wed, Mar 24, 2021 at 10:02 AM Matt Martz <ma...@sivel.net> wrote:
>
> I won't have time until sometime later, but note that I had 100 actual debug tasks, not a loop of 100 items.
>

--
----------
Brian Coca