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

Avoid creating task proxies #5316

Closed

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jan 20, 2023

Reduces the impact of #5315 a bit.

  • There is a significant overhead to initiating a TaskProxy object.
  • Avoid doing this where it is not necessary.
  • In my test with the example in efficiency: increment_graph_window #5315
    with -s TASKS=15 -s CYCLES=1 this reduced the time taken by
    expand_graph_window by ~38% from from 12.2s to 7.54.

Also avoid duplicate get_platform calls.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

* There is a significant overhead to initiating a `TaskProxy` object.
* Avoid doing this where it is not necessary.
* In my test with the example in cylc#5315
  with `-s TASKS=15 -s CYCLES=1` this reduced the time taken by
  `expand_graph_window` by ~38% from from 12.2s to `7.54`.
@oliver-sanders oliver-sanders added the efficiency For notable efficiency improvements label Jan 20, 2023
@oliver-sanders oliver-sanders added this to the 8.1.1 milestone Jan 20, 2023
@oliver-sanders oliver-sanders self-assigned this Jan 20, 2023
@oliver-sanders oliver-sanders requested review from wxtim and MetRonnie and removed request for wxtim January 20, 2023 13:33
Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will alleviate the task proxy creation somewhat.. and no harm in adding it.

However, we can probably get away with not creating task proxies at all.. and just gather the information for the store a different way.. Will look into it.

@@ -651,7 +651,7 @@ def generate_definition_elements(self):
self.parents = parents

def increment_graph_window(
self, itask, edge_distance=0, active_id=None,
self, itask, pool, edge_distance=0, active_id=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pool is available via self.schd by the way.

@dwsutherland
Copy link
Member

Have put up a PR .. it partially supersedes this one:
#5319

Will run profiling on it, but should be a vast improvement

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Jan 23, 2023

wrong PR

@oliver-sanders
Copy link
Member Author

Superseded by #5319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
efficiency For notable efficiency improvements superseded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants