Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webgl profiling #22130

Closed
wants to merge 6 commits into from
Closed

Webgl profiling #22130

wants to merge 6 commits into from

Conversation

@cbrewster
Copy link
Member

cbrewster commented Nov 6, 2018

This adds additional profiling for WebGL commands and some WebGL DOM APIs using the existing profiling infrastructure.

I don't really like adding a ton of new profiling category variants, but it seemed like the simplest approach given how signpost works with repr[u32] and using a bitwise shift for the event category and how characterize.py works.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21150 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because it adds additional profiling

This change is Reviewable

@highfive
Copy link

highfive commented Nov 6, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webglrenderingcontext.rs
  • @paulrouget: components/servo/lib.rs
  • @KiChjang: components/script/dom/webglrenderingcontext.rs
@highfive
Copy link

highfive commented Nov 6, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2018

The latest upstream changes (presumably #22126) made this pull request unmergeable. Please resolve the merge conflicts.

@cbrewster cbrewster force-pushed the cbrewster:webgl_profile branch 2 times, most recently from 0f0bd72 to 0898440 Nov 19, 2018
@cbrewster cbrewster changed the title [WIP] Webgl profiling Webgl profiling Nov 19, 2018
@jdm
Copy link
Member

jdm commented Nov 19, 2018

r? @nox

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2018

The latest upstream changes (presumably #22225) made this pull request unmergeable. Please resolve the merge conflicts.

@nox
Copy link
Member

nox commented Nov 21, 2018

Is there really no way to avoid all those new categories?

@@ -1703,3 +1711,147 @@ fn map_dot_separated<F: Fn(&str, &mut String)>(s: &str, f: F) -> String {
}
mapped
}

fn profiler_category(webgl_command: &WebGLCommand) -> ProfilerCategory {

This comment has been minimized.

Copy link
@nox

nox Nov 21, 2018

Member

This could be generated with a macro wrapping this function and the definition of WebGLCommand.

@cbrewster
Copy link
Member Author

cbrewster commented Nov 21, 2018

There’s no good way to avoid adding new categories since we have to strip away any associated data from the WebGLCommand. I suppose we could use a macro to generate a new enum that is the same as WebGLCommand but without any associated data

@cbrewster cbrewster force-pushed the cbrewster:webgl_profile branch 2 times, most recently from 2336380 to 8cf7313 Dec 10, 2018
@cbrewster cbrewster force-pushed the cbrewster:webgl_profile branch from 8cf7313 to 746578c Dec 11, 2018
@cbrewster cbrewster force-pushed the cbrewster:webgl_profile branch 3 times, most recently from 2168060 to 142256b Dec 11, 2018
@cbrewster cbrewster force-pushed the cbrewster:webgl_profile branch from 142256b to 1097e7f Dec 11, 2018
@cbrewster
Copy link
Member Author

cbrewster commented Dec 11, 2018

Sorry had some git issues...
I added a macro that automatically generates the conversion code between a WebGLCommand and ProfilerCategory. I wasn't sure how to do it via a macro_rules so I made a custom derive. I've not written many macros, so it may need some improvement.

@cbrewster cbrewster assigned nox and unassigned ferjm Dec 11, 2018
@@ -157,6 +157,145 @@ impl Formattable for ProfilerCategory {
ProfilerCategory::IpcReceiver => "Blocked at IPC Receive",
ProfilerCategory::IpcBytesReceiver => "Blocked at IPC Bytes Receive",
ProfilerCategory::ApplicationHeartbeat => "Application Heartbeat",
ProfilerCategory::WebGlGetContextAttributes => "WebGlGetContextAttributes",

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 11, 2019

Member

can we (proc?)macro-generate all of these to avoid clutter?

This comment has been minimized.

Copy link
@cbrewster

cbrewster Feb 15, 2019

Author Member

We could just do a Derive(Debug) here, but we would lose the + and | + formatting for subcategories. Although we could just derive debug, and then just manually add the prefix to the few categories that need them. The output would be like IpcReceiver rather than Blocked at IPC Receive though.

use syn::{Data, DeriveInput, Fields, Ident, Lit, Meta, MetaNameValue};

#[proc_macro_derive(ToProfile, attributes(profile_prefix))]
pub fn derive_to_profile(item: proc_macro::TokenStream) -> proc_macro::TokenStream {

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 11, 2019

Member

please add some comments explaining the transformation being made here

@jdm jdm assigned Manishearth and unassigned nox Feb 11, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Feb 27, 2019

The latest upstream changes (presumably #22938) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm mentioned this pull request May 1, 2019
2 of 2 tasks complete
bors-servo added a commit that referenced this pull request May 6, 2019
Remove mozjs dep from malloc_size_of.

This makes making local changes to mozjs_sys much more frustrating than it should be, and blocks merging #22130.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23299)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request May 6, 2019
Remove mozjs dep from malloc_size_of.

This makes making local changes to mozjs_sys much more frustrating than it should be, and blocks merging #22130.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23299)
<!-- Reviewable:end -->
@cbrewster
Copy link
Member Author

cbrewster commented Jun 25, 2019

I don't have the time at the moment to fix this up, sorry for blocking this so long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.