Skip to content

Commit

Permalink
Improve performance of time panel (rerun-io#8224)
Browse files Browse the repository at this point in the history
### Related

* More optimization depends likely on
rerun-io#8221

### What

Make the timepanel faster. Still plenty of room for improvement though.
Achieved by...
* store subscriber to keep track of cumulative chunks needed for a given
entity & timeline
    * plus some statistics. less important
* a bit faster `num_events_cumulative_per_unique_time_unsorted`
* improved `DensityGraph::add_range` (this one was surprisingly
significant)

### Testing

TL;DR: Minimized timepanel takes less than half the time it use. Other
cases are better as well, but more noisy.

----

All testing done on the 2h airtraffic dataset (pre-saved rrd) with all
panels hidden and views removed (unless noted otherwise) on my M1 Max.
Note that the timeline perf is very dependent on amount of screen real
estate - these tests were done maximized on the 14'' Mac's screen.
(Did some throw-away testing on other configs, but these are the ones
we're comparing here!)

This round of optimization focused mostly on the "idle" case of having
the time panel minimized. There are also some gains for the expanded
case, but it's less significant - as illustrated by the profiler
screenshots this is largely dominated
`num_events_cumulative_per_unique_time` which I hope we can solve with
chunk processors (and some untracked overhead I haven't looked into
yet).

**Before:**

Frame cpu time without profiler attached hovers around 4.2ms with some
outliers.
<img width="169" alt="image"
src="https://github.com/user-attachments/assets/0cfead2d-b485-45e8-b864-390cc8acd341">

Attaching the profiler doesn't tell us much since the profiler overhead
drowns out everything else:

![image](https://github.com/user-attachments/assets/db688dc3-c0bc-449a-a9d7-cce192cfec30)

Restarting without profiler and expanding the time panel and making it
as high as possible gives us about 12ms with frequent spikes beyond 13ms

<img width="1798" alt="image"
src="https://github.com/user-attachments/assets/5732a1ed-9911-4dbe-ae15-c52c9d0e4eeb">

Profiler overhead is ironically not _as_ significant. Averaging a few
frames tells us the time panel is at 11.5ms

![image](https://github.com/user-attachments/assets/6186da15-faa9-469c-8e80-b12184ae1689)


**After**

Frame cpu time without profiler attached hovers between 1.5ms and 2.8ms,
rather unsteady
<img width="124" alt="image"
src="https://github.com/user-attachments/assets/9d709888-0b48-4404-a570-417677175202">

Averaging a bunch of frames tells us that the data_density_graph takes
now 0.55ms (imho still pretty bad for that it is)

![image](https://github.com/user-attachments/assets/37cf9d75-7023-41d9-967c-7555b6fc0740)

Restarting without profiler and expanding the time panel and making it
as high as possible gives us around 11ms

<img width="1798" alt="Screenshot 2024-11-26 at 15 45 20"
src="https://github.com/user-attachments/assets/f4eab17f-db0e-4a21-86d9-5ac47560d7d0">

(important: this picture hasn't changed otherwise!)


The time panel now takes 9.4ms (that is definitely still very bad!),
profiler overhead is still significant but it's manageable:


![image](https://github.com/user-attachments/assets/0f4375e8-cc76-40c1-9f7f-049e5a4c4640)
Wumpf authored Nov 28, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 5ebfbd4 commit f1e48f0
Showing 12 changed files with 513 additions and 125 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
@@ -6270,13 +6270,16 @@ dependencies = [
"criterion",
"egui",
"itertools 0.13.0",
"nohash-hasher",
"once_cell",
"rand",
"re_chunk_store",
"re_context_menu",
"re_data_ui",
"re_entity_db",
"re_format",
"re_int_histogram",
"re_log",
"re_log_types",
"re_tracing",
"re_types",
49 changes: 27 additions & 22 deletions crates/store/re_chunk/src/chunk.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use std::{
collections::BTreeMap,
sync::atomic::{AtomicU64, Ordering},
};
use std::sync::atomic::{AtomicU64, Ordering};

use ahash::HashMap;
use arrow2::{
array::{
Array as Arrow2Array, ListArray as Arrow2ListArray, PrimitiveArray as Arrow2PrimitiveArray,
@@ -452,24 +450,31 @@ impl Chunk {

debug_assert!(!time_column.is_sorted());

self.components
.values()
.flat_map(move |list_array| {
izip!(
time_column.times(),
// Reminder: component columns are sparse, we must take a look at the validity bitmaps.
list_array.validity().map_or_else(
|| arrow2::Either::Left(std::iter::repeat(1).take(self.num_rows())),
|validity| arrow2::Either::Right(validity.iter().map(|b| b as u64)),
)
)
})
.fold(BTreeMap::default(), |mut acc, (time, is_valid)| {
*acc.entry(time).or_default() += is_valid;
acc
})
.into_iter()
.collect()
// NOTE: This is used on some very hot paths (time panel rendering).

let result_unordered =
self.components
.values()
.fold(HashMap::default(), |acc, list_array| {
if let Some(validity) = list_array.validity() {
time_column.times().zip(validity.iter()).fold(
acc,
|mut acc, (time, is_valid)| {
*acc.entry(time).or_default() += is_valid as u64;
acc
},
)
} else {
time_column.times().fold(acc, |mut acc, time| {
*acc.entry(time).or_default() += 1;
acc
})
}
});

let mut result = result_unordered.into_iter().collect_vec();
result.sort_by_key(|val| val.0);
result
}

/// The number of events in this chunk for the specified component.
29 changes: 22 additions & 7 deletions crates/store/re_chunk_store/src/events.rs
Original file line number Diff line number Diff line change
@@ -80,6 +80,23 @@ impl ChunkStoreDiffKind {
}
}

/// Reports which [`Chunk`]s were merged into a new [`Chunk`] during a compaction.
#[derive(Debug, Clone)]
pub struct ChunkCompactionReport {
/// The chunks that were merged into a new chunk.
pub srcs: BTreeMap<ChunkId, Arc<Chunk>>,

/// The new chunk that was created as the result of the compaction.
pub new_chunk: Arc<Chunk>,
}

impl PartialEq for ChunkCompactionReport {
#[inline]
fn eq(&self, rhs: &Self) -> bool {
self.srcs.keys().eq(rhs.srcs.keys()) && self.new_chunk.id() == rhs.new_chunk.id()
}
}

/// Describes an atomic change in the Rerun [`ChunkStore`]: a chunk has been added or deleted.
///
/// From a query model standpoint, the [`ChunkStore`] _always_ operates one chunk at a time:
@@ -120,14 +137,15 @@ pub struct ChunkStoreDiff {
// deallocated.
pub chunk: Arc<Chunk>,

/// Reports which [`Chunk`]s were merged into a new [`ChunkId`] (srcs, dst) during a compaction.
/// Reports which [`Chunk`]s were merged into a new [`Chunk`] during a compaction.
///
/// This is only specified if an addition to the store triggered a compaction.
/// When that happens, it is guaranteed that [`ChunkStoreDiff::chunk`] will be present in the
/// set of source chunks below, since it was compacted on arrival.
///
/// A corollary to that is that the destination [`ChunkId`] must have never been seen before.
pub compacted: Option<(BTreeMap<ChunkId, Arc<Chunk>>, ChunkId)>,
/// A corollary to that is that the destination [`Chunk`] must have never been seen before,
/// i.e. it's [`ChunkId`] must have never been seen before.
pub compacted: Option<ChunkCompactionReport>,
}

impl PartialEq for ChunkStoreDiff {
@@ -146,10 +164,7 @@ impl Eq for ChunkStoreDiff {}

impl ChunkStoreDiff {
#[inline]
pub fn addition(
chunk: Arc<Chunk>,
compacted: Option<(BTreeMap<ChunkId, Arc<Chunk>>, ChunkId)>,
) -> Self {
pub fn addition(chunk: Arc<Chunk>, compacted: Option<ChunkCompactionReport>) -> Self {
Self {
kind: ChunkStoreDiffKind::Addition,
chunk,
4 changes: 3 additions & 1 deletion crates/store/re_chunk_store/src/lib.rs
Original file line number Diff line number Diff line change
@@ -29,7 +29,9 @@ pub use self::dataframe::{
IndexRange, IndexValue, QueryExpression, SparseFillStrategy, TimeColumnDescriptor,
TimeColumnSelector, ViewContentsSelector,
};
pub use self::events::{ChunkStoreDiff, ChunkStoreDiffKind, ChunkStoreEvent};
pub use self::events::{
ChunkCompactionReport, ChunkStoreDiff, ChunkStoreDiffKind, ChunkStoreEvent,
};
pub use self::gc::{GarbageCollectionOptions, GarbageCollectionTarget};
pub use self::stats::{ChunkStoreChunkStats, ChunkStoreStats};
pub use self::store::{
3 changes: 2 additions & 1 deletion crates/store/re_chunk_store/src/query.rs
Original file line number Diff line number Diff line change
@@ -810,7 +810,8 @@ impl ChunkStore {
query: &RangeQuery,
temporal_chunk_ids_per_times: impl Iterator<Item = &'a ChunkIdSetPerTime>,
) -> Vec<Arc<Chunk>> {
re_tracing::profile_function!();
// Too small & frequent for profiling scopes.
//re_tracing::profile_function!();

temporal_chunk_ids_per_times
.map(|temporal_chunk_ids_per_time| {
6 changes: 4 additions & 2 deletions crates/store/re_chunk_store/src/writes.rs
Original file line number Diff line number Diff line change
@@ -322,9 +322,11 @@ impl ChunkStore {
.map(|diff| (diff.chunk.id(), diff.chunk)),
)
.collect();
let dst = chunk_or_compacted.id();

diff.compacted = Some((srcs, dst));
diff.compacted = Some(crate::ChunkCompactionReport {
srcs,
new_chunk: chunk_or_compacted.clone(),
});
}

(chunk_or_compacted, vec![diff])
28 changes: 20 additions & 8 deletions crates/store/re_query/src/cache.rs
Original file line number Diff line number Diff line change
@@ -8,7 +8,9 @@ use nohash_hasher::IntSet;
use parking_lot::RwLock;

use re_chunk::ChunkId;
use re_chunk_store::{ChunkStoreDiff, ChunkStoreEvent, ChunkStoreHandle, ChunkStoreSubscriber};
use re_chunk_store::{
ChunkCompactionReport, ChunkStoreDiff, ChunkStoreEvent, ChunkStoreHandle, ChunkStoreSubscriber,
};
use re_log_types::{EntityPath, ResolvedTimeRange, StoreId, TimeInt, Timeline};
use re_types_core::{components::ClearIsRecursive, Component as _, ComponentName};

@@ -315,11 +317,12 @@ impl ChunkStoreSubscriber for QueryCache {

compacted_events.insert(chunk.id());
// If a compaction was triggered, make sure to drop the original chunks too.
compacted_events.extend(
compacted
.iter()
.flat_map(|(compacted_chunks, _)| compacted_chunks.keys().copied()),
);
compacted_events.extend(compacted.iter().flat_map(
|ChunkCompactionReport {
srcs: compacted_chunks,
new_chunk: _,
}| compacted_chunks.keys().copied(),
));
}
}

@@ -336,7 +339,11 @@ impl ChunkStoreSubscriber for QueryCache {
let mut data_time_min = time_range.min();

// If a compaction was triggered, make sure to drop the original chunks too.
if let Some((compacted_chunks, _)) = compacted {
if let Some(ChunkCompactionReport {
srcs: compacted_chunks,
new_chunk: _,
}) = compacted
{
for chunk in compacted_chunks.values() {
let data_time_compacted = chunk
.time_range_per_component()
@@ -366,7 +373,12 @@ impl ChunkStoreSubscriber for QueryCache {
compacted_events.insert(chunk.id());
// If a compaction was triggered, make sure to drop the original chunks too.
compacted_events.extend(compacted.iter().flat_map(
|(compacted_chunks, _)| compacted_chunks.keys().copied(),
|ChunkCompactionReport {
srcs: compacted_chunks,
new_chunk: _,
}| {
compacted_chunks.keys().copied()
},
));
}
}
Original file line number Diff line number Diff line change
@@ -40,13 +40,13 @@ pub struct TransformComponentTracker {
}

impl TransformComponentTracker {
/// Accesses the spatial topology for a given store.
/// Accesses the transform component tracking data for a given store.
#[inline]
pub fn access<T>(store_id: &StoreId, f: impl FnOnce(&Self) -> T) -> Option<T> {
ChunkStore::with_subscriber_once(
TransformComponentTrackerStoreSubscriber::subscription_handle(),
move |susbcriber: &TransformComponentTrackerStoreSubscriber| {
susbcriber.per_store.get(store_id).map(f)
move |subscriber: &TransformComponentTrackerStoreSubscriber| {
subscriber.per_store.get(store_id).map(f)
},
)
.flatten()
3 changes: 3 additions & 0 deletions crates/viewer/re_time_panel/Cargo.toml
Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@ re_entity_db.workspace = true
re_format.workspace = true
re_int_histogram.workspace = true
re_log_types.workspace = true
re_log.workspace = true
re_tracing.workspace = true
re_types.workspace = true
re_ui.workspace = true
@@ -34,6 +35,8 @@ re_viewport_blueprint.workspace = true

egui.workspace = true
itertools.workspace = true
nohash-hasher.workspace = true
once_cell.workspace = true
serde.workspace = true
vec1.workspace = true

Loading

0 comments on commit f1e48f0

Please sign in to comment.