Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rewrite dependency chapter #1833
base: main
Are you sure you want to change the base?
Rewrite dependency chapter #1833
Changes from all commits
9c317a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
"od" -> "of"
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.
Don't worry too much about typos for now. I will self-review stuff if and when we dedraft this and I will render it and run spellcheck.
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'd rather avoid saying "dependency" in a definition until after the concept has been introduced below (yes I know the text already says it, but we're improving things here 😬)
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.
Dependencies are introduced in the first paragraph of the chapter. For that matter it is the name of the chapter.
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.
By chapter I assume you mean section? And yes, they are, but the concepts themselves are being built up to be explained - they haven't been introduced at this point in the section other than by name. I think this suggestion pairs with the change here or it doesn't entirely make sense.
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.
Line 70, reword.
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'm not sure adding "an implementation's" here has the intended effect. My gut says maybe we need to work harder to separate the definition from who guarantees it.
Maybe:
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.
"happens-before" => "is executed" ? "happens" seems ambiguous.
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.
@stonesthrow I think this is basically first definition of what "happens-before" means, which has very formal meaning. But I think it is otherwisely unfortunately specified only in the extension.
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.
Yea, reading it back it seems this is what we were trying to use as a sort of bootstrapped definition of happens-before, and I don't think it does a good job in light of the memory model definition. "Happens before" is an industry term though (it's on wikipedia!) so I'm kind of inclined to leave it in pending a memory model rewrite that properly introduces the idea later...
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.
OK, if its a understood term. I do understand. But its vague in that "happens" by some unknown entity. Verses this entity does such and such before the next step.
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.
@stonesthrow It's a term defined in the memory_model extension, but it is used througout the specification.
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.
Possible issue here is that *S* itself is a set of operations, so would be included in *Ops*. I'm not 100% sure what the ramifications of that might be at the moment...
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.
@Tobski I try to make the scopes do all the work, so if they do not want to include S they just need to say so.
I have not had time to wholistically review stuff yet (hence draft PR), but typically the scope is writen like "all operations recorded before S that operate in stage P", therefore S should never be part of the scope.
I think it might catch other synchronization operations. Which might be a defect from before I think. Should be cleared all the same though. Going to keep this in mind when I update this PR...
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.
Yea that is all in line with my thinking, so that's fine, just something to be aware of.
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.
Possible alternative terms: A is the "prerequisite" operation(s) and B is the "dependent" operation(s).
Synchronization requires the prerequisite to execute before the dependent executes.
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.
@stonesthrow I don't need A and B here at all, because scopes are generally already defined as downselecting to previous or subsequent operations.
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.
Should this be Scope src not Src S? And similarly for the destination scope below?
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.
Yea.
Going back and forth on those var names. Suggestions?
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.
Hmm - Scope~src makes sense to me. I think that may be better than Src S because in some places in the spec the variable S is used to mean a synchronization command (rather than a scope).
Other people may have more ideas 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.
*Scope~src~* is fine, though I'd be tempted to call it *Scope~first~* (and second) to match the wording in the spec.
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.
@Tobski The nomenclature is sorta mixed. The API parameters use src/dst. TBH I like it more because first/second does not convey a difference between the scope in how they are used.
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.
We landed on first/second for some reason that I honestly can't entirely remember. I'm fine with src/dst here, but might be worth considering a nomenclature change for the spec elsewhere to match - though I'd do that in a separate MR because that's going to be churny.
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.
Unnecessary verbage, easier to read without IMO.
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.
It is sort of a legal doublet. "any and all" means "every". "any" alone is not exhaustive.
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 mean, I get it, but we consistently use "any" for this meaning by itself, precisely for the reasons I mentioned above. It'd be inconsistent to change that now.
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 language is a bit awkward. I'm inclined to say this is where using set theory makes sense. This is a definite intersection of two sets of the same type, and I'd suggest rewording it as such.
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.
It is best I came up with yet.
Problem is if you want it to be set intersection of same types, then what is the type? Set theory gives the wrong idea here and the wrong type of arithmetic. What I want here is "merger of two SQL queries" and not a set intersection. If I wanted set intersection, I would need a static type that forms static sets, which I can cleanly invoke set operations on. But scopes are sort of dynamic types. They are a predicate. You cannot make a set intersection of predicates. As far as I know (because nominally differing predicates can still have a common overlap, but that is not covered by set intersection operation. In a set intersection, elements are either completely identical or they are completely different and nothing between that.). If we really want to go uberformal, we need to introduce bunch of predicate logic theory.
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.
You've identified the two scopes as "sets of selection criteria" so you've identified the type already; calling out the intersection of those two sets as non-null suggests the selection criteria is empty. The only ambiguity here is whether this is an inclusive or exclusive set of criteria, which I don't think would be hard to clarify, and would dictate whether to use union or intersection here. The "predicate" idea you've suggested isn't something you've written down in this PR yet, and I'm not sure it would add anything.
We certainly can't introduce SQL queries as a concept in the Vulkan spec 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.
Well it does not unfortunately work that way. Consider this:
criterion1: "All operations after S1"
criterion2: "All operations before S2"
The strings are clearly different and the criteria are clearly different, therefore set intersection of set {criterion1} and set {criterion2} is an empty set {}, because criterion1 ≠ criterion2.
But the predicate criterion1 ∧ criterion2 yields a new nonempty (resp. non-tautological) predicate "All operations after S1 and before S2".
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'm confused, that sounds like exactly what we want? Where's the problem here?
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.
The problem here is set logic (set intersection) yields empty result, wheere predicate logic (predicate-and) would yield non-empty result (resp. technically non-tautological resulting predicate, without first prematurely evaluating the predicates). We want the predicate logic, and we do not want the set logic for the purposes of determining whether chain is formed. Therefore we cannot use set intersection to describe this, unless we invoke "ethereal operations" as the type of the elements of the set, which would be its own can of worms.
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.
Ok, I see what you're trying to get at here, I don't think the "hypothetical operations" is a much a can of worms as you think it is, and I'm half tempted to go draft that MR so we can compare.
In lieu of doing that though, it sounds like you have an idea to frame this as a sort of database query, which is a perfectly reasonable mental model, but I suspect that if you want to make the spec work that way, you've got a LOT more work to do. I'm concerned that by combining concepts like this PR is currently doing we're heading into a rabbit hole that will just confuse things more.
Like I think you have a vision here for how it's going to work, and it's not the one the spec has currently, and that's fine, but I don't know yet if there's any merit to that framing over what we've currently got.
Based on the discussion in #1815, it felt to me like there was a much smaller fix to that issue, and I'm concerned this PR is starting to drift too far from just solving that original issue. The set of changes to make it deal in "hypothetical operations" I suspect is much smaller.
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.
My problem with "hypothetical operation" is that forms a set of something unreal, magic, and uncountable. Can't say for sure without seeing this alternative, but I feel it will lead to similar vagueness as the original Issue points out. At least when sufficiently literal reader reads that.
After thinking about it. introducing "hypothetical operations" is sort of a hack\hotfix. And predicate logic is the appropriate indirection method to solve describing this situation. As it is now in the PR, it probably is not good either. But it feels one approach cannot be sufficiently formalized no matter how much one tries, while the other fundamentally can. I think the formalism is probably unavoidable, so I should steer the draft that way over the weekend. I could attempt to first make some quick fix if you want, but not sure if it is productive thing to do rather than a doubling of work.
I would avoid the tempting dopamine hit of promptly closing an Issue. I admit I am not a believer in atomic fixes. It is a rare day when something is amenable to fixing atomically in a way it never comes up in subsequent issues. Something top-down and interwoven as the sync system needs some care. At the minimum I consider the whole section to be undivisible and in need to be always edited as a whole.
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 don't think I disagree with much of that, except for your assertion that the other approach can't be formalized. Editing this chapter is hard. I agree that "hypothetical operation" may look like a hotfix and won't know until I try writing it out though.
There's an internal task to rewrite all of this incorporating the memory model language much better, because as you've pointed out there's no formal definition of happens-before without it, and literally nobody is looking forward to it :)
Because of that I'm a tad wary of a major rewrite just now; was hoping that you'd be able to get more with less here.
Ultimately, I tend to stick by the philosophy that "less is more" - if editing a chapter results in less words then it's a clear win. If it's about the same number of words then it's a much tougher sell. If there's more words, you've probably made it harder. This is a bit of a rough metric but I do find it helps when reviewing. So the thing I'd suggest is if you are planning on doing a lot of work here, plan it out first and try to find ways to say more with less. If you can reduce the number of concepts needed, I'd be much more inclined to approve 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.
Well, less words I can give you easily. Just delete all the words :p. Unfortunatelly there is another overriding priority: the words must describe completely, correctly, and coherently (4C) the things they describe.
It is unlikely I can substract concepts, because the Issue implies there actually is a concept missing (or not fully formed). What I try to do is first try to find the ideal state, and only then worry how I get there. Planning a route to a destination you don't know is a tricky business. Anyway I am happy with the name changes, which could now be a neat and likely noncontroversial patch that helps reasoning about all this stuff without getting lost.
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 this is a really helpful example.
I have one question about this. I could be wrong, but I'm thinking VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT might not be included in the combined selection criteria, because this stage happens before the fragment shader stage - does that seem correct to you?
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.
Copypasta error. I think they are listed this way in the enum, but in the graphics pipe there's just late test inbetween
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'm not keen on this specific example because it leans on the pipeline stage order, which isn't defined yet. I'd rather we highlight a simpler example here to avoid new readers getting confused - e.g. something that includes exactly the same stages directly. We could then elaborate on this example later when pipeline stage order is introduced to get that concept highlighted better (which might be a good improvement regardless).
For us I think this example makes sense, because we've read the spec and are familiar - but for a new reader this would be confusing.
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 do think examples might make clear some of the abstract language of scopes and dependencies. So I encourage that.
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.
@Tobski I try to make as brutal example here as possible. I suppose it is sort of a desperate attempt to compensate the lack of sufficiently formal description of what "scope overlap" is...
In my mind the concept is pretty straightforward, so it is mindly annoying that it might probably require like half a page to properly explain in the end. And even so will still probably require an example to have some certainty people grok it correct.
@stonesthrow Yea. I mean a presence of an example (similarly to code comments) indicates that the formal specifying text is lacking. But then again it is better to have crappy specifying language and an example, than have a crappy specifying language and no example...
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.
@krOoze I don't disagree with your intent, but I do think a fresh reader would not be able parse this particular example until they've gone and read a lot more of the chapter. IMO we need something in the style of the Vulkan Guide or a proposal document - where you get a much higher level view of things without all the formality. There you could say a lot of this much more straightforwardly.
In the meantime, I'd rather we simplify this particular example.
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.
@Tobski I was kinda strategically aiming for something hard. This is the where reader needs to slow down. The example demonstrates the original Issue. I suppose I can sacrifice the implicit pipeline stage part of the example for now?
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.
Right, the pipeline stage order is really my main concern here - the rest is fine.
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 a bit looser than I'd like, maybe:
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 don't see the difference. Can you point out what the underlying loosness is?
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.
Literally number of words needed to state the same concepts.
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.
Here you're leaning on submission order, which hasn't been introduced yet. In the execution dependency definition you did an excellent job of getting rid of this, and I think we could repeat that here.
The order of operations is unnecessary if you can formally define the way an execution dependency chain is formed by the intersection of scopes between operations.
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.
Yea it is just the chain of synch dependencies.
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.
IIRC these spaces lead to unnecessary gaps in the bullet list, as asciidoctor treats them as separate lists, so would like them removed.
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.
Don't worry too much about formatting for now. I will self-review stuff if and when we dedraft this and I will render it and run spellcheck.
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.
At some point we hope to better integrate the memory model appendix with the synchronization chapter in general, it's just not something we've had the resource to do, yet - I'm not sure an editors note here is necessary as this is tracked internally, but I'm not particularly against including it. For now we've just hidden it behind the extension as some of the definitions only somewhat apply without it.
Mostly just adding some context here, not necessarily suggesting a change.
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.
Particularly the "happens-before\after" is frequently used, so should be integrated with the core spec sooner rather than later...