Skip to content

Add the "discard-to-zero" store op.#358

Merged
Kangz merged 1 commit intogpuweb:masterfrom
Kangz:discard
Jul 23, 2019
Merged

Add the "discard-to-zero" store op.#358
Kangz merged 1 commit intogpuweb:masterfrom
Kangz:discard

Conversation

@Kangz
Copy link
Contributor

@Kangz Kangz commented Jul 4, 2019

spec/index.bs Outdated
enum GPUStoreOp {
"store"
"store",
"discard-to-zero"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather have either "discard" or "zero", but we definitely need this for efficient multisampling.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember some other options too, "invalidate", "mark-cleared"/"mark-zeroed", and then if we disallowed using uninitialized resources, "discard" or "invalidate"

Copy link

Choose a reason for hiding this comment

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

+1 for preferring "discard"
What's the difference between "discard" and "discard-to-zero"?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no true "discard" as it implies that it is zero-cost and the resource is uninitialized afterward, when in fact it is zeroed (possibly lazily) (like it would be immediately after being created).

Copy link

Choose a reason for hiding this comment

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

Seeing this begs the question of ""where's discard". What's the reason we don't want to allow a true Discard operation? Shouldn't the developers be in control over exactly when the Clear happens (and to what value)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, @Kangz !

To provide some more context to @damyanp - the problem with (2) was that it would limit the ability of the backend to handle this internally, since the API would have to be conservatively defined. For example, if we copy 2 texel rects into a texture, one for the bottom half and one for the top half, it's tricky to write specification in such a way that this is explicitly allowed.

With this in mind, (1) was considered the only viable option.

Copy link

@damyanp damyanp Jul 15, 2019

Choose a reason for hiding this comment

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

Option 2 still seems the better option to me. Is the issue with the conservative spec that you'd like to be able to partially initialize a resource? I'd be fine with prohibiting this and stating that initialization is either a clear or a full copy. I'll concede on this though, I've had my say :)

I will add thought that forcing users to read "discard-to-zero" when then mental model should really just be "discard" seems like unnecessary overhead. A casual developer who is used to using other APIs will need to read the docs to understand what all the "to-zero" stuff is about. Whereas someone who has thought deeply enough to be concerned about the possible implications of a raw "discard" operation would need to read the docs. I'd expect that the former would outnumber the latter.

Copy link

Choose a reason for hiding this comment

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

Keep in mind if we ship the conservative spec today, we can always add a "discard" with looser semantics tomorrow, but if we decide that "discard" was too loose, we won't really add anything with a "discard-to-zero" since we still have to support "discard".

Copy link
Contributor

Choose a reason for hiding this comment

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

My favorite plan for this is that "discard" is meant to yield undefined data, even if initial implementations may clear it.
What I don't want, even today, is to entice apps into using "discard-to-zero" to clear things. We should encourage explicit clearing, not reliance on side-effects of today's restricted behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already discussed this as part of issue #97. The consensus at the time was to call the StoreOp discard-to-zero.

@jdashg , if we want prevent people from forming the bad habit of using discard-to-zero to clear things, I think the best we can do is spec things such that the user is required to fill every byte of a discarded texture/buffer with data before they can use it in the pipeline. Doing this will require more work from implementations that wish to cut corners and eagerly clear discarded (or newly created) resources that the developer fills in multiple steps.

@kainino0x
Copy link
Contributor

Rebased over #365

Copy link

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

I think that this should be "discard".

@kdashg
Copy link
Contributor

kdashg commented Jul 18, 2019

I won't hold this up, but I think discard-to-zero is the wrong direction.

@kainino0x
Copy link
Contributor

Discussed in the 2019-07-22 meeting.

Resolution: rename to "clear", add a non-normative "Note:" that it's expected to be faster than a real clear.

@Kangz
Copy link
Contributor Author

Kangz commented Jul 23, 2019

Landing this with "clear" as was resolved in the call.

@Kangz Kangz merged commit 72b88fd into gpuweb:master Jul 23, 2019
@Kangz Kangz deleted the discard branch July 23, 2019 07:41
JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Jul 24, 2019
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.

7 participants