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

Move description of shared_ptr buffer destructor #298

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Oct 13, 2022

The buffer destructor has special rules about blocking when the buffer is constructed from a shared_ptr. Previously, these rules were described in section 4.7.4.3 "Shared SYCL ownership of the host memory". However, the behavior of the buffer destructor in other cases is described in section 4.7.2.3 "Buffer synchronization rules". This PR moves the description to 4.7.2.3 in order to keep the descriptions of the buffer destructor all in one place.

After doing this, I realized that section 4.7.4.3 did not contain any new information that wasn't already described in 4.7.2.3. Therefore, I removed that section.

I also tightened the specification a bit for the case when a buffer is constructed from a shared_ptr:

  • If the underlying type of the shared_ptr is const, then I assume we want the buffer to be read-only, just as in the raw pointer case.

  • I also assume that the contents of the host memory referenced by the shared_ptr is undefined during the lifetime of the buffer, just as in the raw pointer case.

The buffer destructor has special rules about blocking when the buffer
is constructed from a `shared_ptr`.  Previously, these rules were
described in section 4.7.4.3 "Shared SYCL ownership of the host memory".
However, the behavior of the buffer destructor in other cases is
described in section 4.7.2.3 "Buffer synchronization rules".  This PR
moves the description to 4.7.2.3 in order to keep the descriptions of
the buffer destructor all in one place.

After doing this, I realized that section 4.7.4.3 did not contain any
new information that wasn't already described in 4.7.2.3.  Therefore, I
removed that section.

I also tightened the specification a bit for the case when a buffer is
constructed from a `shared_ptr`:

* If the underlying type of the `shared_ptr` is `const`, then I assume
  we want the buffer to be read-only, just as in the raw pointer case.

* I also assume that the contents of the host memory referenced by the
  `shared_ptr` is undefined during the lifetime of the buffer, just as
  in the raw pointer case.
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.

Globally that sounds like a good simplification and clarification.

}
----


Copy link
Member

Choose a reason for hiding this comment

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

Should we keep these examples?

Copy link
Member

Choose a reason for hiding this comment

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

I have checked again : these were the only 2 code examples showing sycl::buffer and std::shared_ptr. Why removing them? Are they wrong now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the examples are good to keep. I was hoping to move all of the normative description of the buffer APIs to section 4.7.2 Buffers, and in particular the normative description of the blocking behavior and copy-back behavior to section 4.7.2.3 Buffer synchronization rules.

How do you feel about the following ... I'll still augment section 4.7.2.3 to contain a complete description of the blocking behavior and copy-back behavior, so all of that information is together in one place. We can still keep section 4.7.4, though, as a set of examples. In this case, I'd probably retitle the section to something like "Buffer examples", and the introduction of that section will read something like:

This section provides some examples showing typical use cases for buffer. These examples are intended to clarify the definition of the buffer interfaces, but the content of this section is nonnormative.

Over time, we can add more examples to this section to clarify other behavior about buffers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I restored the examples, and converted section 4.7.4 to an examples section as I describe in the comment above. See commit 15e8eab.

Note that this PR is incomplete at this point because I need answers to the questions in (https://gitlab.khronos.org/sycl/Specification/-/issues/478)

buffer<int, 1> b { ptr, range<2>{ 10, 10 } };
// update the data
[...]
ptr.reset();
Copy link
Member

Choose a reason for hiding this comment

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

This line require a comment, for casual readers not using routinely std::shared_ptr.

}
----


Copy link
Member

Choose a reason for hiding this comment

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

I have checked again : these were the only 2 code examples showing sycl::buffer and std::shared_ptr. Why removing them? Are they wrong now?

* Convert section 4.7.4 "Sharing host memory with the SYCL data
  management classes" into a non-normative set of examples and retitle
  the section "Example buffer usage".  This restores the examples
  removed in the previous commit, adds some new examples, and moves the
  existing example about sub-buffers to this section.

* The existing example for sub-buffers was updated to use SYCL 2020
  features that reduce its verbosity.

* Add more clarifications to section 4.7.2.3 "Buffer synchronization
  rules" and reorder some of the information to make it flow better.

The descriptions of `set_final_data` and `set_write_back` are
incomplete, pending answers to questions in internal issue 478.
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.

Great clarification!

@keryell
Copy link
Member

keryell commented Mar 16, 2023

Dedicate some time like 1 hour to solve this after some preparing time.

@fraggamuffin
Copy link

A good F2F

Comment on lines +4984 to +4987
In general, the buffer destructor blocks under two circumstances: if the buffer
assume exclusive access to some host memory passed to its constructor or if
some data needs to be copied back to the host upon completion of commands
associated with the buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In general, the buffer destructor blocks under two circumstances: if the buffer
assume exclusive access to some host memory passed to its constructor or if
some data needs to be copied back to the host upon completion of commands
associated with the buffer.
In general, the buffer destructor blocks under two circumstances: if the buffer
assumes exclusive access to some host memory passed to its constructor or if
some data needs to be copied back to the host upon completion of commands
associated with the buffer.

Comment on lines +4989 to +4990
The following paragraphs describe these rules more precisely for each overload
of the buffer constructor:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're missing the sub-buffer overload here. I see no explicit mention of sub-buffer synchronization rules neither. In the original text, sub-buffers are mentioned at the end of the section, but no synchronization rules are specified. I assume sub-buffers destruction does not block, only the buffer one would, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants