Query API: Timestamp Query #771

haoxli · 2020-05-12T08:11:25Z

Add extension and entrypoints for timestamp query according to #614.

Preview | Diff

Kangz

LGTM overall

spec/index.bs

Kangz · 2020-05-20T09:38:11Z

@RafaelCintron @litherum @kvark please take a look!

kvark · 2020-05-20T14:08:06Z

@annevk pointed out that we might need to say a few words about the resolution of the timing here. More specifically, it will be subject to cross-origin policy and generally not exceed the resolution of performance.now().

I'm fine with following up with that, given that the security consideration section will also need to be adjusted.

Kangz · 2020-05-20T14:30:04Z

Timestamps should have no link with COOP or COEP. Or do you mean that you want timestamps to only be available when the page is COOP + COEP?

My understanding is that SharedArrayBuffer is gated on it because of Specter / Metldown but there is no speculative execution on the GPU so that shouldn't matter.

annevk · 2020-05-20T14:36:43Z

The concern is giving the page access to a high-resolution timer through the GPU.

Kangz · 2020-05-20T14:55:28Z

Timestamp queries give you high-resolution timers on the GPU but there significant roundtripping latency (that can easily vary in the millisecond range) between CPU and GPU so I don't think you'd be able to get any useful high-precision timer CPU timer.

