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

Tighten synchronization scope specification #1843

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krOoze
Copy link
Contributor

@krOoze krOoze commented May 8, 2022

limited and evolved version of #1833
fix #1815

Tighten the definition of what "synchronization scope" actually is.
That should resolve confusions about using set intersections in the specification, and confusions about execution dependency chaining, supplemented by an example.

ping @williamih, @Tobski, @stonesthrow for input

which should resolve confusions about set intersections
and dependency chaining
@williamih
Copy link

Thanks @krOoze - I like the predicate logic approach. Will have a look at this in more detail when I get a chance (probably in the next few days)

* Let *A'* be the set of operations satisfying *A~S~*.
* Let *B'* be the set of operations satisfying *B~S~*.
* Submitting *S* for execution will result in an execution dependency
*E* between *A'* and *B'*.

Choose a reason for hiding this comment

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

I think the use of predicate logic here is great. This seems intuitive and also more precise than the current spec.

I'm wondering what you think about this potential adjustment, which uses some more of the current wording:

* Let *A* and *B* be separate sets of operations.
* Let *S* be a synchronization command.
* Let *A~S~* be the first synchronization scope of *S*.
* Let *B~S~* be the second synchronization scope of *S*.
* Let *A'* be the set of operations in *A* satisfying *A~S~*.
* Let *B'* be the set of operations in *B* satisfying *B~S~*.
* Submitting *A*, *S* and *B* for execution, in that order, will result in
  execution dependency *E* between *A'* and *B'*.

The reasoning being that, at least to me, this makes it a bit more clear that *B'* can include operations that are submitted later. The note you have below clarifies this, which is great, but I think it would be nice if we could have this be really clear just from the main text of the spec, so that the note is less essential.

Arguably the update I'm suggesting is a bit redundant, because the predicates already "select" the operations, but I do think it reduces the need for the extra note. (My understanding is notes are meant to be more of an optional extra, and not essential for understanding the spec).

Copy link
Contributor Author

@krOoze krOoze Oct 12, 2022

Choose a reason for hiding this comment

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

Sorry for late answer. Yea it is somewhat annoying B is a temporal set. One has to go outside spacetime to see the full extent of B.

Notes\comments are always desperate measures. Make no mistake, I would do everyting humanely possible to try to eliminate them.

What you suggest is there originally, and what I wanted to avoid, because it makes several inaccurate preassumptions. For example, let's say S is a vkWaitForFences. What does it even mean "submitting vkWaitForFences in order with B"? There's not even any queue to establish order. The thing that actually says what is covered by the synchronization, is the synchronization scope, not the order in which operations are submitted. For some hypothetical new S command, A should even be submitted after S, because why not (it could be sync command designed to establish dependency between two things in the future that are yet to be submitted).

@krOoze
Copy link
Contributor Author

krOoze commented Oct 12, 2022

Dammit, this got forgotten. Gotta rebase this...

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.

Definition of synchronization scopes as sets
2 participants