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

Clarify that capture by ref host_task requires caution #515

Merged

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Dec 7, 2023

C++ lambdas if capturing by ref need to be aware of the lifetime of the referred-to objects. This adds this information here as well.

Motivation:

oneapi-src/oneDNN#1767

must ensure that referred-to user defined classes use appropriate reference
counting. Since the <<host_task>> callable may execute at an unspecified time
between a command submission and a synchronization point, its execution may
outlive the referred-to objects if reference counting is not used.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems too specific to a particular bug that you had, and the recommendation about reference counting is only one possible solution. I don't think we need to add any new formal requirements to the spec here. We already say that a host task's execution is deferred and that it executes asynchronously. A logical outcome of this is that you need to take care when using pointers or references inside the host task.

If we want to add something to the spec, I'd suggest a non-normative note like this:

[NOTE]
====
Care must be taken when using pointers or references inside a host task when the pointer
or reference refers to an object that is defined outside of the host task.  Because the host
task is executed asynchronously, applications should ensure that the referenced object's
lifetime lasts at least until the host task completes.
====

Copy link
Contributor

Choose a reason for hiding this comment

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

Is reference counting even a solution? This is still a bug:

void submit_host_task(sycl::queue& q, std::shared_ptr<int> /* by value */ something) {
  q.submit([&](sycl::handler& cgh) {
    cgh.host_task([&]() {
      printf("%d\n", *something); // oops, `something` went out of scope
    });
  });
}

A classic footgun is to capture host-task accessors by reference. Maybe it's worth pointing that out specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for comments. Have rephrased to advise caution

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

This seems like a normal caveat with any lambda not in an immediate invocation context.

@hdelan hdelan force-pushed the clarify-host-task-ref-val-capture branch from 61fa780 to f88128c Compare December 7, 2023 15:58
@hdelan
Copy link
Contributor Author

hdelan commented Dec 7, 2023

@keryell I agree. I think it is worth reiterating here, but can close this PR if you think it doesn't belong in the spec

@hdelan hdelan changed the title Clarify that capture by ref requires ref counting in host task Clarify that capture by ref requires caution Dec 7, 2023
@hdelan hdelan changed the title Clarify that capture by ref requires caution Clarify that capture by ref host_task requires caution Dec 7, 2023
@gmlueck
Copy link
Contributor

gmlueck commented Dec 7, 2023

Hi @hdelan, we have some new formatting requirements for the *.adoc files, and these are checked by our CI system. One of these checks failed for this PR. Can you do the following:

  • Run: ./adoc/scripts/reflow.py -overwrite adoc/chapters/programming_interface.adoc
  • This will make some formatting changes to "programming_interface.adoc". Check that nothing unexpected happened.
  • Commit the new changes and push the commit to this PR.

Thanks.

@hdelan hdelan force-pushed the clarify-host-task-ref-val-capture branch from f88128c to fd67962 Compare December 7, 2023 19:53
@hdelan
Copy link
Contributor Author

hdelan commented Dec 7, 2023

Hi @hdelan, we have some new formatting requirements for the *.adoc files, and these are checked by our CI system. One of these checks failed for this PR. Can you do the following:

* Run: `./adoc/scripts/reflow.py -overwrite adoc/chapters/programming_interface.adoc`

* This will make some formatting changes to "programming_interface.adoc".  Check that nothing unexpected happened.

* Commit the new changes and push the commit to this PR.

Thanks.

Thanks @gmlueck have done that

@hdelan
Copy link
Contributor Author

hdelan commented Dec 7, 2023

@gmlueck should I add my name to acknowledgements.adoc?

@gmlueck
Copy link
Contributor

gmlueck commented Dec 8, 2023

@gmlueck should I add my name to acknowledgements.adoc?

Yes, if you like.

@hdelan
Copy link
Contributor Author

hdelan commented Dec 8, 2023

@gmlueck thanks, done

If a C++ lambda is passed to a <<host-task>>, the lambda may capture by
reference or by value.
Since the <<host-task>> callable executes asynchronously, care must be taken to
ensure that lifetimes of objects captured by reference by a <<host-task>> lambda
Copy link
Member

Choose a reason for hiding this comment

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

Even for objects captured by copy you still need to unsure that it works under the hood with the program logic.
The trivial example is a std::ref or a pointer captured by copy. ;-)

@gmlueck
Copy link
Contributor

gmlueck commented Dec 14, 2023

WG approved

@gmlueck gmlueck merged commit bfe3330 into KhronosGroup:SYCL-2020/master Dec 14, 2023
1 check passed
keryell pushed a commit that referenced this pull request Sep 10, 2024
Clarify that capture by ref host_task requires caution
gmlueck added a commit that referenced this pull request Nov 7, 2024
Clarify that capture by ref host_task requires caution

(cherry picked from commit bfe3330)
gmlueck added a commit that referenced this pull request Nov 7, 2024
Clarify that capture by ref host_task requires caution

(cherry picked from commit bfe3330)
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.

6 participants