Skip to content

Commit

Permalink
Keep track of why request_discard was called
Browse files Browse the repository at this point in the history
  • Loading branch information
emilk committed Sep 19, 2024
1 parent 5cc35d2 commit 92a6dc2
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 36 deletions.
4 changes: 2 additions & 2 deletions crates/egui-winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,8 +823,8 @@ impl State {
ime,
#[cfg(feature = "accesskit")]
accesskit_update,
num_completed_passes: _, // `egui::Context::run` handles this
requested_discard: _, // `egui::Context::run` handles this
num_completed_passes: _, // `egui::Context::run` handles this
request_discard_reasons: _, // `egui::Context::run` handles this
} = platform_output;

self.set_cursor_icon(window, cursor_icon);
Expand Down
72 changes: 42 additions & 30 deletions crates/egui/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,12 @@ impl std::fmt::Debug for RepaintCause {
}
}

impl std::fmt::Display for RepaintCause {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}:{}", self.file, self.line)
}
}

impl RepaintCause {
/// Capture the file and line number of the call site.
#[allow(clippy::new_without_default)]
Expand All @@ -313,12 +319,6 @@ impl RepaintCause {
}
}

impl std::fmt::Display for RepaintCause {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}:{}", self.file, self.line)
}
}

/// Per-viewport state related to repaint scheduling.
struct ViewportRepaintInfo {
/// Monotonically increasing counter.
Expand Down Expand Up @@ -791,21 +791,21 @@ impl Context {
let viewport = ctx.viewport_for(viewport_id);
viewport.output.num_completed_passes =
std::mem::take(&mut output.platform_output.num_completed_passes);
output.platform_output.requested_discard = false;
output.platform_output.request_discard_reasons.clear();
});

self.begin_pass(new_input.take());
run_ui(self);
output.append(self.end_pass());
debug_assert!(0 < output.platform_output.num_completed_passes);

