From e32496a5925d231f2a8593c829f29320c78133e8 Mon Sep 17 00:00:00 2001 From: Shyamalan Kannan Date: Tue, 21 Apr 2026 16:31:40 -0700 Subject: [PATCH] browser: Reduce spurious rendering and lock contention Four targeted improvements to the browser crate's rendering pipeline: 1. Gate FrameReady on suspension state (tab.rs): Skip emitting TabEvent::FrameReady for suspended tabs. CEF already stops rendering hidden browsers, but this prevents any in-flight frames from propagating cx.notify() after suspension. 2. Gate FrameReady cx.notify() on surface visibility (browser_view.rs): Only trigger a re-render when BrowserSurfaceState is Visible. In-flight FrameReady events queued just before a mode switch no longer cause spurious layout work on the hidden browser view. 3. Split RenderState Mutex (render_handler.rs, tab.rs, client.rs, life_span_handler.rs): Extract current_frame into its own Arc>> separate from the geometry fields (width, height, scale_factor). CEF's view_rect and screen_info callbacks no longer share a lock with the 60fps frame write path, eliminating cross-contention between geometry queries and frame writes. 4. Compute pinned_count once per render pass (tab_strip.rs, browser_view.rs): Both render_tab_strip and render_sidebar previously ran an independent O(n) tab scan to count pinned tabs. The count is now computed once in the render() method via a pinned_tab_count() helper and passed as a parameter to both render methods. --- crates/browser/src/browser_view.rs | 47 +++++++++++--------- crates/browser/src/browser_view/tab_strip.rs | 20 ++++++--- crates/browser/src/client.rs | 27 +++++++++-- crates/browser/src/life_span_handler.rs | 11 ++++- crates/browser/src/render_handler.rs | 22 ++++++--- crates/browser/src/tab.rs | 32 ++++++++++--- 6 files changed, 117 insertions(+), 42 deletions(-) diff --git a/crates/browser/src/browser_view.rs b/crates/browser/src/browser_view.rs index 9205e50160b499..24e97ce5c39df5 100644 --- a/crates/browser/src/browser_view.rs +++ b/crates/browser/src/browser_view.rs @@ -848,7 +848,9 @@ impl BrowserView { match event { #[cfg(target_os = "macos")] TabEvent::FrameReady => { - cx.notify(); + if self.surface_state == BrowserSurfaceState::Visible { + cx.notify(); + } } TabEvent::NavigateToUrl(url) => { let url = url.clone(); @@ -1493,26 +1495,29 @@ impl Render for BrowserView { .into_any_element(); #[cfg(not(target_os = "macos"))] - let element = match self.tab_bar_mode { - TabBarMode::Horizontal => element - .flex_col() - .child(div().mt(px(-1.)).child(self.render_tab_strip(cx))) - .child(self.bookmark_bar.clone()) - .child(self.render_browser_content(window, cx)) - .into_any_element(), - TabBarMode::Sidebar => element - .flex_row() - .child(self.render_sidebar(cx)) - .child( - div() - .flex_1() - .flex() - .flex_col() - .overflow_hidden() - .child(self.bookmark_bar.clone()) - .child(self.render_browser_content(window, cx)), - ) - .into_any_element(), + let element = { + let pinned_count = self.pinned_tab_count(cx); + match self.tab_bar_mode { + TabBarMode::Horizontal => element + .flex_col() + .child(div().mt(px(-1.)).child(self.render_tab_strip(pinned_count, cx))) + .child(self.bookmark_bar.clone()) + .child(self.render_browser_content(window, cx)) + .into_any_element(), + TabBarMode::Sidebar => element + .flex_row() + .child(self.render_sidebar(pinned_count, cx)) + .child( + div() + .flex_1() + .flex() + .flex_col() + .overflow_hidden() + .child(self.bookmark_bar.clone()) + .child(self.render_browser_content(window, cx)), + ) + .into_any_element(), + } }; div() diff --git a/crates/browser/src/browser_view/tab_strip.rs b/crates/browser/src/browser_view/tab_strip.rs index 5944062cef098a..e20c7ae3af64b6 100644 --- a/crates/browser/src/browser_view/tab_strip.rs +++ b/crates/browser/src/browser_view/tab_strip.rs @@ -242,13 +242,20 @@ impl Render for BrowserSidebarPanel { impl BrowserView { #[cfg(not(target_os = "macos"))] - pub(super) fn render_tab_strip(&mut self, cx: &mut Context) -> impl IntoElement { + pub(super) fn pinned_tab_count(&self, cx: &mut gpui::Context) -> usize { + self.tabs.iter().filter(|t| t.read(cx).is_pinned()).count() + } + + #[cfg(not(target_os = "macos"))] + pub(super) fn render_tab_strip( + &mut self, + pinned_count: usize, + cx: &mut Context, + ) -> impl IntoElement { let theme = cx.theme(); let active_index = self.active_tab_index; let view = cx.entity().downgrade(); - let pinned_count = self.tabs.iter().filter(|t| t.read(cx).is_pinned()).count(); - h_flex() .w_full() .h(px(34.)) @@ -613,12 +620,15 @@ impl BrowserView { } #[cfg(not(target_os = "macos"))] - pub(super) fn render_sidebar(&mut self, cx: &mut Context) -> impl IntoElement { + pub(super) fn render_sidebar( + &mut self, + pinned_count: usize, + cx: &mut Context, + ) -> impl IntoElement { let theme = cx.theme(); let active_index = self.active_tab_index; let view = cx.entity().downgrade(); - let pinned_count = self.tabs.iter().filter(|t| t.read(cx).is_pinned()).count(); let grid_cols = pinned_count.min(3); v_flex() diff --git a/crates/browser/src/client.rs b/crates/browser/src/client.rs index 1efdfcb00202a3..5084132340941d 100644 --- a/crates/browser/src/client.rs +++ b/crates/browser/src/client.rs @@ -24,6 +24,8 @@ use crate::permission_handler::{OsrPermissionHandler, PermissionHandlerBuilder}; use crate::render_handler::{OsrRenderHandler, RenderHandlerBuilder, RenderState}; use crate::request_handler::{OsrRequestHandler, RequestHandlerBuilder}; use crate::text_input::extract_text_input_state_from_message; +#[cfg(target_os = "macos")] +use core_video::pixel_buffer::CVPixelBuffer; use parking_lot::Mutex; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; @@ -189,16 +191,29 @@ wrap_client! { } impl ClientBuilder { - pub fn build(render_state: Arc>, event_sender: EventSender) -> cef::Client { - Self::build_inner(render_state, event_sender, KeyboardHandlerBuilder::build()) + pub fn build( + render_state: Arc>, + #[cfg(target_os = "macos")] current_frame: Arc>>, + event_sender: EventSender, + ) -> cef::Client { + Self::build_inner( + render_state, + #[cfg(target_os = "macos")] + current_frame, + event_sender, + KeyboardHandlerBuilder::build(), + ) } pub fn build_for_popup( render_state: Arc>, + #[cfg(target_os = "macos")] current_frame: Arc>>, event_sender: EventSender, ) -> cef::Client { Self::build_inner( render_state, + #[cfg(target_os = "macos")] + current_frame, event_sender, PopupKeyboardHandlerBuilder::build(), ) @@ -206,10 +221,16 @@ impl ClientBuilder { fn build_inner( render_state: Arc>, + #[cfg(target_os = "macos")] current_frame: Arc>>, event_sender: EventSender, keyboard_handler: cef::KeyboardHandler, ) -> cef::Client { - let render_handler = OsrRenderHandler::new(render_state, event_sender.clone()); + let render_handler = OsrRenderHandler::new( + render_state, + #[cfg(target_os = "macos")] + current_frame, + event_sender.clone(), + ); let load_handler = OsrLoadHandler::new(event_sender.clone()); let display_handler = OsrDisplayHandler::new(event_sender.clone()); let life_span_handler = OsrLifeSpanHandler::new(event_sender.clone()); diff --git a/crates/browser/src/life_span_handler.rs b/crates/browser/src/life_span_handler.rs index 4d8170a49d2a0c..3ec36a4611dfe5 100644 --- a/crates/browser/src/life_span_handler.rs +++ b/crates/browser/src/life_span_handler.rs @@ -11,6 +11,8 @@ use cef::{ Browser, ImplLifeSpanHandler, LifeSpanHandler, WrapLifeSpanHandler, rc::Rc as _, wrap_life_span_handler, }; +#[cfg(target_os = "macos")] +use core_video::pixel_buffer::CVPixelBuffer; use parking_lot::Mutex; use std::sync::Arc; @@ -26,8 +28,15 @@ impl OsrLifeSpanHandler { fn popup_client() -> cef::Client { let render_state = Arc::new(Mutex::new(RenderState::default())); + #[cfg(target_os = "macos")] + let current_frame: Arc>> = Arc::new(Mutex::new(None)); let (popup_sender, _popup_receiver) = crate::events::event_channel(); - ClientBuilder::build_for_popup(render_state, popup_sender) + ClientBuilder::build_for_popup( + render_state, + #[cfg(target_os = "macos")] + current_frame, + popup_sender, + ) } } diff --git a/crates/browser/src/render_handler.rs b/crates/browser/src/render_handler.rs index d677ba8a3e0d94..abb4fcf1d09122 100644 --- a/crates/browser/src/render_handler.rs +++ b/crates/browser/src/render_handler.rs @@ -20,12 +20,12 @@ use io_surface::IOSurface; use parking_lot::Mutex; use std::sync::Arc; +/// Viewport geometry shared between the GPUI resize path and CEF's view_rect/screen_info callbacks. +/// Kept separate from the frame buffer so geometry reads never contend with 60fps frame writes. pub struct RenderState { pub width: u32, pub height: u32, pub scale_factor: f32, - #[cfg(target_os = "macos")] - pub current_frame: Option, } impl Default for RenderState { @@ -34,8 +34,6 @@ impl Default for RenderState { width: 800, height: 600, scale_factor: 1.0, - #[cfg(target_os = "macos")] - current_frame: None, } } } @@ -44,14 +42,24 @@ impl Default for RenderState { pub struct OsrRenderHandler { state: Arc>, #[cfg(target_os = "macos")] + current_frame: Arc>>, + #[cfg(target_os = "macos")] sender: EventSender, } impl OsrRenderHandler { - pub fn new(state: Arc>, sender: EventSender) -> Self { + pub fn new( + state: Arc>, + #[cfg(target_os = "macos")] current_frame: Arc>>, + sender: EventSender, + ) -> Self { #[cfg(target_os = "macos")] { - Self { state, sender } + Self { + state, + current_frame, + sender, + } } #[cfg(not(target_os = "macos"))] @@ -153,7 +161,7 @@ wrap_render_handler! { } }; - self.handler.state.lock().current_frame = Some(pixel_buffer); + *self.handler.current_frame.lock() = Some(pixel_buffer); let _ = self.handler.sender.send(BrowserEvent::FrameReady); } diff --git a/crates/browser/src/tab.rs b/crates/browser/src/tab.rs index df1d1a0da0e531..0e2b8f2d000651 100644 --- a/crates/browser/src/tab.rs +++ b/crates/browser/src/tab.rs @@ -105,6 +105,8 @@ pub struct BrowserTab { browser_id: Option, client: cef::Client, render_state: Arc>, + #[cfg(target_os = "macos")] + current_frame: Arc>>, event_receiver: EventReceiver, url: String, title: String, @@ -127,13 +129,22 @@ impl EventEmitter for BrowserTab {} impl BrowserTab { pub fn new(_cx: &mut Context) -> Self { let render_state = Arc::new(Mutex::new(RenderState::default())); + #[cfg(target_os = "macos")] + let current_frame: Arc>> = Arc::new(Mutex::new(None)); let (sender, receiver) = events::event_channel(); - let client = ClientBuilder::build(render_state.clone(), sender); + let client = ClientBuilder::build( + render_state.clone(), + #[cfg(target_os = "macos")] + current_frame.clone(), + sender, + ); Self { browser_id: None, client, render_state, + #[cfg(target_os = "macos")] + current_frame, event_receiver: receiver, url: String::from("glass://newtab"), title: String::from("New Tab"), @@ -160,13 +171,22 @@ impl BrowserTab { _cx: &mut Context, ) -> Self { let render_state = Arc::new(Mutex::new(RenderState::default())); + #[cfg(target_os = "macos")] + let current_frame: Arc>> = Arc::new(Mutex::new(None)); let (sender, receiver) = events::event_channel(); - let client = ClientBuilder::build(render_state.clone(), sender); + let client = ClientBuilder::build( + render_state.clone(), + #[cfg(target_os = "macos")] + current_frame.clone(), + sender, + ); Self { browser_id: None, client, render_state, + #[cfg(target_os = "macos")] + current_frame, event_receiver: receiver, url, title, @@ -225,7 +245,9 @@ impl BrowserTab { } #[cfg(target_os = "macos")] BrowserEvent::FrameReady => { - cx.emit(TabEvent::FrameReady); + if !is_suspended { + cx.emit(TabEvent::FrameReady); + } } BrowserEvent::BrowserCreated => {} BrowserEvent::LoadError { url, error_text } => { @@ -672,7 +694,7 @@ impl BrowserTab { #[cfg(target_os = "macos")] pub fn current_frame(&self) -> Option { - self.render_state.lock().current_frame.clone() + self.current_frame.lock().clone() } pub fn url(&self) -> &str { @@ -768,7 +790,7 @@ impl BrowserTab { } #[cfg(target_os = "macos")] { - self.render_state.lock().current_frame = None; + *self.current_frame.lock() = None; } }