Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PlayIterator, refactored #83134

Open
wants to merge 29 commits into
base: devel
Choose a base branch
from

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Apr 24, 2024

SUMMARY

This is a series of refactorings split into separate, fairly simple, commits for easier review. The benefits are:

  • Less code, ~100 lines of PlayIterator code is being removed.
  • Easier to reason about and debug.
  • Performance benefits. See the Additional information below, those are isolated tests, some of them with ridiculous number of nested blocks. I am not sure how noticeable it will be in playbooks in general but I imagine it should save some time in those with a lot of includes. I have not done memory profiling but with reduced HostState copying this PR introduces it might be interesting to see as well.
  • Changes are made only to internals of PlayIterator and should not be visible to its users.

There are more things to improve, I have mentioned some of them in FIXMEs/TODOs but this isn't small as-is so leaving them for next time.

ISSUE TYPE
  • Feature Pull Request
ADDITIONAL INFORMATION

Iterating through N nested blocks, each block has one task and a nested block, the most nested block is being failed.

Before:
0.00024080276489257812 seconds, 10 blocks
0.004384040832519531 seconds, 50 blocks
0.9080429077148438 seconds, 350 blocks
19.866453170776367 seconds, 992 blocks (I believe 992 was a limit for devel to handle)

After:
0.00009512901306152344 seconds, 10 blocks
0.0005481243133544922 seconds, 50 blocks
0.01589512825012207 seconds, 350 blocks
0.11786723136901855 seconds, 992 blocks

this simplifies things a lot, before the complete state there is still
posibility that the failure would be recovered, or the host is failed
but the always section needs to be iterated
now that we only have one child_state
... in case of a failure outside of PlayIterator
easier to preserve order
...operate on the state itself, not the copy, which is almost always
what we want, especially for read-only operations
... as printing the whole struture could be very inefficient since the
states are always str'ed for display.debug
@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Apr 24, 2024
@webknjaz

This comment was marked as resolved.

This comment was marked as resolved.

@ansibot ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Apr 24, 2024
@ansibot

This comment was marked as resolved.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 24, 2024
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Apr 24, 2024
@ansibot ansibot removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. labels Apr 25, 2024
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Apr 25, 2024
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants