Skip to content

Commit fb01c00

Browse files
committed
compute: track a dataflow's exports explicitly
`handle_schedule` needs a dataflow's other exports, and inferred them from `Rc` bookkeeping: a strong count of two meant "one export, plus the clone I just took", and anything else fell back to a pointer-equality scan over every collection. Both encoded a fact about the function's own locals rather than about the dataflow. `ComputeState::dataflow_exports` records the export set directly, keyed by dataflow index, which timely mints from a per-worker counter that only increases. `insert_collection` is now the one path that installs a collection, so the two maps cannot drift. That leaves one representation of the fact rather than two, so `dataflow_index` goes back to a plain `usize` and `drop_collection` decides by emptiness of the export set instead of by `Rc::try_unwrap`. The same map also answers whether the dataflow is still suspended, by asking whether any of its exports still holds a token, replacing a second `Rc::strong_count` read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZVCMBSLdxzGus78ZKWhZz
1 parent 37a7b35 commit fb01c00

1 file changed

Lines changed: 65 additions & 56 deletions

File tree

src/compute/src/compute_state.rs

Lines changed: 65 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,13 @@ pub struct ComputeState {
9393
/// * Persist sinks store their current frontier in `CollectionState::sink_write_frontier`.
9494
/// * Subscribes report their frontiers through the `subscribe_response_buffer`.
9595
pub collections: BTreeMap<GlobalId, CollectionState>,
96+
/// The exports of each installed dataflow, keyed by dataflow index.
97+
///
98+
/// Timely mints indices from a per-worker counter that only increases, so an index is never
99+
/// reused within a process. Maintained alongside `collections` by
100+
/// [`ComputeState::insert_collection`] and [`ActiveComputeState::drop_collection`], which is
101+
/// the only reason a dataflow's export set is knowable without scanning every collection.
102+
dataflow_exports: BTreeMap<usize, BTreeSet<GlobalId>>,
96103
/// The traces available for sharing across dataflows.
97104
pub traces: TraceManager,
98105
/// Shared buffer with SUBSCRIBE operator instances by which they can respond.
@@ -208,6 +215,7 @@ impl ComputeState {
208215
worker_config: mz_dyncfgs::all_dyncfgs().into(),
209216
metrics_registry,
210217
workers_per_process,
218+
dataflow_exports: Default::default(),
211219
suspended_collections: Default::default(),
212220
server_maintenance_interval: Duration::ZERO,
213221
init_system_time: mz_ore::now::SYSTEM_TIME(),
@@ -216,6 +224,21 @@ impl ComputeState {
216224
}
217225
}
218226

227+
/// Install the state for a new collection and record it as an export of its dataflow.
228+
///
229+
/// Returns the state this displaced, which is always a bug in the caller.
230+
fn insert_collection(
231+
&mut self,
232+
id: GlobalId,
233+
collection: CollectionState,
234+
) -> Option<CollectionState> {
235+
self.dataflow_exports
236+
.entry(collection.dataflow_index)
237+
.or_default()
238+
.insert(id);
239+
self.collections.insert(id, collection)
240+
}
241+
219242
/// Return a mutable reference to the identified collection.
220243
///
221244
/// Panics if the collection doesn't exist.
@@ -624,7 +647,7 @@ impl<'a> ActiveComputeState<'a> {
624647
&mut self,
625648
dataflow: DataflowDescription<RenderPlan, CollectionMetadata>,
626649
) {
627-
let dataflow_index = Rc::new(self.timely_worker.next_dataflow_index());
650+
let dataflow_index = self.timely_worker.next_dataflow_index();
628651
let as_of = dataflow.as_of.clone().unwrap();
629652

630653
let dataflow_expiration = dataflow
@@ -689,18 +712,14 @@ impl<'a> ActiveComputeState<'a> {
689712
for object_id in dataflow.export_ids() {
690713
let is_subscribe_or_copy = subscribe_copy_ids.contains(&object_id);
691714
let metrics = self.compute_state.metrics.for_collection(object_id);
692-
let mut collection = CollectionState::new(
693-
Rc::clone(&dataflow_index),
694-
is_subscribe_or_copy,
695-
as_of.clone(),
696-
metrics,
697-
);
715+
let mut collection =
716+
CollectionState::new(dataflow_index, is_subscribe_or_copy, as_of.clone(), metrics);
698717

699718
if let Some(logger) = self.compute_state.compute_logger.clone() {
700719
let logging = CollectionLogging::new(
701720
object_id,
702721
logger,
703-
*dataflow_index,
722+
dataflow_index,
704723
as_of.as_option().copied(),
705724
dataflow.import_ids(),
706725
);
@@ -714,7 +733,7 @@ impl<'a> ActiveComputeState<'a> {
714733
lower: as_of.clone(),
715734
});
716735

717-
let existing = self.compute_state.collections.insert(object_id, collection);
736+
let existing = self.compute_state.insert_collection(object_id, collection);
718737
if existing.is_some() {
719738
error!(
720739
id = ?object_id,
@@ -746,48 +765,39 @@ impl<'a> ActiveComputeState<'a> {
746765
// dataflow can export multiple collections and they all share one suspension token, so the
747766
// computation of a dataflow will only start once all its exported collections have been
748767
// scheduled.
749-
let suspension_token = self.compute_state.suspended_collections.remove(&id);
750-
// Only the last token release actually unsuspends the dataflow, and the signal holds no
751-
// strong reference of its own, so this is the whole outstanding count.
752-
let unsuspended = suspension_token
753-
.as_ref()
754-
.is_some_and(|token| Rc::strong_count(token) == 1);
755-
drop(suspension_token);
756-
757-
if !unsuspended {
758-
return;
759-
}
768+
self.compute_state.suspended_collections.remove(&id);
760769

761770
// Report the start for every export of the dataflow, not just the one this command named.
762771
// Computation begins for all of them at this instant, so crediting each export from its
763772
// own `Schedule` would date the earlier ones to before their dataflow was running and
764773
// overstate the compute time between `started` and `snapshot_complete`.
765-
let Some(dataflow_index) = self
774+
let Some(collection) = self.compute_state.collections.get(&id) else {
775+
return;
776+
};
777+
let Some(export_ids) = self
766778
.compute_state
767-
.collections
768-
.get(&id)
769-
.map(|c| Rc::clone(&c.dataflow_index))
779+
.dataflow_exports
780+
.get(&collection.dataflow_index)
770781
else {
771782
return;
772783
};
773-
// Two strong references, this collection's and the clone above, mean this is the
774-
// dataflow's only export and there is nothing to scan for. `Schedule` arrives once per
775-
// dataflow, so without this the sweep is quadratic in the number of collections, on the
776-
// timely worker thread, exactly while start-up latency matters.
777-
if Rc::strong_count(&dataflow_index) == 2 {
778-
if let Some(collection) = self.compute_state.collections.get(&id) {
779-
if let Some(logging) = &collection.logging {
780-
logging.set_hydration_start();
781-
}
782-
}
784+
785+
// An export still holding its token means the dataflow is still suspended, so there is no
786+
// start to report yet.
787+
let still_suspended = export_ids
788+
.iter()
789+
.any(|id| self.compute_state.suspended_collections.contains_key(id));
790+
if still_suspended {
783791
return;
784792
}
785793

786-
for collection in self.compute_state.collections.values() {
787-
if !Rc::ptr_eq(&collection.dataflow_index, &dataflow_index) {
788-
continue;
789-
}
790-
if let Some(logging) = &collection.logging {
794+
for export_id in export_ids {
795+
let logging = self
796+
.compute_state
797+
.collections
798+
.get(export_id)
799+
.and_then(|c| c.logging.as_ref());
800+
if let Some(logging) = logging {
791801
logging.set_hydration_start();
792802
}
793803
}
@@ -871,8 +881,13 @@ impl<'a> ActiveComputeState<'a> {
871881
self.compute_state.suspended_collections.remove(&id);
872882

873883
// Drop the dataflow, if all its exports have been dropped.
874-
if let Ok(index) = Rc::try_unwrap(collection.dataflow_index) {
875-
self.timely_worker.drop_dataflow(index);
884+
let index = collection.dataflow_index;
885+
if let Some(exports) = self.compute_state.dataflow_exports.get_mut(&index) {
886+
exports.remove(&id);
887+
if exports.is_empty() {
888+
self.compute_state.dataflow_exports.remove(&index);
889+
self.timely_worker.drop_dataflow(index);
890+
}
876891
}
877892

878893
// The compute protocol requires us to send a `Frontiers` response with empty frontiers
@@ -919,7 +934,6 @@ impl<'a> ActiveComputeState<'a> {
919934
storage_log_reader,
920935
);
921936

922-
let dataflow_index = Rc::new(dataflow_index);
923937
let mut log_index_ids = config.index_logs;
924938
for (log, trace) in traces {
925939
// Install trace as maintained index.
@@ -932,17 +946,13 @@ impl<'a> ActiveComputeState<'a> {
932946
let is_subscribe_or_copy = false;
933947
let as_of = Antichain::from_elem(Timestamp::MIN);
934948
let metrics = self.compute_state.metrics.for_collection(id);
935-
let mut collection = CollectionState::new(
936-
Rc::clone(&dataflow_index),
937-
is_subscribe_or_copy,
938-
as_of.clone(),
939-
metrics,
940-
);
949+
let mut collection =
950+
CollectionState::new(dataflow_index, is_subscribe_or_copy, as_of.clone(), metrics);
941951

942952
let logging = CollectionLogging::new(
943953
id,
944954
logger.clone(),
945-
*dataflow_index,
955+
dataflow_index,
946956
as_of.as_option().copied(),
947957
std::iter::empty(),
948958
);
@@ -953,7 +963,7 @@ impl<'a> ActiveComputeState<'a> {
953963
logging.set_hydration_start();
954964
collection.logging = Some(logging);
955965

956-
let existing = self.compute_state.collections.insert(id, collection);
966+
let existing = self.compute_state.insert_collection(id, collection);
957967
if existing.is_some() {
958968
error!(
959969
id = ?id,
@@ -2048,10 +2058,9 @@ pub struct CollectionState {
20482058
reported_frontiers: ReportedFrontiers,
20492059
/// The index of the dataflow computing this collection.
20502060
///
2051-
/// Used for dropping the dataflow when the collection is dropped.
2052-
/// The Dataflow index is wrapped in an `Rc`s and can be shared between collections, to reflect
2053-
/// the possibility that a single dataflow can export multiple collections.
2054-
dataflow_index: Rc<usize>,
2061+
/// A dataflow can compute more than one collection. Which ones is tracked by
2062+
/// `ComputeState::dataflow_exports`, which is also what decides when the dataflow is dropped.
2063+
dataflow_index: usize,
20552064
/// Whether this collection is a subscribe or copy-to.
20562065
///
20572066
/// The compute protocol does not allow `Frontiers` responses for subscribe and copy-to
@@ -2107,7 +2116,7 @@ pub struct CollectionState {
21072116

21082117
impl CollectionState {
21092118
fn new(
2110-
dataflow_index: Rc<usize>,
2119+
dataflow_index: usize,
21112120
is_subscribe_or_copy: bool,
21122121
as_of: Antichain<Timestamp>,
21132122
metrics: CollectionMetrics,
@@ -2231,7 +2240,7 @@ impl CollectionState {
22312240
/// Allow writes for this collection.
22322241
fn allow_writes(&self) {
22332242
info!(
2234-
dataflow_index = *self.dataflow_index,
2243+
dataflow_index = self.dataflow_index,
22352244
export = ?self.logging.as_ref().map(|l| l.export_id()),
22362245
"allowing writes for dataflow",
22372246
);

0 commit comments

Comments
 (0)