-
Notifications
You must be signed in to change notification settings - Fork 332
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
DataStore
changelog 3: introduce StoreView
s
#4205
Conversation
092b7e0
to
d0fa7a8
Compare
c29f8a2
to
65dfa8e
Compare
d0fa7a8
to
009dcfa
Compare
65dfa8e
to
838d2c5
Compare
009dcfa
to
70571cd
Compare
838d2c5
to
79e1d31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:chefkiss: 👌
// TODO(cmc): Not sure why I need the extra Box here, RwLock should be `?Sized`. | ||
type SharedStoreView = RwLock<Box<dyn StoreView>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looked it up and it is indeed https://docs.rs/lock_api/latest/lock_api/struct.RwLock.html
So it gotta be something else that's wrong here if you say we can't put the StoreView directly 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, looked it up too, and ended up with the same emotion: 🤔
// TODO(cmc): Not sure why I need the extra Box here, RwLock should be `?Sized`. | ||
type SharedStoreView = RwLock<Box<dyn StoreView>>; | ||
|
||
/// A [`StoreView`] subscribes to atomic changes in one or more [`DataStore`]s through [`StoreEvent`]s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-nit: This is your field not mine, but I thought the more technical correct term is transactional changes
instead of atomic
? Since the changes happen in one a single transaction but not necessarily atomic in the sense of an atomic operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now one always subscribes to all DataStore
, whether that's a good idea in the long run or not I don't know yet, but it seems the comment is incorrect as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-nit: This is your field not mine, but I thought the more technical correct term is
transactional changes
instead ofatomic
? Since the changes happen in one a single transaction but not necessarily atomic in the sense of an atomic operation
Atomicity is a property of the query model, transactions are just one way of achieving it (case in point: we don't have transactions!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now one always subscribes to all
DataStore
, whether that's a good idea in the long run or not I don't know yet, but it seems the comment is incorrect as-is?
That's pretty much what I meant, I can replace with "all".
/// Arbitrary name for the view. | ||
/// | ||
/// Does not need to be unique. | ||
fn name(&self) -> String; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the intended use? I'd guess it's a display name used for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this isn't just always a &'static str
, but would get annoying eventually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the intended use? I'd guess it's a display name used for debugging?
Yeah, just a good habit for things to be named IME. Especially plugin-y stuff like this that ends up living all over the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this isn't just always a
&'static str
, but would get annoying eventually
I would much prefer not forcing people to just through hoops if they need to format!()
something (and we can turn to a Cow
later if the string is a performance concern (highly unlikely)).
/// Passes a reference to the downcasted view to the given callback. | ||
/// | ||
/// Returns `None` if the view doesn't exist or downcasting failed. | ||
pub fn with_view<V: StoreView, T, F: FnMut(&V) -> T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand why it's called with_
it's not like I get a DataStore with some handle. More like view
or get_view
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is because it doesn't return the view, but rather takes a closure that is executed with the view as an argument. As in "With this view, run this function". That said, I also find with_
confusing here. Maybe for_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly based this on the thread_local
API from the stdlib: https://doc.rust-lang.org/std/thread/struct.LocalKey.html#examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep it as is for now as it feels somewhat consistent with std at least... definitely feel free to change it later if you come up with anything better.
…4202) The upcoming `StoreView` works in global scope: by registering a view you subscribe to changes to _all_ `DataStore`s, including those that are yet to be created. This is very powerful as it allows views & triggers implementers to build cross-recording indices as well as be notified as soon as new recordings come in and go out. But it means that `StoreEvent`s must indicate which `DataStore` they originate from, which isn't possible today since the stores themselves don't know who they are to begin with. This trivial PR plumbs the `StoreId` all the way through so `DataStore`s know about their own ID. Also made `StoreGeneration` account for the GC counter while I was at it. --- Requires: - #4215 `DataStore` changelog PR series: - #4202 - #4203 - #4205 - #4206 - #4208 - #4209
e0e5d6d
to
93d6739
Compare
Introduces `StoreEvent`, an event that describes the atomic unit of change in the Rerun `DataStore`: a row has been added to or removed from the store. `StoreEvent`s are fired on both the insertion and garbage collection paths, enabling listeners to build arbitrary, always up-to-date views & trigger systems. ```rust /// The atomic unit of change in the Rerun [`DataStore`]. /// /// A [`StoreEvent`] describes the changes caused by the addition or deletion of a /// [`re_log_types::DataRow`] in the store. /// /// Methods that mutate the [`DataStore`], such as [`DataStore::insert_row`] and [`DataStore::gc`], /// return [`StoreEvent`]s that describe the changes. /// /// Refer to field-level documentation for more details and check out [`StoreDiff`] for a precise /// definition of what an event involves. #[derive(Debug, Clone, PartialEq)] pub struct StoreEvent { /// Which [`DataStore`] sent this event? pub store_id: StoreId, /// What was the store's generation when it sent that event? pub store_generation: StoreGeneration, /// Monotonically increasing ID of the event. /// /// This is on a per-store basis. /// /// When handling a [`StoreEvent`], if this is the first time you process this [`StoreId`] and /// the associated `event_id` is not `1`, it means you registered late and missed some updates. pub event_id: u64, /// What actually changed? /// /// Refer to [`StoreDiff`] for more information. pub diff: StoreDiff, } /// Describes an atomic change in the Rerun [`DataStore`]: a row has been added or deleted. /// /// From a query model standpoint, the [`DataStore`] _always_ operates one row at a time: /// - The contents of a row (i.e. its columns) are immutable past insertion, by virtue of /// [`RowId`]s being unique and non-reusable. /// - Similarly, garbage collection always removes _all the data_ associated with a row in one go: /// there cannot be orphaned columns. When a row is gone, all data associated with it is gone too. /// /// Refer to field-level documentation for more information. #[derive(Debug, Clone, PartialEq)] pub struct StoreDiff { /// Addition or deletion? /// /// The store's internals are opaque and don't necessarily reflect the query model (e.g. there /// might be data in the store that cannot by reached by any query). /// /// A [`StoreDiff`] answers a logical question: "does there exist a query path which can return /// data from that row?". pub kind: StoreDiffKind, /// What's the row's [`RowId`]? /// /// [`RowId`]s are guaranteed to be unique within a single [`DataStore`]. /// /// Put another way, the same [`RowId`] can only appear twice in a [`StoreDiff`] event: /// one addition and (optionally) one deletion (in that order!). pub row_id: RowId, /// The [`TimePoint`] associated with that row. /// /// Since insertions and deletions both work on a row-level basis, this is guaranteed to be the /// same value for both the insertion and deletion events (if any). pub timepoint: TimePoint, /// The [`EntityPath`] associated with that row. /// /// Since insertions and deletions both work on a row-level basis, this is guaranteed to be the /// same value for both the insertion and deletion events (if any). pub entity_path: EntityPath, /// All the [`DataCell`]s associated with that row. /// /// Since insertions and deletions both work on a row-level basis, this is guaranteed to be the /// same set of values for both the insertion and deletion events (if any). pub cells: IntMap<ComponentName, DataCell>, } ``` --- `DataStore` changelog PR series: - #4202 - #4203 - #4205 - #4206 - #4208 - #4209
d5b3373
to
eb01c98
Compare
…4206) Standalone example of how to implement and register custom `StoreView`s, even from external code. <picture> <img src="https://static.rerun.io/custom_store_view/f7258673486f91d944180bd4a83307bce09b741e/full.png" alt=""> <source media="(max-width: 480px)" srcset="https://static.rerun.io/custom_store_view/f7258673486f91d944180bd4a83307bce09b741e/480w.png"> <source media="(max-width: 768px)" srcset="https://static.rerun.io/custom_store_view/f7258673486f91d944180bd4a83307bce09b741e/768w.png"> <source media="(max-width: 1024px)" srcset="https://static.rerun.io/custom_store_view/f7258673486f91d944180bd4a83307bce09b741e/1024w.png"> <source media="(max-width: 1200px)" srcset="https://static.rerun.io/custom_store_view/f7258673486f91d944180bd4a83307bce09b741e/1200w.png"> </picture> --- `DataStore` changelog PR series: - #4202 - #4203 - #4205 - #4206 - #4208 - #4209
This is mostly preliminary work for #4209, which makes this PR a bit weird. Basically just trying to offload complexity from #4209. `TimesPerTimeline` as well as `TimeHistogramPerTimeline` are now living on their own and are maintained as `StoreView`s, i.e. they react to changes to the `DataStore` rather than constructing alternate truths. This is the first step towards turning the `EntityTree` giga-structure into an event-driven view in the next PR. --- `DataStore` changelog PR series: - #4202 - #4203 - #4205 - #4206 - #4208 - #4209
Turns the `EntityTree` giga-datastructure into a `StoreView`, meaning it now reacts to `StoreEvent`s rather than creating alternate truths. This introduces the notion of cascading side-effects, and more specifically `ClearCascade`s. When the `EntityTree` reacts to changes in the store, this might cause cascading effects (e.g. pending clears), that in turn need to write back to the store, which in turn sends more events to react to! The cycle is guaranteed finite because "clears don't get cleared"! Cascading side-effects have an interesting requirement: they need to log their cascaded data using a `RowId` _similar_ to the one used in the original event that caused the cascade (so they get GC'd at roughly the same pace). "Similar" in this cases means that their `TUID` shares the same timestamp and that the new `RowId` is strictly greater than the old one. `PathOp` has finally been annihilated. According to our new "Clears" & "Time Histograms" test suites, this behaves exactly like the `main` branch. --- `DataStore` changelog PR series: - #4202 - #4203 - #4205 - #4206 - #4208 - #4209
Introducing the
StoreView
trait and registration system, allowing anybody to subscribe toDataStore
changes, even from external code.StoreView
s global scope: by registering a view you subscribe to changes to allDataStore
s, including those that are yet to be created.This is very powerful as it allows views & triggers implementers to build cross-recording indices as well as be notified as soon as new recordings come in and go out.
DataStore
changelog PR series:DataStore
changelog 1: letDataStore
s know about theirStoreId
#4202DataStore
changelog 2: introduceStoreEvent
s #4203DataStore
changelog 3: introduceStoreView
s #4205DataStore
changelog 4: add standalone "Custom StoreView" example #4206DataStore
changelog 5: event-driven time histograms #4208DataStore
changelog 6: event-drivenEntityTree
#4209Checklist