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

task_proxy: cache prereq status for speed #1813

Merged
merged 3 commits into from
Apr 28, 2016

Conversation

benfitzpatrick
Copy link
Contributor

This adds some caching to speed up task proxy ready_to_run.

(By the way, all these speed up PRs are coming from a single branch I've been working on for a few weeks. I've decided to split it up into related pieces.)

If a task doesn't have conditional prerequisites, then we can simply cache the 'am I satisfied' result instead of rechecking it every loop. We only normally need to recheck it once one or more relevant outputs have been found in satisfy_me.

I've moved the conditional expression checking into its own method for neatness and ease of future profiling - I wonder how much it costs if we have lots of 'OR' dependent tasks!

This reduces the cost of calling ready_to_run (another dominant cost in a modified busy suite with lots of waiting tasks) by around two thirds.

@hjoliver, @kaday please review.

@benfitzpatrick benfitzpatrick added this to the next-release milestone Apr 26, 2016
@hjoliver
Copy link
Member

@benfitzpatrick - why can't we cache conditional results too? If a conditional prerequisite is unsatisfied, it only needs checking if any of the involved outputs get completed, and once satisfied it doesn't need rechecking short of a manual state reset. But maybe I'm missing something? The test battery passes OK after a quick stab at implementing this on top of your branch: hjoliver@bc815b8 (in a simple test suite this halved the number of calls to eval() the conditional expression).

@hjoliver hjoliver assigned benfitzpatrick and unassigned hjoliver Apr 27, 2016
@benfitzpatrick
Copy link
Contributor Author

I think you're right, looking at it. I've incorporated your commit.

@hjoliver
Copy link
Member

Thanks. Review 1 - good, test battery passes here. Over to @kaday

@hjoliver hjoliver assigned kaday and unassigned hjoliver Apr 27, 2016
@kaday
Copy link
Contributor

kaday commented Apr 28, 2016

Looks okay to me. Passes test battery in my environment.

@kaday kaday merged commit ffae12e into cylc:master Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants