Skip to content

Commit

Permalink
Merge packed and unpacked into single type
Browse files Browse the repository at this point in the history 
This commit consolidates the packed and unpacked data from frame_data
into a single enum. The goal of this change is to minimize the number of
locks required when retrieving information about a frame.

Previously, the process of reading the packing information for stats
required four separate locks:
 1. One to check 'has_unpacked'
 2. Another to verify 'has_unpacked' within 'unpacked_size'
 3. One to retrieve 'unpacked_size'
 4. And finally, one to retrieve 'packed_size'

With this optimization, the lock count is reduced to a single one,
achieved through invoking the 'packing_info' function.
  • Loading branch information
@andreiltd
andreiltd committed Jun 26, 2023
1 parent 2f8f16f commit c497844
Show file tree
Hide file tree
Showing 3 changed files with 164 additions and 88 deletions.
210 changes: 143 additions & 67 deletions puffin/src/frame_data.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub type ThreadStreams = BTreeMap<ThreadInfo, Arc<StreamInfo>>;

/// Meta-information about a frame.
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[derive(Clone, Debug)]
#[derive(Clone, Copy, Debug)]
pub struct FrameMeta {
/// What frame this is (counting from 0 at application startup).
pub frame_index: FrameIndex,
Expand Down Expand Up @@ -130,6 +130,13 @@ impl FrameData {
pub fn bytes_of_ram_used(&self) -> usize {
self.unpacked_frame.meta.num_bytes
}

pub fn packing_info(&self) -> PackingInfo {
PackingInfo {
unpacked_size: Some(self.unpacked_frame.meta.num_bytes),
packed_size: None,
}
}
pub fn has_packed(&self) -> bool {
false
}
Expand Down Expand Up @@ -278,14 +285,106 @@ impl PackedStreams {
#[cfg(feature = "packing")]
pub struct FrameData {
meta: FrameMeta,
/// * [`None`] if still compressed.
/// * `Some(Err(…))` if there was a problem during unpacking.
/// * `Some(Ok(…))` if unpacked.
unpacked_frame: RwLock<Option<anyhow::Result<Arc<UnpackedFrameData>>>>,
inner: RwLock<Inner>,
}

#[derive(Clone, Copy, Debug)]
pub struct PackingInfo {
/// Number of bytes used when unpacked, if has unpacked.
pub unpacked_size: Option<usize>,
/// Number of bytes used by the packed data, if has packed.
pub packed_size: Option<usize>,
}

#[cfg(feature = "packing")]
enum Inner {
/// Unpacked data.
Unpacked(Arc<UnpackedFrameData>),

/// [`UnpackedFrameData::thread_streams`], compressed.
/// [`None`] if not yet compressed.
packed_streams: RwLock<Option<PackedStreams>>,
Packed(PackedStreams),

/// Both compressed and uncompressed.
Both(Arc<UnpackedFrameData>, PackedStreams),
}

#[cfg(feature = "packing")]
impl Inner {
fn unpacked_size(&self) -> Option<usize> {
match self {
Inner::Packed(_) => None,
Inner::Unpacked(unpacked) | Inner::Both(unpacked, _) => Some(unpacked.meta.num_bytes),
}
}

fn unpacked(&self) -> Option<Arc<UnpackedFrameData>> {
match self {
Inner::Packed(_) => None,
Inner::Unpacked(unpacked) | Inner::Both(unpacked, _) => Some(unpacked.clone()),
}
}

fn unpack(&mut self, unpacked: Arc<UnpackedFrameData>) {
let temp = std::mem::replace(
self,
Inner::Packed(PackedStreams::new(CompressionKind::Uncompressed, vec![])),
);

if let Inner::Packed(packed) = temp {
// Transform only if we don't have unpacked already
*self = Inner::Both(unpacked, packed);
} else {
// Restore the original value if it was not Inner::Packed
*self = temp;
}
}

fn packed_size(&self) -> Option<usize> {
match self {
Inner::Unpacked(_) => None,
Inner::Packed(packed) | Inner::Both(_, packed) => Some(packed.num_bytes()),
}
}

fn packed(&self) -> Option<&PackedStreams> {
match self {
Inner::Unpacked(_) => None,
Inner::Packed(packed) | Inner::Both(_, packed) => Some(packed),
}
}

fn pack_and_remove(&mut self) {
if let Inner::Unpacked(ref unpacked) | Inner::Both(ref unpacked, _) = *self {
let packed = PackedStreams::pack(&unpacked.thread_streams);
*self = Self::Packed(packed);
}
}

fn pack_and_keep(&mut self) {
if let Inner::Unpacked(ref unpacked) = *self {
let packed = PackedStreams::pack(&unpacked.thread_streams);
*self = Self::Packed(packed);
}
}

fn bytes_of_ram_used(&self) -> usize {
self.unpacked_size().unwrap_or(0) + self.packed_size().unwrap_or(0)
}

fn has_packed(&self) -> bool {
matches!(self, Inner::Packed(_) | Inner::Both(..))
}

fn has_unpacked(&self) -> bool {
matches!(self, Inner::Unpacked(_) | Inner::Both(..))
}

fn packing_info(&self) -> PackingInfo {
PackingInfo {
unpacked_size: self.unpacked_size(),
packed_size: self.packed_size(),
}
}
}

#[cfg(feature = "packing")]
Expand All @@ -302,9 +401,8 @@ impl FrameData {

fn from_unpacked(unpacked_frame: Arc<UnpackedFrameData>) -> Self {
Self {
meta: unpacked_frame.meta.clone(),
unpacked_frame: RwLock::new(Some(Ok(unpacked_frame))),
packed_streams: RwLock::new(None),
meta: unpacked_frame.meta,
inner: RwLock::new(Inner::Unpacked(unpacked_frame)),
}
}

Expand All @@ -315,31 +413,37 @@ impl FrameData {

/// Number of bytes used by the packed data, if packed.
pub fn packed_size(&self) -> Option<usize> {
self.packed_streams.read().as_ref().map(|c| c.num_bytes())
self.inner.read().packed_size()
}

/// Number of bytes used when unpacked, if known.
pub fn unpacked_size(&self) -> Option<usize> {
if self.has_unpacked() {
Some(self.meta.num_bytes)
} else {
None
}
self.inner.read().unpacked_size()
}

/// bytes currently used by the unpacked and packed data.
pub fn bytes_of_ram_used(&self) -> usize {
self.unpacked_size().unwrap_or(0) + self.packed_size().unwrap_or(0)
self.inner.read().bytes_of_ram_used()
}

/// Do we have a packed version stored internally?
pub fn has_packed(&self) -> bool {
self.packed_streams.read().is_some()
self.inner.read().has_packed()
}

/// Do we have a unpacked version stored internally?
pub fn has_unpacked(&self) -> bool {
self.unpacked_frame.read().is_some()
self.inner.read().has_unpacked()
}

/// Provides an overview of the frame's packing status.
///
/// The function retrieves both the sizes of the unpacked and packed frames, as well as whether
/// packed/unpacked versions of the frame exist internally. The goal of this function is to
/// minimize the number of lock accesses by consolidating information retrieval into a single
/// operation.
pub fn packing_info(&self) -> PackingInfo {
self.inner.read().packing_info()
}

/// Return the unpacked data.
Expand All @@ -348,60 +452,34 @@ impl FrameData {
///
/// Returns `Err` if failing to decode the packed data.
pub fn unpacked(&self) -> anyhow::Result<Arc<UnpackedFrameData>> {
fn unpack_frame_data(
meta: FrameMeta,
packed: &PackedStreams,
) -> anyhow::Result<UnpackedFrameData> {
Ok(UnpackedFrameData {
meta,
thread_streams: packed.unpack()?,
})
}
let unpacked = {
let inner_guard = self.inner.read();
let Inner::Packed(ref packed) = *inner_guard else {
// Safe to unwrap, variant has to contain unpacked if NOT `Packed`
return Ok(self.inner.read().unpacked().unwrap());
};

let has_unpacked = self.unpacked_frame.read().is_some();
if !has_unpacked {
crate::profile_scope!("unpack_puffin_frame");
let packed_lock = self.packed_streams.read();
let packed = packed_lock
.as_ref()
.expect("FrameData is neither packed or unpacked");

let frame_data_result = unpack_frame_data(self.meta.clone(), packed);
let frame_data_result = frame_data_result.map(Arc::new);
*self.unpacked_frame.write() = Some(frame_data_result);
}

match self.unpacked_frame.read().as_ref().unwrap() {
Ok(frame) => Ok(frame.clone()),
Err(err) => Err(anyhow::format_err!("{}", err)), // can't clone `anyhow::Error`
}
Arc::new(UnpackedFrameData {
meta: self.meta,
thread_streams: packed.unpack()?,
})
};

self.inner.write().unpack(unpacked.clone());
Ok(unpacked)
}

/// Make the [`FrameData`] use up less memory.
/// Idempotent.
pub fn pack(&self) {
self.create_packed();
*self.unpacked_frame.write() = None;
self.inner.write().pack_and_remove();
}

/// Create a packed storage without freeing the unpacked storage.
fn create_packed(&self) {
let has_packed = self.packed_streams.read().is_some();
if !has_packed {
// crate::profile_scope!("pack_puffin_frame"); // we get called from `GlobalProfiler::new_frame`, so avoid recursiveness!
let unpacked_frame = self
.unpacked_frame
.read()
.as_ref()
.expect("We should have an unpacked frame if we don't have a packed one")
.as_ref()
.expect("The unpacked frame should be error free, since it doesn't come from packed source")
.clone();

let packed = PackedStreams::pack(&unpacked_frame.thread_streams);

*self.packed_streams.write() = Some(packed);
}
self.inner.write().pack_and_keep();
}

/// Writes one [`FrameData`] into a stream, prefixed by its length ([`u32`] le).
Expand All @@ -417,8 +495,8 @@ impl FrameData {
write.write_all(&meta_serialized)?;

self.create_packed();
let packed_streams_lock = self.packed_streams.read();
let packed_streams = packed_streams_lock.as_ref().unwrap(); // We just called create_packed
let packed_streams_lock = self.inner.read();
let packed_streams = packed_streams_lock.packed().unwrap(); // We just called create_packed

write.write_all(&(packed_streams.num_bytes() as u32).to_le_bytes())?;
write.write_u8(packed_streams.compression_kind as u8)?;
Expand Down Expand Up @@ -534,8 +612,7 @@ impl FrameData {

Ok(Some(Self {
meta,
unpacked_frame: RwLock::new(None),
packed_streams: RwLock::new(Some(packed_streams)),
inner: RwLock::new(Inner::Packed(packed_streams)),
}))
} else if &header == b"PFD3" {
// Added 2023-05-13: CompressionKind field
Expand Down Expand Up @@ -564,8 +641,7 @@ impl FrameData {

Ok(Some(Self {
meta,
unpacked_frame: RwLock::new(None),
packed_streams: RwLock::new(Some(packed_streams)),
inner: RwLock::new(Inner::Packed(packed_streams)),
}))
} else {
anyhow::bail!("Failed to decode: this data is newer than this reader. Please update your puffin version!");
Expand Down
38 changes: 19 additions & 19 deletions puffin/src/profile_view.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl FrameView {
self.add_recent_frame(&new_frame);
}

pub fn add_slow_frame(&mut self, new_frame: &Arc<FrameData>) {
fn add_slow_frame(&mut self, new_frame: &Arc<FrameData>) {
assert_eq!(self.slowest_by_duration.len(), self.slowest_by_index.len());

self.slowest_by_duration
Expand All @@ -108,7 +108,7 @@ impl FrameView {
}
}

pub fn add_recent_frame(&mut self, new_frame: &Arc<FrameData>) {
fn add_recent_frame(&mut self, new_frame: &Arc<FrameData>) {
self.recent.push_back(OrderedByIndex(new_frame.clone()));

while self.recent.len() > self.max_recent {
Expand Down Expand Up @@ -194,8 +194,8 @@ impl FrameView {
/// Retrieve statistics for added frames. This operation is efficient and suitable when
/// frames have not been manipulated outside of `ProfileView`, such as being unpacked. For
/// comprehensive statistics, refer to [`Self::stats_full()`]
pub fn stats(&self) -> &FrameStats {
&self.stats
pub fn stats(&self) -> FrameStats {
self.stats
}

/// Retrieve detailed statistics by performing a full computation on all the added frames.
Expand Down Expand Up @@ -358,8 +358,16 @@ impl GlobalFrameView {

// ----------------------------------------------------------------------------

fn stats_entry(frame: &FrameData) -> (usize, usize) {
let info = frame.packing_info();
(
info.packed_size.unwrap_or(0) + info.unpacked_size.unwrap_or(0),
info.unpacked_size.is_some() as usize,
)
}

/// Collect statistics for maintained frames
#[derive(Clone, Debug, Default)]
#[derive(Clone, Copy, Debug, Default)]
pub struct FrameStats {
unique_frames: usize,
total_ram_used: usize,
Expand All @@ -378,26 +386,18 @@ impl FrameStats {
}

fn add(&mut self, frame: &FrameData) {
self.total_ram_used = self
.total_ram_used
.saturating_add(frame.bytes_of_ram_used());

self.unpacked_frames = self
.unpacked_frames
.saturating_add(frame.has_unpacked() as usize);
let (total, unpacked) = stats_entry(frame);

self.total_ram_used = self.total_ram_used.saturating_add(total);
self.unpacked_frames = self.unpacked_frames.saturating_add(unpacked);
self.unique_frames = self.unique_frames.saturating_add(1);
}

fn remove(&mut self, frame: &FrameData) {
self.total_ram_used = self
.total_ram_used
.saturating_sub(frame.bytes_of_ram_used());

self.unpacked_frames = self
.unpacked_frames
.saturating_sub(frame.has_unpacked() as usize);
let (total, unpacked) = stats_entry(frame);

self.total_ram_used = self.total_ram_used.saturating_sub(total);
self.unpacked_frames = self.unpacked_frames.saturating_sub(unpacked);
self.unique_frames = self.unique_frames.saturating_sub(1);
}

Expand Down
4 changes: 2 additions & 2 deletions puffin_egui/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,12 +388,12 @@ impl ProfilerUi {
self.paused.as_ref().map_or_else(
|| {
let mut frames = AvailableFrames::latest(frame_view);
frames.stats = frame_view.stats().clone();
frames.stats = frame_view.stats();
frames
},
|paused| {
let mut frames = paused.frames.clone();
frames.stats = frame_view.stats_full();
frames.stats = FrameStats::from_frames(paused.frames.uniq.iter().map(Arc::as_ref));
frames
},
)
Expand Down

0 comments on commit c497844

Please sign in to comment.