Do we need StrageyBase._results_lock?

Archives Ansible Developer
1

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

2

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.

3

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

4

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.

5

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) }}”
6

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

7

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