That said timestamp queries are mostly useful for development (for telemetry there is the GPUCommandBuffer timing that we discussed and isn't in the spec yet), so restricting to COOP + COEP or being devtools enabled could be fine.

kainino0x · 2020-05-20T18:14:32Z

Even without the rountripping latency, I don't think there's a way to measure CPU time at all using timestamp queries. A timestamp query begins and ends in the same command buffer and measures only GPU-side work. It's not possible to do e.g.:

begin();
doSomeCPUWork();
end();

kvark · 2020-05-20T19:20:01Z

Sounds like this is mostly a non-concern, unless we start considering GPU precision timing to be an attack vector (as opposed to CPU precision timer). We can land the PR as is.

annevk · 2020-05-21T06:06:08Z

Does that mean https://gpuweb.github.io/gpuweb/#security-timing needs changes as well? It also feels a little brittle to rely on the latency between the CPU and GPU. But yeah, this should probably be discussed separately.

kainino0x · 2020-05-21T06:52:43Z

This definitely deserves a mention there, but like I explained I don't think we are actually relying on the latency between CPU and GPU.

You've reminded me about what is written in that section already, also. We do know we still have to figure out if there will be restrictions on the use of WebGPU (especially on multiple threads) given what's written there about the equivalence with SAB.

annevk · 2020-05-21T07:02:00Z

FWIW, what I was thinking about was more

getTimestampFromGPU()
doSomething()
getAnotherTimestampFromGPU()

Kangz · 2020-05-21T11:53:19Z

I guess you could submit the commands writing to a timestamp and then asynchronously read them back. Each of the two submissions would involve multiple syscalls, IPC between the content process and the GPU process, so it doesn't seem to practical.

What might be more practical is doing the same when software fallback is happening via Swiftshader since only the IPC calls are done. Maybe the timestamp feature could be force to disabled with Swiftshader.

kainino0x · 2020-05-21T19:45:54Z

Oh, I was thinking about disjoint timer queries, where there are no timestamps, only time durations. I forgot our timestamp queries are not disjoint. Sorry for the confusion.

kainino0x · 2020-05-21T19:46:17Z

Maybe we should consider disjoint timer queries to sidestep any high precision timer issue?

haoxli · 2020-05-22T13:55:46Z

Actually the timestamps returned to users will be a time delta processed by compute shader, the first timestamp will be 0 , the following values are the deltas based on the first one.

But no matter how to return the results of timestamp query, they're in nanosecond. If we want to sidestep potential risks in high precision, how about define an internal timestamp period to increase nanosecond interval like Vulkan did (VkPhysicalDeviceLimits::timestampPeriod).

annevk · 2020-05-26T08:09:08Z

Note that if a process that talks to the GPU is cross-origin isolated it should be fine for the timers to be as precise as can be (assuming the GPU cannot leak those to other processes that are not cross-origin isolated). And I think offering high-precision timers in cross-origin isolated cases is desirable as we do want to offer as much raw power as we can to those who isolate themselves.

Hope that helps.

kainino0x · 2020-05-26T17:40:03Z

Actually the timestamps returned to users will be a time delta processed by compute shader, the first timestamp will be 0 , the following values are the deltas based on the first one.

This is similar to performance.now(), and it still returns timestamps, not time deltas.

But yes, the timestamp values come from the GPU through the GPU process, so they're giving precise timing values for a different piece of hardware, through a different process and an IPC channel. I don't know exactly how much precision is needed (since timings can be amplified through multiple attempts), but the precision of a single timestamp would be very low (probably 0.25ms?).

Kangz · 2020-06-03T09:20:28Z

We've had good discussion about the potential risks of this API and mechanisms when it could be enabled, but there's been few comments on the shape of the API itself. It would be nice to get this landed if there are no more concerns about the shape. @kvark @litherum @JusSn do you have any comments on the API shape?

We should definitely keep discussing the constraints on availability, but since this is an optional feature of WebGPU, I think if we choose to add any constraints it will be easy to do so.

Kangz · 2020-06-03T09:21:32Z

spec/index.bs

+
+
+## Timestamp Query ## {#timestamp}
+Timestamp query allows application to write timestamp values to a {{GPUQuerySet}} by calling {{GPUProgrammablePassEncoder/writeTimestamp()}} on render pass or compute pass, and then resolve timestamp values in **nanoseconds** (type of {{GPUSize64}}) to a {{GPUBuffer}} (using {{GPUCommandEncoder/resolveQuerySet()}}).


Please add an Issue that points to this PR and explains that because the feature provides high-resolution GPU timestamps we need to decide what constraints, if any, are on its availability.

Done.
Do we need to open an issue for further discussion?

kvark

I'm fine with the shape, especially since it's an optional extension.
We can talk about the resolution of the timestamp query and the availability of the extension separately.
Also, if we decide to move this out into a separate document, we can also do this later.

kvark · 2020-06-03T17:24:03Z

spec/index.bs

@@ -3693,6 +3696,10 @@ interface mixin GPUProgrammablePassEncoder {
    void pushDebugGroup(USVString groupLabel);
    void popDebugGroup();
    void insertDebugMarker(USVString markerLabel);
+
+    void beginPipelineStatisticsQuery(GPUQuerySet querySet, GPUSize32 queryIndex);


can we limit this PR to timestamp queries only?

I think this is just a merge error; beginPipelineStatisticsQuery is already in master.

Yes, beginPipelineStatisticsQuery and endPipelineStatisticsQuery have been changed in #797, this two lines should be added when git rebase, I will remove them.

Oh, I forgot that timestamp query is also not supported on render bundle encoder, as same as pipeline statistics query, so we need to move writeTimestamp to render encoder and compute encoder.

beginPipelineStatisticsQuery and endPipelineStatisticsQuery have been changed in gpuweb#797, remove them in GPUProgrammablePassEncoder which are added when rebasing.

spec/index.bs

kainino0x · 2020-06-10T00:01:31Z

Travis hiccupped; tested build locally and force merging.

`maxStorageTexturesPerShaderStage` is `4` according to: https://www.w3.org/TR/webgpu/#dom-supported-limits-maxstoragetexturespershaderstage

Kangz approved these changes May 12, 2020

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x approved these changes May 14, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

kainino0x approved these changes May 14, 2020

View reviewed changes

annevk mentioned this pull request May 26, 2020

Add position on WebGPU (important) mozilla/standards-positions#341

Merged

Kangz reviewed Jun 3, 2020

View reviewed changes

haoxli added 5 commits June 3, 2020 17:48

Query API: Timestamp Query

d77aef1

Add writeTimestamp in GPUCommandEncoder

59bc226

Add extension requirements in queries

a2dd460

Add issue for constraints on the availability

c22f4ee

Unified text format

f16fdbe

haoxli force-pushed the timestamp-query branch from f7165d9 to f16fdbe Compare June 3, 2020 10:50

kvark requested changes Jun 3, 2020

View reviewed changes

Move writeTimestamp to render/compute encoders

7c478a8

beginPipelineStatisticsQuery and endPipelineStatisticsQuery have been changed in gpuweb#797, remove them in GPUProgrammablePassEncoder which are added when rebasing.

kvark reviewed Jun 4, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

haoxli commented Jun 5, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

spec/index.bs Show resolved Hide resolved

kvark approved these changes Jun 5, 2020

View reviewed changes

kainino0x approved these changes Jun 5, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

spec/index.bs Show resolved Hide resolved

Update spec/index.bs

6611370

kdashg approved these changes Jun 8, 2020

View reviewed changes

kainino0x reviewed Jun 8, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

Update spec/index.bs

53adb8e

kainino0x merged commit 4514260 into gpuweb:master Jun 10, 2020

kainino0x mentioned this pull request Apr 27, 2023

Timestamp query resets #4069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query API: Timestamp Query #771

Query API: Timestamp Query #771

haoxli commented May 12, 2020 •

edited by pr-preview bot

Loading

Kangz left a comment

Kangz commented May 20, 2020

kvark commented May 20, 2020

Kangz commented May 20, 2020

annevk commented May 20, 2020

Kangz commented May 20, 2020

kainino0x commented May 20, 2020

kvark commented May 20, 2020

annevk commented May 21, 2020

kainino0x commented May 21, 2020

annevk commented May 21, 2020

Kangz commented May 21, 2020

kainino0x commented May 21, 2020

kainino0x commented May 21, 2020 •

edited

Loading

haoxli commented May 22, 2020

annevk commented May 26, 2020

kainino0x commented May 26, 2020

Kangz commented Jun 3, 2020

Kangz Jun 3, 2020

haoxli Jun 3, 2020

kvark left a comment

kvark Jun 3, 2020

kainino0x Jun 3, 2020

haoxli Jun 4, 2020

haoxli Jun 4, 2020

kainino0x commented Jun 10, 2020



		## Timestamp Query ## {#timestamp}
		Timestamp query allows application to write timestamp values to a {{GPUQuerySet}} by calling {{GPUProgrammablePassEncoder/writeTimestamp()}} on render pass or compute pass, and then resolve timestamp values in nanoseconds (type of {{GPUSize64}}) to a {{GPUBuffer}} (using {{GPUCommandEncoder/resolveQuerySet()}}).

Query API: Timestamp Query #771

Query API: Timestamp Query #771

Conversation

haoxli commented May 12, 2020 • edited by pr-preview bot Loading

Kangz left a comment

Choose a reason for hiding this comment

Kangz commented May 20, 2020

kvark commented May 20, 2020

Kangz commented May 20, 2020

annevk commented May 20, 2020

Kangz commented May 20, 2020

kainino0x commented May 20, 2020

kvark commented May 20, 2020

annevk commented May 21, 2020

kainino0x commented May 21, 2020

annevk commented May 21, 2020

Kangz commented May 21, 2020

kainino0x commented May 21, 2020

kainino0x commented May 21, 2020 • edited Loading

haoxli commented May 22, 2020

annevk commented May 26, 2020

kainino0x commented May 26, 2020

Kangz commented Jun 3, 2020

Kangz Jun 3, 2020

Choose a reason for hiding this comment

haoxli Jun 3, 2020

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

kvark Jun 3, 2020

Choose a reason for hiding this comment

kainino0x Jun 3, 2020

Choose a reason for hiding this comment

haoxli Jun 4, 2020

Choose a reason for hiding this comment

haoxli Jun 4, 2020

Choose a reason for hiding this comment

kainino0x commented Jun 10, 2020

haoxli commented May 12, 2020 •

edited by pr-preview bot

Loading

kainino0x commented May 21, 2020 •

edited

Loading