if !output.platform_output.requested_discard {
if !output.platform_output.requested_discard() {
break; // no need for another pass
}

if max_passes <= output.platform_output.num_completed_passes {
#[cfg(feature = "log")]
log::debug!("Ignoring call request_discard, because max_passes={max_passes}");
log::debug!("Ignoring call request_discard, because max_passes={max_passes}. Requested from {:?}", output.platform_output.request_discard_reasons);

break;
}
Expand Down Expand Up @@ -1641,8 +1641,12 @@ impl Context {
///
/// You should be very conservative with when you call [`Self::request_discard`],
/// as it will cause an extra ui pass, potentially leading to extra CPU use and frame judder.
pub fn request_discard(&self) {
self.output_mut(|o| o.requested_discard = true);
///
/// The given reason should be a human-readable string that explains why `request_discard`
/// was called. This will be shown in certain debug situations, to help you figure out
/// why a pass was discarded.
pub fn request_discard(&self, reason: impl Into<String>) {
self.output_mut(|o| o.request_discard_reasons.push(reason.into()));

#[cfg(feature = "log")]
log::trace!(
Expand All @@ -1664,7 +1668,7 @@ impl Context {
self.write(|ctx| {
let vp = ctx.viewport();
// NOTE: `num_passes` is incremented
vp.output.requested_discard
vp.output.requested_discard()
&& vp.output.num_completed_passes + 1 < ctx.memory.options.max_passes.get()
})
}
Expand Down Expand Up @@ -2197,12 +2201,16 @@ impl Context {
if 3 <= num_multipass_in_row {
// If you see this message, it means we've been paying the cost of multi-pass for multiple frames in a row.
// This is likely a bug. `request_discard` should only be called in rare situations, when some layout changes.
self.debug_painter().debug_text(
Pos2::ZERO,
Align2::LEFT_TOP,
Color32::RED,
format!("egui PERF WARNING: request_discard has been called {num_multipass_in_row} frames in a row"),
);

let mut warning = format!("egui PERF WARNING: request_discard has been called {num_multipass_in_row} frames in a row");
self.viewport(|vp| {
for reason in &vp.output.request_discard_reasons {
warning += &format!("\n {reason}");
}
});

self.debug_painter()
.debug_text(Pos2::ZERO, Align2::LEFT_TOP, Color32::RED, warning);
}
}
}
Expand Down Expand Up @@ -3734,28 +3742,32 @@ mod test {
let output = ctx.run(Default::default(), |ctx| {
num_calls += 1;
assert_eq!(ctx.output(|o| o.num_completed_passes), 0);
assert!(!ctx.output(|o| o.requested_discard));
assert!(!ctx.output(|o| o.requested_discard()));
assert!(!ctx.will_discard());
});
assert_eq!(num_calls, 1);
assert_eq!(output.platform_output.num_completed_passes, 1);
assert!(!output.platform_output.requested_discard);
assert!(!output.platform_output.requested_discard());
}

// A single call, with a denied request to discard:
{
let mut num_calls = 0;
let output = ctx.run(Default::default(), |ctx| {
num_calls += 1;
ctx.request_discard();
ctx.request_discard("test");
assert!(!ctx.will_discard(), "The request should have been denied");
});
assert_eq!(num_calls, 1);
assert_eq!(output.platform_output.num_completed_passes, 1);
assert!(
output.platform_output.requested_discard,
output.platform_output.requested_discard(),
"The request should be reported"
);
assert_eq!(
output.platform_output.request_discard_reasons,
vec!["test".to_owned()]
);
}
}

Expand All @@ -3769,13 +3781,13 @@ mod test {
let mut num_calls = 0;
let output = ctx.run(Default::default(), |ctx| {
assert_eq!(ctx.output(|o| o.num_completed_passes), 0);
assert!(!ctx.output(|o| o.requested_discard));
assert!(!ctx.output(|o| o.requested_discard()));
assert!(!ctx.will_discard());
num_calls += 1;
});
assert_eq!(num_calls, 1);
assert_eq!(output.platform_output.num_completed_passes, 1);
assert!(!output.platform_output.requested_discard);
assert!(!output.platform_output.requested_discard());
}

// Request discard once:
Expand All @@ -3786,7 +3798,7 @@ mod test {

assert!(!ctx.will_discard());
if num_calls == 0 {
ctx.request_discard();
ctx.request_discard("test");
assert!(ctx.will_discard());
}

Expand All @@ -3795,7 +3807,7 @@ mod test {
assert_eq!(num_calls, 2);
assert_eq!(output.platform_output.num_completed_passes, 2);
assert!(
!output.platform_output.requested_discard,
!output.platform_output.requested_discard(),
"The request should have been cleared when fulfilled"
);
}
Expand All @@ -3807,7 +3819,7 @@ mod test {
assert_eq!(ctx.output(|o| o.num_completed_passes), num_calls);

assert!(!ctx.will_discard());
ctx.request_discard();
ctx.request_discard("test");
if num_calls == 0 {
assert!(ctx.will_discard(), "First request granted");
} else {
Expand All @@ -3819,7 +3831,7 @@ mod test {
assert_eq!(num_calls, 2);
assert_eq!(output.platform_output.num_completed_passes, 2);
assert!(
output.platform_output.requested_discard,
output.platform_output.requested_discard(),
"The unfulfilled request should be reported"
);
}
Expand All @@ -3838,7 +3850,7 @@ mod test {

assert!(!ctx.will_discard());
if num_calls <= 2 {
ctx.request_discard();
ctx.request_discard("test");
assert!(ctx.will_discard());
}

Expand All @@ -3847,7 +3859,7 @@ mod test {
assert_eq!(num_calls, 4);
assert_eq!(output.platform_output.num_completed_passes, 4);
assert!(
!output.platform_output.requested_discard,
!output.platform_output.requested_discard(),
"The request should have been cleared when fulfilled"
);
}
Expand Down
16 changes: 13 additions & 3 deletions crates/egui/src/data/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,11 @@ pub struct PlatformOutput {
pub num_completed_passes: usize,

/// Was [`crate::Context::request_discard`] called during the latest pass?
pub requested_discard: bool,
///
/// If so, what was the reason(s) for it?
///
/// If empty, there was never any calls.
pub request_discard_reasons: Vec<String>,
}

impl PlatformOutput {
Expand Down Expand Up @@ -167,7 +171,7 @@ impl PlatformOutput {
#[cfg(feature = "accesskit")]
accesskit_update,
num_completed_passes,
requested_discard,
mut request_discard_reasons,
} = newer;

self.cursor_icon = cursor_icon;
Expand All @@ -181,7 +185,8 @@ impl PlatformOutput {
self.mutable_text_under_cursor = mutable_text_under_cursor;
self.ime = ime.or(self.ime);
self.num_completed_passes += num_completed_passes;
self.requested_discard |= requested_discard;
self.request_discard_reasons
.append(&mut request_discard_reasons);

#[cfg(feature = "accesskit")]
{
Expand All @@ -197,6 +202,11 @@ impl PlatformOutput {
self.cursor_icon = taken.cursor_icon; // everything else is ephemeral
taken
}

/// Was [`Context::request_discard`] called?
pub fn requested_discard(&self) -> bool {
!self.request_discard_reasons.is_empty()
}
}

/// What URL to open, and how.
Expand Down
2 changes: 1 addition & 1 deletion crates/egui/src/grid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ impl Grid {

if ui.is_visible() {
// Try to cover up the glitchy initial frame:
ui.ctx().request_discard();
ui.ctx().request_discard("new Grid");
}

// Hide the ui this frame, and make things as narrow as possible:
Expand Down

0 comments on commit 92a6dc2

Please sign in to comment.