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

Tasks can force foreground execution in the LocalDispatcher #927

Closed
4 tasks done
andrewkaufman opened this issue Jul 22, 2014 · 5 comments · Fixed by #951
Closed
4 tasks done

Tasks can force foreground execution in the LocalDispatcher #927

andrewkaufman opened this issue Jul 22, 2014 · 5 comments · Fixed by #951
Assignees
Labels
core Issues with core Gaffer functionality
Milestone

Comments

@andrewkaufman
Copy link
Contributor

Now that LocalDispatcher has an executeInBackground option, some of the tasks may need to force foreground execution (things that need doing on the main thread of the live app for example).

  • LocalDispatcher should add an executeInForeground plug to ExecutableNodes during setupPlugs(), which overrides it's own executeInBackground plug.
  • Tasks executed in the foreground use ExecutableNode::execute() directly rather than the gaffer execute app.
  • Any node with executeInForeground on forces all of its requirements to execute in the foreground as well.
  • All tasks use ExecutableNode::execute() directly when executeInBackground is off.
@johnhaddon
Copy link
Member

Do you think there's a benefit to this being a user-facing option? I've been trying to think of use cases outside of a hosted app, and struggled a little bit. The best I can come up with is if you know that you have some tasks which could execute much more quickly in the foreground, because they're already in the cache, whereas they'd have to be recomputed if done in the background.

In the use case that motivates this, would we ever want the user to turn executeInForeground off? If not, perhaps it's worth registering a null PlugValueWidget creator to hide it from the UI?

@andrewkaufman
Copy link
Contributor Author

I'll have a think about it, though I think your user-knows-whats-quicker example is a pretty good one. On the particular nodes we're talking about in the hosted app, I'd imagine they'd either hide or lock it since it needs to be True.

@andrewkaufman
Copy link
Contributor Author

Regarding the 3rd tick box "Any node with executeInForeground on forces all of its requirements to execute in the foreground as well."

The simplest implementation of this isn't ideal. My idea was to look at the TaskDescriptions and find the last one with an executeInForeground node. All tasks before that would be in the foreground, and all after would be in the background. In a complex setup this can result foreground execution for several tasks that could have run in the background if we did something smarter. This happens because some of the tasks from a foreground node appear in the TaskDescriptions after some of the tasks from a background node (there is no requirement between the 2 nodes).

Do you have any ideas regarding this? Did I even explain that in a way that makes sense to anyone but me?

@johnhaddon
Copy link
Member

Yeah, it's almost like you need some sort of tree representation, so as you walk the tree from the root you can turn on the "executeInForeground" flag when you hit one of those nodes, and that flag is then on for any requirements that you subsequently recurse to. I wonder if we have a design for some sort of tree of batches of tasks knocking around anywhere?

@andrewkaufman
Copy link
Contributor Author

Hardee-har-har. I guess I'll get on that redesign then...

andrewkaufman added a commit to andrewkaufman/gaffer that referenced this issue Aug 7, 2014
…ching.

Each ExecutableNode has an executeInForeground plug in the Local section of the Dispatcher plugs. This is used by the LocalDispatcher to determine when it's safe to start dispatching from a background thread (if the executeInBackground plug is on in the first place).

Fixes GafferHQ#927.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues with core Gaffer functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants