Skip to content

Commit

Permalink
Reducing memory leaks in the built in status bar
Browse files Browse the repository at this point in the history 
This addresses several places where the interactions with xlib were
leaking memory and/or having performance issues. There is still
something leaking though as I can consistently cause the memory usage of
my personal setup to increase by rapidly switching between screens and
workspaces.

Interestingly, running something like the layout-render example with a
frame time of 200ms for multiple minutes shows consistent memory usage
without any leaks so there is something in particular about the status
bar itself which is causing this it seems.
  • Loading branch information
@sminez
sminez committed Jun 18, 2024
1 parent 36f7a1a commit 4edc789
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 77 deletions.
10 changes: 6 additions & 4 deletions crates/penrose_ui/examples/layout-render.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ use penrose_ui::layout_viewer::LayoutViewer;
const BLACK: u32 = 0x252535ff; // #252535
const WHITE: u32 = 0xdcd7baff; // #dcd7ba
const BLUE: u32 = 0x658594ff; // #658594
pub const RED: u32 = 0xc34043ff; // #C34043
const R: Rect = Rect::new(0, 0, 640, 480);
const FRAME_MS: u64 = 400;
const FRAME_MS: u64 = 200;
const GPX: u32 = 5;

fn main() -> anyhow::Result<()> {
Expand All @@ -24,9 +25,10 @@ fn main() -> anyhow::Result<()> {
Grid::boxed(),
];

let mut v = LayoutViewer::new(R, BLACK, BLUE, WHITE)?;
let mut v = LayoutViewer::new(R, BLACK, BLUE, WHITE, RED)?;
let s = stack!(1, 2, 3, 4, 5, 6).map(Into::into);
v.showcase_layouts(s, layouts, GPX, FRAME_MS)?;

Ok(())
loop {
v.showcase_layouts(s.clone(), &layouts, GPX, FRAME_MS)?;
}
}
39 changes: 19 additions & 20 deletions crates/penrose_ui/src/bar/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,20 @@ impl<X: XConn> Widgets<X> {
.any(|ps| ps.ws.iter().any(|w| w.require_draw())),
}
}

fn update_schedules(&mut self) -> Vec<UpdateSchedule> {
match self {
Self::Shared(ps) => ps
.ws
.iter_mut()
.filter_map(|w| w.update_schedule())
.collect(),
Self::PerScreen(pss) => pss
.iter_mut()
.flat_map(|ps| ps.ws.iter_mut().filter_map(|w| w.update_schedule()))
.collect(),
}
}
}

