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

doc: add note about AsyncResource for Worker pooling #28023

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 2, 2019

When implementing a pool for Worker threads, the correlation between
posting tasks and getting their results may get lost, depending on
the implementation.

The AsyncResource API is the primary way to solve that issue,
so link it from the recommendation in the worker docs.

(This was brought up at the collaborator summit in Berlin.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

When implementing a pool for Worker threads, the correlation between
posting tasks and getting their results may get lost, depending on
the implementation.

The `AsyncResource` API is the primary way to solve that issue,
so link it from the recommendation in the worker docs.

(This was brought up at the collaborator summit in Berlin.)
@addaleax addaleax requested a review from watson June 2, 2019 14:12
@addaleax addaleax added doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support. labels Jun 2, 2019
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@@ -48,6 +48,9 @@ if (isMainThread) {
The above example spawns a Worker thread for each `parse()` call. In actual
practice, use a pool of Workers instead for these kinds of tasks. Otherwise, the
overhead of creating Workers would likely exceed their benefit.
When implementing a worker pool, it is strongly recommended to use the
[`AsyncResource`][] API to inform diagnostic tools (for e.g. asynchronous stack
Copy link
Member

Choose a reason for hiding this comment

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

Typo: for e.g. should be either for example or e.g., although I would go with such as:

Suggested change
[`AsyncResource`][] API to inform diagnostic tools (for e.g. asynchronous stack
[`AsyncResource`][] API to inform diagnostic tools (such as asynchronous stack

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott I don’t think for example or just e.g. would work here, as asynchronous stack traces aren’t diagnostic tools by themselves, but rather provided by them. I’ve changed this to e.g. in order to provide asynchronous stack traces, maybe that works for you?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's great.

Copy link
Member

Choose a reason for hiding this comment

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

(Total ignorable nit: Maybe drop "in order"?)

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with typo fixed. One other optional suggestion: Set the new material off in a paragraph of its own?

@gireeshpunathil
Copy link
Member

@addaleax - does this mean that the result of a computation coming from a pooled thread can only be consumed by tools, not the calling code?

how about attaching a work id as an optional parameter to the worker constructor when being pooled, and then worker returning an object that contain this ID and the result?

@addaleax
Copy link
Member Author

addaleax commented Jun 9, 2019

@addaleax - does this mean that the result of a computation coming from a pooled thread can only be consumed by tools, not the calling code?

It means that the code that calls into worker pools and reads from them should perform async tracking. It means that tools that provide worker pools should do that, and in that case, the user of that tool should not need to be concerned with async tracking, because that’s already taken care of. Does that answer your question?

how about attaching a work id as an optional parameter to the worker constructor when being pooled, and then worker returning an object that contain this ID and the result?

I don’t understand what you are suggesting here, tbh. Each worker does get assigned a unique ID, and worker’s generally don’t return anything – they do emit message events, but you can use this.id inside the event handler to figure out that ID, so I’m not assuming that that’s what you’re talking about.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 9, 2019
@gireeshpunathil
Copy link
Member

@addaleax - thanks for the clarification. Yes, I was referring to the scenario wherein a number of tasks are pushed onto pooled threads, and be able to identify tasks when they complete and post messages / result. Your explanation helped me understand that the unique worker ID can be used for that purpose.

@addaleax
Copy link
Member Author

Landed in 55de209

@addaleax addaleax closed this Jun 10, 2019
@addaleax addaleax deleted the worker-pooling branch June 10, 2019 13:22
addaleax added a commit that referenced this pull request Jun 10, 2019
When implementing a pool for Worker threads, the correlation between
posting tasks and getting their results may get lost, depending on
the implementation.

The `AsyncResource` API is the primary way to solve that issue,
so link it from the recommendation in the worker docs.

(This was brought up at the collaborator summit in Berlin.)

PR-URL: #28023
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
When implementing a pool for Worker threads, the correlation between
posting tasks and getting their results may get lost, depending on
the implementation.

The `AsyncResource` API is the primary way to solve that issue,
so link it from the recommendation in the worker docs.

(This was brought up at the collaborator summit in Berlin.)

PR-URL: #28023
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants