Details
- Reviewers
kvark emilio - Commits
- Restricted Diffusion Commit
rMOZILLACENTRALbf288072dd8a: Bug 1527522 - Add external hooks to WR to allow integration with the Gecko…
Restricted Diffusion Commit
rMOZILLACENTRALbc69bea83b66: Bug 1527522 - Add external hooks to WR to allow integration with the Gecko… - Bugzilla Bug ID
- 1527522
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable. - Build Status
Buildable 40242 Build 51238: arc lint + arc unit
Event Timeline
This is a not-very-elegant integration of gecko profile markers. Maybe there is a cleaner / simpler way to implement this?
Listed a few suggestions below
| gfx/wr/webrender/src/frame_builder.rs | ||
|---|---|---|
| 377 | if we are using a macro anyway, maybe we can do "b" and "\0" in it? | |
| gfx/wr/webrender/src/profiler.rs | ||
| 28 | I think it needs to be &mut self or otherwise the API allows those calls to be done concurrently | |
| 41 | can we ask Gecko to just provide it in Renderer::new() constructor? | |
| 74 | can we end up in a situation when the PROFILER_HOOKS was None when we began the scope but Some when we end it? Perhaps, it should be Option<'static [u8]> for the name field to reflect this | |
| gfx/wr/webrender/src/frame_builder.rs | ||
|---|---|---|
| 377 | That'd be great, if possible! I'm no expert on macros though - do you know how to achieve this? | |
| gfx/wr/webrender/src/profiler.rs | ||
| 41 | I think the semantics are a bit clearer to have them set by a single function before creation of a renderer, since a new renderer is created for each window, but the intent is for the profiler to be truly global. | |
| 74 | I don't think so - because we guarantee via the init atomic that the hooks will only ever be set before any WR instances are created that could invoke the profile scope. The worst case here is that we call an unmatched end_marker, which the profiler handles. | |
| gfx/wr/webrender/src/frame_builder.rs | ||
|---|---|---|
| 377 | There's the cstr! crate that the style system uses for this. | |
| gfx/wr/webrender/src/profiler.rs | ||
|---|---|---|
| 36 | I'd feel a little better about this if this used an atomic pointer rather than static mut. This is completely UB if called from two different threads, right? Can we somehow guarantee that they're only called on a given thread? If not, can we avoid making begin_marker and end_marker &mut self? | |
| gfx/wr/webrender/src/profiler.rs | ||
|---|---|---|
| 42 | It's expected to be called in the current bindings each time a new window is created, so we can't assert here. | |
Rebased and fixed Cargo.lock in WR.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf3383f28bc5ab54dc585ae62eb959244dd5423c