/// A simple text based status bar that renders a user defined array of [`Widget`]s.
Expand Down Expand Up @@ -160,18 +174,7 @@ impl<X: XConn> StatusBar<X> {
where
X: 'static,
{
let schedules: Vec<UpdateSchedule> = match &mut self.widgets {
Widgets::Shared(ps) => ps
.ws
.iter_mut()
.filter_map(|w| w.update_schedule())
.collect(),
Widgets::PerScreen(pss) => pss
.iter_mut()
.flat_map(|ps| ps.ws.iter_mut().filter_map(|w| w.update_schedule()))
.collect(),
};

let schedules = self.widgets.update_schedules();
if !schedules.is_empty() {
run_update_schedules(schedules);
}
Expand Down Expand Up @@ -228,7 +231,7 @@ impl<X: XConn> StatusBar<X> {
/// Re-render all widgets in this status bar for a single screen.
/// Will panic if `i` is out of bounds
fn redraw_screen(&mut self, i: usize) -> Result<()> {
let (id, w) = self.screens[i];
let (id, w_screen) = self.screens[i];
let screen_has_focus = self.active_screen == i;
let ps = self.widgets.for_screen_mut(i);

Expand All @@ -248,8 +251,8 @@ impl<X: XConn> StatusBar<X> {
let total = extents.iter().map(|(w, _)| w).sum::<u32>();
let n_greedy = greedy_indices.len();

if total < w && n_greedy > 0 {
let per_greedy = (w - total) / n_greedy as u32;
if total < w_screen && n_greedy > 0 {
let per_greedy = (w_screen - total) / n_greedy as u32;
for i in greedy_indices.iter() {
let (w, h) = extents[*i];
extents[*i] = (w + per_greedy, h);
Expand All @@ -260,7 +263,6 @@ impl<X: XConn> StatusBar<X> {
for (wd, (w, _)) in ps.ws.iter_mut().zip(extents) {
wd.draw(&mut ctx, self.active_screen, screen_has_focus, w, ps.h)?;
x += w;
ctx.flush();
ctx.set_x_offset(x as i32);
}

Expand All @@ -281,9 +283,6 @@ impl<X: XConn> StatusBar<X> {
fn redraw_if_needed(&mut self) -> Result<()> {
if self.widgets.require_draw(self.screens.len()) {
self.redraw()?;
for (id, _) in self.screens.iter() {
self.draw.flush(*id)?;
}
}

Ok(())
Expand Down Expand Up @@ -348,7 +347,7 @@ pub fn event_hook<X: XConn + 'static>(

if matches!(event, RandrNotify) || matches!(event, ConfigureNotify(e) if e.is_root) {
info!("screens have changed: recreating status bars");
let screens: Vec<_> = bar.screens.drain(0..).collect();
let screens: Vec<_> = bar.screens.drain(..).collect();

for (id, _) in screens {
info!(%id, "removing previous status bar");
Expand Down
13 changes: 8 additions & 5 deletions crates/penrose_ui/src/core/fontset.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use fontconfig_sys::{
FcPatternDuplicate,
};
use std::{
alloc::{alloc, handle_alloc_error, Layout},
alloc::{alloc, dealloc, handle_alloc_error, Layout},
collections::HashMap,
ffi::CString,
};
Expand Down Expand Up @@ -115,8 +115,10 @@ impl Drop for Fontset {
fn drop(&mut self) {
// SAFETY: the Display we have a pointer to is freed by the parent draw
unsafe {
FcPatternDestroy(self.primary.pattern as _);
XftFontClose(self.dpy, self.primary.xfont);
for f in self.fallback.drain(..) {
FcPatternDestroy(f.pattern as _);
XftFontClose(self.dpy, f.xfont);
}
}
Expand Down Expand Up @@ -215,7 +217,7 @@ impl Font {
);

let x_off = (*ext).xOff as u32;
std::alloc::dealloc(ext as *mut u8, layout);
dealloc(ext as *mut u8, layout);

Ok((x_off, self.h))
}
Expand Down Expand Up @@ -255,15 +257,16 @@ impl Font {
let res = ptr as *mut FcResult;

// Passing the pointer from fontconfig_sys to x11 here
let font_match = XftFontMatch(dpy, SCREEN, pat as *const _, res);
let fc_pattern = XftFontMatch(dpy, SCREEN, pat as *const _, res);

dealloc(res as *mut u8, layout);
FcCharSetDestroy(charset);
FcPatternDestroy(pat);

if font_match.is_null() {
if fc_pattern.is_null() {
Err(Error::NoFallbackFontForChar(c))
} else {
Ok(font_match as *mut _)
Ok(fc_pattern as *mut _)
}
}
}
Expand Down
100 changes: 55 additions & 45 deletions crates/penrose_ui/src/core/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use penrose::{
use std::{
alloc::{alloc, dealloc, handle_alloc_error, Layout},
cmp::max,
collections::HashMap,
collections::{hash_map::Entry, HashMap},
ffi::CString,
};
use tracing::{debug, info};
Expand Down Expand Up @@ -59,6 +59,16 @@ struct Surface {
drawable: Drawable,
gc: GC,
r: Rect,
id: u64,
}

impl Surface {
/// SAFETY: dpy must be non-null
pub(crate) unsafe fn flush(&self, dpy: *mut Display) {
let Rect { w, h, .. } = self.r;
XCopyArea(dpy, self.drawable, self.id, self.gc, 0, 0, w, h, 0, 0);
XSync(dpy, False);
}
}

/// A minimal back end for rendering simple text based UIs.
Expand Down Expand Up @@ -128,6 +138,8 @@ pub struct Draw {
active_font: String,
}

unsafe impl Send for Draw {}

Check warning on line 141 in crates/penrose_ui/src/core/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

unsafe impl missing a safety comment 

warning: unsafe impl missing a safety comment
   --> crates/penrose_ui/src/core/mod.rs:141:1
    |
141 | unsafe impl Send for Draw {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider adding a safety comment on the preceding line
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
note: the lint level is defined here
   --> crates/penrose_ui/src/lib.rs:25:5
    |
25  |     clippy::undocumented_unsafe_blocks,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Check warning on line 141 in crates/penrose_ui/src/core/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

unsafe impl missing a safety comment 

warning: unsafe impl missing a safety comment
   --> crates/penrose_ui/src/core/mod.rs:141:1
    |
141 | unsafe impl Send for Draw {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider adding a safety comment on the preceding line
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
note: the lint level is defined here
   --> crates/penrose_ui/src/lib.rs:25:5
    |
25  |     clippy::undocumented_unsafe_blocks,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

impl Drop for Draw {
fn drop(&mut self) {
// SAFETY: all pointers being freed are known to be non-null
Expand Down Expand Up @@ -197,17 +209,25 @@ impl Draw {
(drawable, gc)
};

self.surfaces.insert(id, Surface { r, gc, drawable });
self.surfaces.insert(
id,
Surface {
id: *id as u64,
r,
gc,
drawable,
},
);

Ok(id)
}

/// Destroy the specified window along with any surface and graphics context state held
/// within this draw.
pub fn destroy_window_and_surface(&mut self, id: Xid) -> Result<()> {
self.conn.destroy_window(id)?;
if let Some(s) = self.surfaces.remove(&id) {
// SAFETY: the pointerse being freed are known to be non-null
self.conn.destroy_window(id)?;
// SAFETY: the pointers being freed are known to be non-null
unsafe {
XFreePixmap(self.dpy, s.drawable);
XFreeGC(self.dpy, s.gc);
Expand All @@ -219,20 +239,19 @@ impl Draw {

pub(crate) fn add_font(&mut self, font: &str, point_size: u8) -> Result<()> {
let k = font_key(font, point_size);
let fs = Fontset::try_new(self.dpy, &k)?;
self.fss.insert(k, fs);
if let Entry::Vacant(e) = self.fss.entry(k) {
let fs = Fontset::try_new(self.dpy, e.key())?;
e.insert(fs);
}

Ok(())
}

/// Set the font being used for rendering text and clear the existing cache of fallback fonts
/// for characters that are not supported by the primary font.
pub fn set_font(&mut self, font: &str, point_size: u8) -> Result<()> {
let k = font_key(font, point_size);
if !self.fss.contains_key(&k) {
self.add_font(font, point_size)?;
}
self.active_font = k;
self.add_font(font, point_size)?;
self.active_font = font_key(font, point_size);

Ok(())
}
Expand All @@ -248,7 +267,6 @@ impl Draw {
.ok_or(Error::UnintialisedSurface { id })?;

Ok(Context {
id: *id as u64,
dx: 0,
dy: 0,
dpy: self.dpy,
Expand All @@ -265,19 +283,12 @@ impl Draw {
/// Flush any pending requests to the X server and map the specifed window to the screen.
pub fn flush(&self, id: Xid) -> Result<()> {
if let Some(s) = self.surfaces.get(&id) {
let Rect { x, y, w, h } = s.r;
let (x, y) = (x as i32, y as i32);

// SAFETY: the pointers for self.dpy, s.drawable, s.gc are known to be non-null
unsafe {
XCopyArea(self.dpy, s.drawable, *id as u64, s.gc, x, y, w, h, x, y);
XSync(self.dpy, False);
}
// SAFETY: self.dpy is non-null
unsafe { s.flush(self.dpy) };
self.conn.map(id)?;
self.conn.flush();
};

self.conn.map(id)?;
self.conn.flush();

Ok(())
}
}
Expand All @@ -296,7 +307,6 @@ impl Draw {
/// [0]: crate::StatusBar
#[derive(Debug)]
pub struct Context<'a> {
id: u64,
dx: i32,
dy: i32,
dpy: *mut Display,
Expand All @@ -318,10 +328,10 @@ impl<'a> Context<'a> {
self.dy += dy;
}

/// Set future drawing operations to apply from the origin.
pub fn reset_offset(&mut self) {
self.dx = 0;
self.dy = 0;
/// Set future drawing operations to apply from a specified point.
pub fn set_offset(&mut self, x: i32, y: i32) {
self.dx = x;
self.dy = y;
}

/// Set an absolute x offset for future drawing operations.
Expand All @@ -334,12 +344,22 @@ impl<'a> Context<'a> {
self.dy = y;
}

/// Set future drawing operations to apply from the origin.
pub fn reset_offset(&mut self) {
self.dx = 0;
self.dy = 0;
}

fn get_or_try_init_xcolor(&mut self, c: Color) -> Result<*mut XftColor> {
Ok(self
.colors
.entry(c)
.or_insert(XColor::try_new(self.dpy, &c)?)
.0)
if let Some(xc) = self.colors.get(&c) {
return Ok(xc.0);
}

let xc = XColor::try_new(self.dpy, &c)?;
let ptr = xc.0;
self.colors.insert(c, xc);

Ok(ptr)
}

/// Render a rectangular border using the supplied color.
Expand Down Expand Up @@ -500,18 +520,8 @@ impl<'a> Context<'a> {
/// This method does not need to be called explicitly if the flush method for
/// the parent [Draw] is being called as well.
pub fn flush(&self) {
let Surface {
r: Rect { w, h, .. },
gc,
drawable,
} = *self.s;

// SAFETY:
// - the pointers for self.dpy, drawable and gc are known to be non-null
unsafe {
XCopyArea(self.dpy, drawable, self.id, gc, 0, 0, w, h, 0, 0);
XSync(self.dpy, False);
}
// SAFETY: self.dpy is non-null
unsafe { self.s.flush(self.dpy) }
}
}

Expand Down
Loading

0 comments on commit 4edc789

Please sign in to comment.