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

Cache System Tracing Spans #9390

Merged
merged 7 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 24 additions & 29 deletions crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use bevy_tasks::{ComputeTaskPool, Scope, TaskPool, ThreadExecutor};
use bevy_utils::default;
use bevy_utils::syncunsafecell::SyncUnsafeCell;
#[cfg(feature = "trace")]
use bevy_utils::tracing::{info_span, Instrument};
use bevy_utils::tracing::{info_span, Instrument, Span};
use std::panic::AssertUnwindSafe;

use async_channel::{Receiver, Sender};
Expand Down Expand Up @@ -62,6 +62,9 @@ struct SystemTaskMetadata {
is_send: bool,
/// Is `true` if the system is exclusive.
is_exclusive: bool,
/// Cached tracing span for system task
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Cached tracing span for system task
/// Tracing span for system task, cached for performance.

#[cfg(feature = "trace")]
system_task_span: Span,
}

/// The result of running a system that is sent across a channel.
Expand Down Expand Up @@ -153,6 +156,11 @@ impl SystemExecutor for MultiThreadedExecutor {
dependents: schedule.system_dependents[index].clone(),
is_send: schedule.systems[index].is_send(),
is_exclusive: schedule.systems[index].is_exclusive(),
#[cfg(feature = "trace")]
system_task_span: info_span!(
"system_task",
name = &*schedule.systems[index].name()
),
});
}

