oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Revert FPS counter (#17485)

Browse Source 
**UPDATE**: Response so far seems to be that this fixes the performance
issues on Intel MacBooks. So we're going to go ahead and merge it.

This reverts the FPS counter added in 11753914d (#16422) because in this
issue someone bisected recent performance regressions down to this
commit:

- https://github.com/zed-industries/zed/issues/16729

Another issue that's possibly related:

-
https://github.com/zed-industries/zed/issues/17305#issuecomment-2332316242

We're reverting this in a PR to create a bundle that people can try out.

Assets:

- Universal Binary:
https://github.com/zed-industries/zed/actions/runs/10735702994/artifacts/1900460781
- x86/Intel:
https://github.com/zed-industries/zed/actions/runs/10735702994/artifacts/1900461236
- Apple Silicon:
https://github.com/zed-industries/zed/actions/runs/10735702994/artifacts/1900460978


Release Notes:

- Removed the recently-added FPS counter since the changes it made to
the Metal renderer on macOS could lead to performance regressions on
Intel MacBooks.

Co-authored-by: Bennet <bennet@zed.dev>
This commit is contained in:
Thorsten Ball
2024-09-06 15:35:00 +02:00
committed by GitHub
co-authored by Bennet
parent 54dd40878f
commit 938c90fd3b
19 changed files with 60 additions and 524 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/gpui/src/platform/blade/blade_renderer.rs
+1 -13
View File
@@ -9,7 +9,6 @@ use crate::{
};
use bytemuck::{Pod, Zeroable};
use collections::HashMap;
use futures::channel::oneshot;
#[cfg(target_os = "macos")]
use media::core_video::CVMetalTextureCache;
#[cfg(target_os = "macos")]
@@ -543,12 +542,7 @@ impl BladeRenderer {
self.gpu.destroy_command_encoder(&mut self.command_encoder);
}
pub fn draw(
&mut self,
scene: &Scene,
// Required to compile on macOS, but not currently supported.
_on_complete: Option<oneshot::Sender<()>>,
) {
pub fn draw(&mut self, scene: &Scene) {
self.command_encoder.start();
self.atlas.before_frame(&mut self.command_encoder);
self.rasterize_paths(scene.paths());
@@ -777,10 +771,4 @@ impl BladeRenderer {
self.wait_for_gpu();
self.last_sync_point = Some(sync_point);
}
/// Required to compile on macOS, but not currently supported.
#[cfg_attr(any(target_os = "linux", target_os = "windows"), allow(dead_code))]
pub fn fps(&self) -> f32 {
0.0
}
}
crates/gpui/src/platform/fps.rs
-94
View File
@@ -1,94 +0,0 @@
use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
use std::sync::Arc;
const NANOS_PER_SEC: u64 = 1_000_000_000;
const WINDOW_SIZE: usize = 128;
/// Represents a rolling FPS (Frames Per Second) counter.
///
/// This struct provides a lock-free mechanism to measure and calculate FPS
/// continuously, updating with every frame. It uses atomic operations to
/// ensure thread-safety without the need for locks.
pub struct FpsCounter {
frame_times: [AtomicU64; WINDOW_SIZE],
head: AtomicUsize,
tail: AtomicUsize,
}
impl FpsCounter {
/// Creates a new `Fps` counter.
///
/// Returns an `Arc<Fps>` for safe sharing across threads.
pub fn new() -> Arc<Self> {
Arc::new(Self {
frame_times: std::array::from_fn(|_| AtomicU64::new(0)),
head: AtomicUsize::new(0),
tail: AtomicUsize::new(0),
})
}
/// Increments the FPS counter with a new frame timestamp.
///
/// This method updates the internal state to maintain a rolling window
/// of frame data for the last second. It uses atomic operations to
/// ensure thread-safety.
///
/// # Arguments
///
/// * `timestamp_ns` - The timestamp of the new frame in nanoseconds.
pub fn increment(&self, timestamp_ns: u64) {
let mut head = self.head.load(Ordering::Relaxed);
let mut tail = self.tail.load(Ordering::Relaxed);
// Add new timestamp
self.frame_times[head].store(timestamp_ns, Ordering::Relaxed);
// Increment head and wrap around to 0 if it reaches WINDOW_SIZE
head = (head + 1) % WINDOW_SIZE;
self.head.store(head, Ordering::Relaxed);
// Remove old timestamps (older than 1 second)
while tail != head {
let oldest = self.frame_times[tail].load(Ordering::Relaxed);
if timestamp_ns.wrapping_sub(oldest) <= NANOS_PER_SEC {
break;
}
// Increment tail and wrap around to 0 if it reaches WINDOW_SIZE
tail = (tail + 1) % WINDOW_SIZE;
self.tail.store(tail, Ordering::Relaxed);
}
}
/// Calculates and returns the current FPS.
///
/// This method computes the FPS based on the frames recorded in the last second.
/// It uses atomic loads to ensure thread-safety.
///
/// # Returns
///
/// The calculated FPS as a `f32`, or 0.0 if no frames have been recorded.
pub fn fps(&self) -> f32 {
let head = self.head.load(Ordering::Relaxed);
let tail = self.tail.load(Ordering::Relaxed);
if head == tail {
return 0.0;
}
let newest =
self.frame_times[head.wrapping_sub(1) & (WINDOW_SIZE - 1)].load(Ordering::Relaxed);
let oldest = self.frame_times[tail].load(Ordering::Relaxed);
let time_diff = newest.wrapping_sub(oldest) as f32;
if time_diff == 0.0 {
return 0.0;
}
let frame_count = if head > tail {
head - tail
} else {
WINDOW_SIZE - tail + head
};
(frame_count as f32 - 1.0) * NANOS_PER_SEC as f32 / time_diff
}
}
crates/gpui/src/platform/linux/wayland/window.rs
+4 -8
View File
@@ -6,7 +6,7 @@ use std::sync::Arc;
use blade_graphics as gpu;
use collections::HashMap;
use futures::channel::oneshot;
use futures::channel::oneshot::Receiver;
use raw_window_handle as rwh;
use wayland_backend::client::ObjectId;
@@ -831,7 +831,7 @@ impl PlatformWindow for WaylandWindow {
_msg: &str,
_detail: Option<&str>,
_answers: &[&str],
) -> Option<oneshot::Receiver<usize>> {
) -> Option<Receiver<usize>> {
None
}
@@ -938,9 +938,9 @@ impl PlatformWindow for WaylandWindow {
self.0.callbacks.borrow_mut().appearance_changed = Some(callback);
}
fn draw(&self, scene: &Scene, on_complete: Option<oneshot::Sender<()>>) {
fn draw(&self, scene: &Scene) {
let mut state = self.borrow_mut();
state.renderer.draw(scene, on_complete);
state.renderer.draw(scene);
}
fn completed_frame(&self) {
@@ -1018,10 +1018,6 @@ impl PlatformWindow for WaylandWindow {
fn gpu_specs(&self) -> Option<GPUSpecs> {
self.borrow().renderer.gpu_specs().into()
}
fn fps(&self) -> Option<f32> {
None
}
}
fn update_window(mut state: RefMut<WaylandWindowState>) {
crates/gpui/src/platform/linux/x11/window.rs
+4 -9
View File
@@ -1,3 +1,5 @@
use anyhow::Context;
use crate::{
platform::blade::{BladeRenderer, BladeSurfaceConfig},
px, size, AnyWindowHandle, Bounds, Decorations, DevicePixels, ForegroundExecutor, GPUSpecs,
@@ -7,9 +9,7 @@ use crate::{
X11ClientStatePtr,
};
use anyhow::Context;
use blade_graphics as gpu;
use futures::channel::oneshot;
use raw_window_handle as rwh;
use util::{maybe, ResultExt};
use x11rb::{
@@ -1210,10 +1210,9 @@ impl PlatformWindow for X11Window {
self.0.callbacks.borrow_mut().appearance_changed = Some(callback);
}
// TODO: on_complete not yet supported for X11 windows
fn draw(&self, scene: &Scene, on_complete: Option<oneshot::Sender<()>>) {
fn draw(&self, scene: &Scene) {
let mut inner = self.0.state.borrow_mut();
inner.renderer.draw(scene, on_complete);
inner.renderer.draw(scene);
}
fn sprite_atlas(&self) -> Arc<dyn PlatformAtlas> {
@@ -1406,8 +1405,4 @@ impl PlatformWindow for X11Window {
fn gpu_specs(&self) -> Option<GPUSpecs> {
self.0.state.borrow().renderer.gpu_specs().into()
}
fn fps(&self) -> Option<f32> {
None
}
}
crates/gpui/src/platform/mac/metal_renderer.rs
+9 -28
View File
@@ -1,7 +1,7 @@
use super::metal_atlas::MetalAtlas;
use crate::{
point, size, AtlasTextureId, AtlasTextureKind, AtlasTile, Bounds, ContentMask, DevicePixels,
FpsCounter, Hsla, MonochromeSprite, PaintSurface, Path, PathId, PathVertex, PolychromeSprite,
Hsla, MonochromeSprite, PaintSurface, Path, PathId, PathVertex, PolychromeSprite,
PrimitiveBatch, Quad, ScaledPixels, Scene, Shadow, Size, Surface, Underline,
};
use anyhow::{anyhow, Result};
@@ -14,7 +14,6 @@ use cocoa::{
use collections::HashMap;
use core_foundation::base::TCFType;
use foreign_types::ForeignType;
use futures::channel::oneshot;
use media::core_video::CVMetalTextureCache;
use metal::{CAMetalLayer, CommandQueue, MTLPixelFormat, MTLResourceOptions, NSRange};
use objc::{self, msg_send, sel, sel_impl};
@@ -106,7 +105,6 @@ pub(crate) struct MetalRenderer {
instance_buffer_pool: Arc<Mutex<InstanceBufferPool>>,
sprite_atlas: Arc<MetalAtlas>,
core_video_texture_cache: CVMetalTextureCache,
fps_counter: Arc<FpsCounter>,
}
impl MetalRenderer {
@@ -252,7 +250,6 @@ impl MetalRenderer {
instance_buffer_pool,
sprite_atlas,
core_video_texture_cache,
fps_counter: FpsCounter::new(),
}
}
@@ -295,8 +292,7 @@ impl MetalRenderer {
// nothing to do
}
pub fn draw(&mut self, scene: &Scene, on_complete: Option<oneshot::Sender<()>>) {
let on_complete = Arc::new(Mutex::new(on_complete));
pub fn draw(&mut self, scene: &Scene) {
let layer = self.layer.clone();
let viewport_size = layer.drawable_size();
let viewport_size: Size<DevicePixels> = size(
@@ -323,24 +319,13 @@ impl MetalRenderer {
Ok(command_buffer) => {
let instance_buffer_pool = self.instance_buffer_pool.clone();
let instance_buffer = Cell::new(Some(instance_buffer));
let device = self.device.clone();
let fps_counter = self.fps_counter.clone();
let completed_handler =
ConcreteBlock::new(move |_: &metal::CommandBufferRef| {
let mut cpu_timestamp = 0;
let mut gpu_timestamp = 0;
device.sample_timestamps(&mut cpu_timestamp, &mut gpu_timestamp);
fps_counter.increment(gpu_timestamp);
if let Some(on_complete) = on_complete.lock().take() {
on_complete.send(()).ok();
}
if let Some(instance_buffer) = instance_buffer.take() {
instance_buffer_pool.lock().release(instance_buffer);
}
});
let completed_handler = completed_handler.copy();
command_buffer.add_completed_handler(&completed_handler);
let block = ConcreteBlock::new(move |_| {
if let Some(instance_buffer) = instance_buffer.take() {
instance_buffer_pool.lock().release(instance_buffer);
}
});
let block = block.copy();
command_buffer.add_completed_handler(&block);
if self.presents_with_transaction {
command_buffer.commit();
@@ -1132,10 +1117,6 @@ impl MetalRenderer {
}
true
}
pub fn fps(&self) -> f32 {
self.fps_counter.fps()
}
}
fn build_pipeline_state(
crates/gpui/src/platform/mac/window.rs
+32 -36
View File
@@ -783,14 +783,14 @@ impl PlatformWindow for MacWindow {
self.0.as_ref().lock().bounds()
}
fn is_maximized(&self) -> bool {
self.0.as_ref().lock().is_maximized()
}
fn window_bounds(&self) -> WindowBounds {
self.0.as_ref().lock().window_bounds()
}
fn is_maximized(&self) -> bool {
self.0.as_ref().lock().is_maximized()
}
fn content_size(&self) -> Size<Pixels> {
self.0.as_ref().lock().content_size()
}
@@ -974,6 +974,8 @@ impl PlatformWindow for MacWindow {
}
}
fn set_app_id(&mut self, _app_id: &str) {}
fn set_background_appearance(&self, background_appearance: WindowBackgroundAppearance) {
let mut this = self.0.as_ref().lock();
this.renderer
@@ -1004,6 +1006,30 @@ impl PlatformWindow for MacWindow {
}
}
fn set_edited(&mut self, edited: bool) {
unsafe {
let window = self.0.lock().native_window;
msg_send![window, setDocumentEdited: edited as BOOL]
}
// Changing the document edited state resets the traffic light position,
// so we have to move it again.
self.0.lock().move_traffic_light();
}
fn show_character_palette(&self) {
let this = self.0.lock();
let window = this.native_window;
this.executor
.spawn(async move {
unsafe {
let app = NSApplication::sharedApplication(nil);
let _: () = msg_send![app, orderFrontCharacterPalette: window];
}
})
.detach();
}
fn minimize(&self) {
let window = self.0.lock().native_window;
unsafe {
@@ -1080,41 +1106,15 @@ impl PlatformWindow for MacWindow {
self.0.lock().appearance_changed_callback = Some(callback);
}
fn draw(&self, scene: &crate::Scene, on_complete: Option<oneshot::Sender<()>>) {
fn draw(&self, scene: &crate::Scene) {
let mut this = self.0.lock();
this.renderer.draw(scene, on_complete);
this.renderer.draw(scene);
}
fn sprite_atlas(&self) -> Arc<dyn PlatformAtlas> {
self.0.lock().renderer.sprite_atlas().clone()
}
fn set_edited(&mut self, edited: bool) {
unsafe {
let window = self.0.lock().native_window;
msg_send![window, setDocumentEdited: edited as BOOL]
}
// Changing the document edited state resets the traffic light position,
// so we have to move it again.
self.0.lock().move_traffic_light();
}
fn show_character_palette(&self) {
let this = self.0.lock();
let window = this.native_window;
this.executor
.spawn(async move {
unsafe {
let app = NSApplication::sharedApplication(nil);
let _: () = msg_send![app, orderFrontCharacterPalette: window];
}
})
.detach();
}
fn set_app_id(&mut self, _app_id: &str) {}
fn gpu_specs(&self) -> Option<crate::GPUSpecs> {
None
}
@@ -1125,10 +1125,6 @@ impl PlatformWindow for MacWindow {
let _: () = msg_send![input_context, invalidateCharacterCoordinates];
}
}
fn fps(&self) -> Option<f32> {
Some(self.0.lock().renderer.fps())
}
}
impl rwh::HasWindowHandle for MacWindow {
crates/gpui/src/platform/test/window.rs
+1 -10
View File
@@ -251,12 +251,7 @@ impl PlatformWindow for TestWindow {
fn on_appearance_changed(&self, _callback: Box<dyn FnMut()>) {}
fn draw(
&self,
_scene: &crate::Scene,
_on_complete: Option<futures::channel::oneshot::Sender<()>>,
) {
}
fn draw(&self, _scene: &crate::Scene) {}
fn sprite_atlas(&self) -> sync::Arc<dyn crate::PlatformAtlas> {
self.0.lock().sprite_atlas.clone()
@@ -284,10 +279,6 @@ impl PlatformWindow for TestWindow {
fn gpu_specs(&self) -> Option<GPUSpecs> {
None
}
fn fps(&self) -> Option<f32> {
None
}
}
pub(crate) struct TestAtlasState {
crates/gpui/src/platform/windows/window.rs
+2 -6
View File
@@ -660,8 +660,8 @@ impl PlatformWindow for WindowsWindow {
self.0.state.borrow_mut().callbacks.appearance_changed = Some(callback);
}
fn draw(&self, scene: &Scene, on_complete: Option<oneshot::Sender<()>>) {
self.0.state.borrow_mut().renderer.draw(scene, on_complete)
fn draw(&self, scene: &Scene) {
self.0.state.borrow_mut().renderer.draw(scene)
}
fn sprite_atlas(&self) -> Arc<dyn PlatformAtlas> {
@@ -679,10 +679,6 @@ impl PlatformWindow for WindowsWindow {
fn update_ime_position(&self, _bounds: Bounds<Pixels>) {
// todo(windows)
}
fn fps(&self) -> Option<f32> {
None
}
}
#[implement(IDropTarget)]