Skip to content

Conversation

kainino0x
Copy link
Contributor

@kainino0x kainino0x commented Jun 19, 2020

These methods are simple and extremely useful. They allow programs to
easily wait for these long running compilation operations to complete
before issuing work that blocks on them, making it easier to avoid
hitches in webpage (and whole-browser) rendering.

Previously I removed these in favor of wrapping them in error scopes
with GPUErrorFilter "none" (#238) as I felt that they overlapped in
functionality. However, the createPipeline methods are the most
significant source asynchronous resource creation blocking queue
timeline work.

TODO: (If there are others we think are important, we should either
unremove GPUErrorFilter "none" or give them Promise-based versions.)


Preview | Diff

These methods are simple and extremely useful. They allow programs to
easily wait for these long running compilation operations to complete
before issuing work that blocks on them, making it easier to avoid
hitches in webpage (and whole-browser) rendering.

Previously I removed these in favor of wrapping them in error scopes
with GPUErrorFilter "none" as I felt that they overlapped in
functionality. However, the createPipeline methods are the most
significant source asynchronous resource creation blocking queue
timeline work.

TODO: (If there are others we think are important, we should either
unremove GPUErrorFilter "none" or give them Promise-based versions.)
@kainino0x
Copy link
Contributor Author

Apologies for the late PR. Hopefully we will be able to discuss this during the VF2F since it's just reintroducing a concept we previously removed.

@kvark
Copy link
Contributor

kvark commented Jun 22, 2020

Could you clarify this bit:

However, the createPipeline methods are the most
significant source asynchronous resource creation blocking queue
timeline work.

So the motivation here is driven by the fact this will be most demanded? Is there a downside for the user to just have the polyfill instead:

async function createReadyRenderPipeline(device, desc) {
  device.pushErrorScope('none');
  const pipeline = device.createRenderPipeline(desc);
  await device.popErrorScope(); // always resolves to null
  return pipeline;
}

I thought letting the user to decide the scope has its advantages beyond spec simplicity: they can create multiple pipelines in this scope, for example, or do things like resource creation (which may also take time).

@kainino0x
Copy link
Contributor Author

It allows us to not spec that an error scope surrounding a createPipeline has to wait for actual compilation (just for validation). I think this is the only place in the API where we currently need error scopes to act as "fences" on anything other than validation and recoverable allocation, so I think this significantly simplifies the concept of error scopes (and reduces (user) confusion and (our) uncertainty about what exactly they should fence on).

@kainino0x
Copy link
Contributor Author

To address some more I didn't reply to:

Is there a downside for the user to just have the polyfill instead

That requires us to continue supporting pushErrorScope('none') and guaranteeing its timing behavior relative to the compilation of the pipeline. With 'none' removed, the other option is 'validation' which only waits for validation (e.g. can wait for the WGSL frontend but not the backend going to SPIR-V/MSL/HLSL/etc.)

can create multiple pipelines in this scope

I think Promise.all is okay for this purpose.

or do things like resource creation

I'm not so concerned about the time it takes to create resources, unless they're very large - in which case it's generally necessary to use an out-of-memory error scope anyway which will have the desired effect.

We could maybe rename the 'out-of-memory' error scope to 'allocation' to clarify that it waits for the full allocation. (But the behavior is exactly the same - we always have to wait for allocation to know whether out-of-memory has occurred.)

@kvark kvark changed the base branch from master to main June 23, 2020 13:13
@kainino0x
Copy link
Contributor Author

kainino0x commented Jul 6, 2020

TODO(@kainino0x) double check the minutes to make sure this was resolved, then get it landed

@kainino0x
Copy link
Contributor Author

Resolved error case per editors' meeting. Landing with agreement from vf2f.

@kainino0x kainino0x merged commit 7b53e30 into gpuweb:main Jul 6, 2020
@kainino0x kainino0x deleted the createReadyPipeline branch July 6, 2020 22:47
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
* Few test plan updates

* Update documentation for CTS project tracker
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.

3 participants