Expand Down Expand Up @@ -486,26 +494,16 @@ impl MultiThreadedExecutor {
) {
// SAFETY: this system is not running, no other reference exists
let system = unsafe { &mut *systems[system_index].get() };

#[cfg(feature = "trace")]
let task_span = info_span!("system_task", name = &*system.name());
#[cfg(feature = "trace")]
let system_span = info_span!("system", name = &*system.name());

let sender = self.sender.clone();
let panic_payload = self.panic_payload.clone();
let task = async move {
#[cfg(feature = "trace")]
let system_guard = system_span.enter();
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
// SAFETY:
// - The caller ensures that we have permission to
// access the world data used by the system.
// - `update_archetype_component_access` has been called.
unsafe { system.run_unsafe((), world) };
}));
#[cfg(feature = "trace")]
drop(system_guard);
// tell the executor that the system finished
sender
.try_send(SystemResult {
Expand All @@ -524,7 +522,11 @@ impl MultiThreadedExecutor {
};

#[cfg(feature = "trace")]
let task = task.instrument(task_span);
let task = task.instrument(
self.system_task_metadata[system_index]
.system_task_span
.clone(),
);
Comment on lines +525 to +529
Copy link
Contributor

Choose a reason for hiding this comment

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

This clone makes me wonder, do we really need to instrument the Future as well? Normally this is done because Futures can suspend execution during .await points and that would mess up spans, but here we are never awaiting and moreover we're already measuring the run_unsafe internally, which should account for most of the execution time.

Copy link
Member

Choose a reason for hiding this comment

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

The instrumentation here does help measure the additional scheduler overhead, which is thankfully very low right now. However, I do see how this both adds even more profiling overhead and how it might not be all that useful to the typical user. Not sure how to best approach toggling this on or off though.

Copy link
Contributor

Choose a reason for hiding this comment

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

My worry is that the time needed for the instrumentation itself here may be in the same order of magniture as the one it is measuring (in addition to the one already measured inside the run_unsafe). That is, it is measuring how much time is needed to run the catch_unwind, doing a non-blocking send on a channel and checking if the catch_unwind returned an error, all of which are pretty fast.


let system_meta = &self.system_task_metadata[system_index];
self.active_access
Expand All @@ -550,23 +552,14 @@ impl MultiThreadedExecutor {
// SAFETY: this system is not running, no other reference exists
let system = unsafe { &mut *systems[system_index].get() };

#[cfg(feature = "trace")]
let task_span = info_span!("system_task", name = &*system.name());
#[cfg(feature = "trace")]
let system_span = info_span!("system", name = &*system.name());

let sender = self.sender.clone();
let panic_payload = self.panic_payload.clone();
if is_apply_deferred(system) {
// TODO: avoid allocation
let unapplied_systems = self.unapplied_systems.clone();
self.unapplied_systems.clear();
let task = async move {
#[cfg(feature = "trace")]
let system_guard = system_span.enter();
let res = apply_deferred(&unapplied_systems, systems, world);
#[cfg(feature = "trace")]
drop(system_guard);
// tell the executor that the system finished
sender
.try_send(SystemResult {
Expand All @@ -582,17 +575,17 @@ impl MultiThreadedExecutor {
};

#[cfg(feature = "trace")]
let task = task.instrument(task_span);
let task = task.instrument(
self.system_task_metadata[system_index]
.system_task_span
.clone(),
);
scope.spawn_on_scope(task);
} else {
let task = async move {
#[cfg(feature = "trace")]
let system_guard = system_span.enter();
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
system.run((), world);
}));
#[cfg(feature = "trace")]
drop(system_guard);
// tell the executor that the system finished
sender
.try_send(SystemResult {
Expand All @@ -612,7 +605,11 @@ impl MultiThreadedExecutor {
};

#[cfg(feature = "trace")]
let task = task.instrument(task_span);
let task = task.instrument(
self.system_task_metadata[system_index]
.system_task_span
.clone(),
);
scope.spawn_on_scope(task);
}

Expand Down Expand Up @@ -718,8 +715,6 @@ unsafe fn evaluate_and_fold_conditions(
conditions
.iter_mut()
.map(|condition| {
#[cfg(feature = "trace")]
let _condition_span = info_span!("condition", name = &*condition.name()).entered();
// SAFETY: The caller ensures that `world` has permission to
// access any data required by the condition.
unsafe { condition.run_unsafe((), world) }
Expand Down
10 changes: 1 addition & 9 deletions crates/bevy_ecs/src/schedule/executor/simple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,9 @@ impl SystemExecutor for SimpleExecutor {
}

let system = &mut schedule.systems[system_index];
#[cfg(feature = "trace")]
let system_span = info_span!("system", name = &*name).entered();
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
system.run((), world);
}));
#[cfg(feature = "trace")]
system_span.exit();
if let Err(payload) = res {
eprintln!("Encountered a panic in system `{}`!", &*system.name());
std::panic::resume_unwind(payload);
Expand Down Expand Up @@ -113,10 +109,6 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
#[allow(clippy::unnecessary_fold)]
conditions
.iter_mut()
.map(|condition| {
#[cfg(feature = "trace")]
let _condition_span = info_span!("condition", name = &*condition.name()).entered();
condition.run((), world)
})
.map(|condition| condition.run((), world))
.fold(true, |acc, res| acc && res)
}
14 changes: 1 addition & 13 deletions crates/bevy_ecs/src/schedule/executor/single_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,19 +86,11 @@ impl SystemExecutor for SingleThreadedExecutor {

let system = &mut schedule.systems[system_index];
if is_apply_deferred(system) {
#[cfg(feature = "trace")]
let system_span = info_span!("system", name = &*name).entered();
self.apply_deferred(schedule, world);
#[cfg(feature = "trace")]
system_span.exit();
} else {
#[cfg(feature = "trace")]
let system_span = info_span!("system", name = &*name).entered();
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
system.run((), world);
}));
#[cfg(feature = "trace")]
system_span.exit();
if let Err(payload) = res {
eprintln!("Encountered a panic in system `{}`!", &*system.name());
std::panic::resume_unwind(payload);
Expand Down Expand Up @@ -143,10 +135,6 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
#[allow(clippy::unnecessary_fold)]
conditions
.iter_mut()
.map(|condition| {
#[cfg(feature = "trace")]
let _condition_span = info_span!("condition", name = &*condition.name()).entered();
condition.run((), world)
})
.map(|condition| condition.run((), world))
.fold(true, |acc, res| acc && res)
}
4 changes: 1 addition & 3 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,7 @@ impl SystemBuffer for CommandQueue {
#[inline]
fn apply(&mut self, _system_meta: &SystemMeta, world: &mut World) {
#[cfg(feature = "trace")]
let _system_span =
bevy_utils::tracing::info_span!("system_commands", name = _system_meta.name())
.entered();
let _span_guard = _system_meta.commands_span.enter();
self.apply(world);
}
}
Expand Down
4 changes: 1 addition & 3 deletions crates/bevy_ecs/src/system/commands/parallel_scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ impl SystemBuffer for ParallelCommandQueue {
#[inline]
fn apply(&mut self, _system_meta: &SystemMeta, world: &mut World) {
#[cfg(feature = "trace")]
let _system_span =
bevy_utils::tracing::info_span!("system_commands", name = _system_meta.name())
.entered();
let _system_span = _system_meta.commands_span.enter();
for cq in &mut self.thread_local_storage {
cq.get_mut().apply(world);
}
Expand Down
3 changes: 3 additions & 0 deletions crates/bevy_ecs/src/system/exclusive_function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ where
}

fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out {
#[cfg(feature = "trace")]
let _span_guard = self.system_meta.system_span.enter();

let saved_last_tick = world.last_change_tick;
world.last_change_tick = self.system_meta.last_run;

Expand Down
17 changes: 16 additions & 1 deletion crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ use crate::{
use bevy_utils::all_tuples;
use std::{any::TypeId, borrow::Cow, marker::PhantomData};

#[cfg(feature = "trace")]
use bevy_utils::tracing::{info_span, Span};

use super::{In, IntoSystem, ReadOnlySystem};

/// The metadata of a [`System`].
Expand All @@ -22,16 +25,25 @@ pub struct SystemMeta {
// SystemParams from overriding each other
is_send: bool,
pub(crate) last_run: Tick,
#[cfg(feature = "trace")]
pub(crate) system_span: Span,
#[cfg(feature = "trace")]
pub(crate) commands_span: Span,
}

impl SystemMeta {
pub(crate) fn new<T>() -> Self {
let name = std::any::type_name::<T>();
Self {
name: std::any::type_name::<T>().into(),
name: name.into(),
archetype_component_access: Access::default(),
component_access_set: FilteredAccessSet::default(),
is_send: true,
last_run: Tick::new(0),
#[cfg(feature = "trace")]
system_span: info_span!("system", name = name),
#[cfg(feature = "trace")]
commands_span: info_span!("system_commands", name = name),
}
}

Expand Down Expand Up @@ -444,6 +456,9 @@ where

#[inline]
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out {
#[cfg(feature = "trace")]
let _span_guard = self.system_meta.system_span.enter();

let change_tick = world.increment_change_tick();

// SAFETY:
Expand Down
Loading