-
Notifications
You must be signed in to change notification settings - Fork 67
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
Small clarification to buffer sync rules #291
Small clarification to buffer sync rules #291
Conversation
The section on buffer synchronization rules define the scenarios when the buffer destructor must block vs. the scenarios where it does not block. The old wording made it seem like a buffer created from no host pointer is only non-blocking if it uses the default memory allocator. I assume this was not our intent, and such a buffer should have a non-blocking destructor regardless of the memory allocator. The main change in this PR is to remove the phrase "and using the default buffer allocator". However, I also reworded a bit to clarify which type of buffers we are talking about.
We do not have any issue about this, so we haven't discussed whether this is the correct interpretation of the spec. Our implementation team raised a question about how to implement buffer destructors, though, and I think this PR reflects what we probably intended. Feel free to object if you think this is not what we meant. |
Our implementation team raised a counter argument: What if the allocator is user-supplied and stateful. For example, consider an allocator that returns blocks of memory from a pool. If the buffer destructor doesn't block, how does the application know when there are no more buffers that are using the pool? If we wanted to preserve the "does not block" semantic, I think we could add some guarantee that limits the lifetime of the runtime's use of the allocator. For example, we could say that any memory allocated for a buffer is guaranteed to be deallocated by the time all commands with accessors to that buffer have completed. There is also another option, which is a somewhat larger change. I've always thought that the rules about when the buffer blocks are pretty complex. We could simplify this and say that the buffer destructor always blocks until all commands with accessors to that buffer have completed. This seems like a much simpler rule, which is easy to understand. It also addresses the outstanding issue in #240. |
Changing the blocking behaviour is a good F2F meeting subject. :-) Invite your colleague @andrewrichards since he was a proponent of this behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
. A buffer can be constructed from a [code]#hostData# pointer. The buffer | ||
will use this host memory for its full lifetime, but the contents of this | ||
host memory are unspecified for the lifetime of the buffer. If the host | ||
memory is modified by the host, or mapped to another buffer or image during |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR but I have the feeling that this mapped
concept is outside of the scope of SYCL since it is either an OS concept or more probable here an OpenCL SYCL 1.2.1 from the past.
At the end, mapped
is kind of modifying the memory, at least making it unavailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that statement is just saying that you cannot construct another buffer from the same hostData
pointer. I agree that the word "mapped" is a bit confusing, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarified the wording here in 87a71b1
add a stateful allocator to track the state? add wording |
Avoid the word "mapped" and say "used to construct" instead.
Clarify that the implementation may keep a copy of a buffer allocator even after the buffer is destroyed.
I added some information about the use of buffer allocators in f4f70c7. I think this PR is ready now, so please re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good clarification.
then the results are undefined. The initial contents of the buffer will | ||
be the contents of the host memory at the time of construction. | ||
. A buffer can be constructed from a [code]#hostData# pointer. The buffer | ||
will use this host memory for its full lifetime, but the contents of this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious about plural of contents
. In French I would use singular, so just checking. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complicated in English! We use "content" for things that are uncountable and "contents" for things that are countable. It's not entirely clear whether it should be "content of memory" or "contents of memory". When I do a Google search of each, I see there are about the same number of occurrences of each. It seems like the SYCL spec use "contents of memory" in many places, so I think we should use that consistently here.
Co-authored-by: Ronan Keryell <[email protected]>
Small clarification to buffer sync rules
The section on buffer synchronization rules define the scenarios when the buffer destructor must block vs. the scenarios where it does not block. The old wording made it seem like a buffer created from no host pointer is only non-blocking if it uses the default memory allocator. I assume this was not our intent, and such a buffer should have a non-blocking destructor regardless of the memory allocator.
The main change in this PR is to remove the phrase "and using the default buffer allocator". However, I also reworded a bit to clarify which type of buffers we are talking about.