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

Register thread to WR's profiler and the external profiler. #2014

Merged
merged 2 commits into from
Nov 10, 2017

Conversation

JerryShih
Copy link
Contributor

@JerryShih JerryShih commented Nov 8, 2017

The gecko profiler needs the thread to register itself to gecko profiler before collecting the performance data. In this patch, we have a new "thread listener" interface to do somethings when the thread is created and deleted. Then, we could put the registering function inside the listener interface.


This change is Reviewable

@@ -1787,33 +1787,60 @@ impl Renderer {
let debug_flags = options.debug_flags;
let payload_tx_for_backend = payload_tx.clone();
let recorder = options.recorder;
let worker_config = ThreadPoolConfig::new()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The worker_config is only used when we don't have the thread pool. So, move the config into the worker's unwrap_or_else() block.

pub enable_render_on_scroll: bool,
pub debug_flags: DebugFlags,
pub renderer_id: Option<u64>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might have multiple renderer in the same process. Use this renderer_id for identification.

@JerryShih
Copy link
Contributor Author

@glennw
Copy link
Member

glennw commented Nov 9, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


webrender/src/renderer.rs, line 1799 at r1 (raw file):

                let worker_config = ThreadPoolConfig::new()
                    .thread_name(|idx|{ format!("WRWorker#{}", idx) })
                    .start_handler(move |idx| {

I think the start and end handlers are only called when the worker thread is created and destroyed. If so, that won't actually be useful for profiling - since we won't know when the thread is doing work or idle?


webrender/src/renderer.rs, line 1816 at r1 (raw file):

        let blob_image_renderer = options.blob_image_renderer.take();
        let thread_listener_for_render_backend = thread_listener.clone();
        let mut thread_name = String::from("WRRenderBackend");

This could be something like:

let thread_name = format!("WRRenderBackend {}", options.renderer_id.unwrap_or(0));


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Nov 9, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit e9f943c has been approved by glennw

@glennw
Copy link
Member

glennw commented Nov 9, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 7c03ad8 has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 7c03ad8 with merge 060a6e6...

bors-servo pushed a commit that referenced this pull request Nov 9, 2017
Register thread to WR's profiler and the external profiler.

The gecko profiler needs the thread to register itself to gecko profiler before collecting the performance data. In this patch, we have a new "thread listener" interface to do somethings when the thread is created and deleted. Then, we could put the registering function inside the listener interface.

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

💔 Test failed - status-travis

@glennw
Copy link
Member

glennw commented Nov 9, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit 7c03ad8 with merge f58ed65...

bors-servo pushed a commit that referenced this pull request Nov 10, 2017
Register thread to WR's profiler and the external profiler.

The gecko profiler needs the thread to register itself to gecko profiler before collecting the performance data. In this patch, we have a new "thread listener" interface to do somethings when the thread is created and deleted. Then, we could put the registering function inside the listener interface.

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

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing f58ed65 to master...

@bors-servo bors-servo merged commit 7c03ad8 into servo:master Nov 10, 2017
@glennw
Copy link
Member

glennw commented Nov 26, 2017

Wow, I had not extensively used the Gecko CPU profiler before. It's really excellent - and with this addition we get perfect callstacks for profiling in the WR backend thread. @mrobinson @kvark heads up - next time you need to profile some CPU code in WR, you should try this out! :)

Thanks @JerryShih and others who worked on the patch.

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