这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@kainino0x
Copy link
Contributor

@kainino0x kainino0x commented Jan 31, 2019

Completely separates Telemetry from Fallback/Fatal Errors. Simplifies the log entry API to hopefully include exactly what is necessary (although there is a TODO about whether object should be included).

@kainino0x kainino0x force-pushed the errors2-telemetry branch 3 times, most recently from ace9dfc to 7ab26f4 Compare January 31, 2019 22:25
Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

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

Per review on #197: I think it's worth continuing to categorize types of errors so that an application can, in a guaranteed fashion, watch for any out-of-memory errors and attempt to recover from them.

Per the other pull request, only adding fallible Promise-based allocation entry points for buffers and textures doesn't seem a robust enough mechanism.

Sorry for not thinking this through more in our face-to-face discussions.

What do you think?

@kainino0x
Copy link
Contributor Author

kainino0x commented Feb 1, 2019

EDIT: moved comment here: #197 (comment) because it makes more sense on that PR

@kenrussell
Copy link
Member

Consider this: OpenGL errors are categorized, and no more than one error of any particular type is reported at a time. Out-of-memory is essentially the only one an app would like to try to recover from. Does it seem a good idea to allow that recovery to be robust - no matter what kind of GPU object was allocated that triggered the OOM?

@beaufortfrancois
Copy link
Contributor

beaufortfrancois commented Feb 4, 2019

Thank you @kainino0x for doing this work! It is much easier to digest with 3 separate PRs.
Here are my 2 cents below.

TLDR;

  • Rename gpulogentry to validationerror for clarity.
  • Use an EventHandler for GPUDevice
  • Remove object and use debug label in newly renamed message attribute.

partial interface GPUDevice : EventTarget {
  attribute EventHandler onvalidationerror;
};

onvalidationerror is an EventHandler which is called whenever a validation error occurs in the API (including operations on "invalid" WebGPU objects).

[
    Constructor(DOMString type, GPUValidationErrorEventInit gpuValidationErrorEventInitDict),
    Exposed=Window
]
interface GPUValidationErrorEvent : Event {
  readonly attribute DOMString message;
};

dictionary GPUValidationErrorEventInit : EventInit {
  required DOMString message;
};

Fire an event named "validationerror" at GPUDevice using GPUValidationErrorEvent with its message attribute initialized to the validation error message. It should include the debug label of the object if possible.


Moreover, in your other PRs, you've include some Javascript examples. And I really liked it. Maybe doing the same here could be nice as well.

const gpuAdapter = await gpu.requestAdapter({ /* options */ });
const gpuDevice = await gpuAdapter.requestDevice({ /* options */ });

gpuDevice.addEventListener('validationerror', function(event) {
  console.log(event.message);
  // TODO: Push logs to remote server for telemetry.
});

@kainino0x
Copy link
Contributor Author

Applied your changes, thank you for the input! Not 100% certain that all log entries would be validation errors, but did that rename for now.

Can you check the wording for "fires on the main thread event loop"? Not sure how to word it properly.

Copy link
Contributor

@beaufortfrancois beaufortfrancois left a comment

Choose a reason for hiding this comment

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

By the way, did you have a look at https://developers.google.com/web/updates/2018/09/reportingapi?
It may be good to mention it when talking about telemetry.

  (An application may request a new device, or choose to fallback to other content.)
- `"out-of-memory"`: an allocation failed because too much memory was used by the application (CPU or GPU).
This includes recoverable out of memory errors that aren't opt-ed in to be handled by the application when the resource was created.
The `"validationerror"` event always fires on the main thread (Window) event loop.
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to use the existing WebGL task source as defined in https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15 and says something like

The task source for all tasks queued [HTML] in this section is the WebGL task source.

For info, in Picture-in-Picture, I wrote:

The task source for all the tasks queued in this specification is the media element event task source of the video element in question.

For more, check out https://cs.chromium.org/chromium/src/third_party/blink/public/platform/task_type.h

When there is a validation error in the API (including operations on "invalid" WebGPU objects), an error is logged.
When a validation error is discovered by the WebGPU implementation, it may fire a `"validationerror"` event on the `GPUDevice`.
These events should not be used by applications to recover from expected, recoverable errors.
Instead, the error log may be used for handling unexpected errors in deployment, for bug reporting and telemetry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: validation errors log

@kainino0x
Copy link
Contributor Author

@beaufortfrancois I have glanced at the Reporting API, but I haven't dug into it enough to understand whether it relates to the kind of reporting we need. Do you know whether we should try to integrate (or merge) this stuff with it?

@kainino0x
Copy link
Contributor Author

Reading a little further it seems like:

  • It could be good to surface these errors through it (but it would probably be ideal if we can surface only one error per app error?)
  • Not enough adoption to merge, but we can have our error stream feed into it later on

@grorg
Copy link
Contributor

grorg commented Feb 11, 2019

Discussed at the 11 Feb 2019 WebGPU Meeting

@kainino0x kainino0x merged commit 6274491 into gpuweb:master Feb 11, 2019
@kainino0x
Copy link
Contributor Author

Merged at the meeting, any further comments can be addressed later

@kainino0x kainino0x deleted the errors2-telemetry branch February 11, 2019 20:25
JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Mar 26, 2019
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
* fix warnings

* Replace usages of constants.ts with `as const`
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.

5 participants