-
Notifications
You must be signed in to change notification settings - Fork 2
feat(store): added store interface for batched exact-window queries #627
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
Changes from all commits
be58526
47904b6
dd92fcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ use crate::data_model::{ | |
| AggregateCore, AggregationType, CleanupPolicy, PrecomputedOutput, StreamingConfig, | ||
| }; | ||
| use crate::stores::simple_map_store::common::{ | ||
| sort_buckets_chronologically, EpochID, InternTable, MetricBucketMap, MetricID, MutableEpoch, | ||
| SealedEpoch, TimestampRange, | ||
| resolve_exact_windows, sort_buckets_chronologically, EpochID, InternTable, MetricBucketMap, | ||
| MetricID, MutableEpoch, SealedEpoch, TimestampRange, | ||
| }; | ||
| use crate::stores::{Store, StoreResult, TimestampedBucketsMap}; | ||
| use dashmap::DashMap; | ||
|
|
@@ -756,6 +756,119 @@ impl Store for SimpleMapStorePerKey { | |
| Ok(results) | ||
| } | ||
|
|
||
| /// Batched exact-window lookup (#609): acquires the shard's read lock once for the | ||
| /// whole `windows` slice instead of once per window, resolving each window against | ||
| /// `current_epoch` / `sealed_epochs` in a single pass. Otherwise identical semantics | ||
| /// to calling `query_precomputed_output_exact` once per window and merging the | ||
| /// results (a window with no exact match simply contributes nothing). | ||
| fn query_precomputed_output_exact_batch( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new batched exact-query methods drop the Building with |
||
| &self, | ||
| metric: &str, | ||
| aggregation_id: u64, | ||
| windows: &[TimestampRange], | ||
| ) -> Result<TimestampedBucketsMap, Box<dyn std::error::Error + Send + Sync>> { | ||
| if windows.is_empty() { | ||
| return Ok(HashMap::new()); | ||
| } | ||
|
|
||
| let query_start_time = Instant::now(); | ||
| let store_key = aggregation_id; | ||
|
|
||
| #[cfg(feature = "lock_profiling")] | ||
| let lock_wait_start = Instant::now(); | ||
|
|
||
| let store_data_lock = match self.store.get(&store_key) { | ||
| Some(lock) => lock, | ||
| None => { | ||
| debug!( | ||
| "Metric {} not found in store for batched exact query", | ||
| metric | ||
| ); | ||
| return Ok(HashMap::new()); | ||
| } | ||
| }; | ||
|
|
||
| #[cfg(feature = "lock_profiling")] | ||
| { | ||
| let lock_wait_duration = lock_wait_start.elapsed(); | ||
| info!( | ||
| "🔒 Batched exact query DashMap get time: {:.2}ms (metric: {}, agg_id: {}, windows: {})", | ||
| lock_wait_duration.as_secs_f64() * 1000.0, | ||
| metric, | ||
| aggregation_id, | ||
| windows.len() | ||
| ); | ||
| } | ||
|
|
||
| #[cfg(feature = "lock_profiling")] | ||
| let rwlock_wait_start = Instant::now(); | ||
|
|
||
| // Same rationale as query_precomputed_output_exact: exact_query takes &self, so a | ||
| // read lock covers the whole batch (issue #607). | ||
| let data = store_data_lock.read().map_err(|e| { | ||
| format!( | ||
| "Failed to acquire read lock for batched exact query aggregation_id {}: {}", | ||
| store_key, e | ||
| ) | ||
| })?; | ||
|
|
||
| #[cfg(feature = "lock_profiling")] | ||
| { | ||
| let rwlock_wait_duration = rwlock_wait_start.elapsed(); | ||
| info!( | ||
| "🔒 Batched exact query RwLock wait time: {:.2}ms (metric: {}, agg_id: {}, windows: {})", | ||
| rwlock_wait_duration.as_secs_f64() * 1000.0, | ||
| metric, | ||
| aggregation_id, | ||
| windows.len() | ||
| ); | ||
| } | ||
|
|
||
| #[cfg(feature = "lock_profiling")] | ||
| let lock_hold_start = Instant::now(); | ||
|
|
||
| let (results, found_windows, total_entries) = resolve_exact_windows( | ||
| &data.current_epoch, | ||
| &data.sealed_epochs, | ||
| &data.intern, | ||
| windows, | ||
| metric, | ||
| aggregation_id, | ||
| ); | ||
|
|
||
| // Batch the read-count update too: one inner-Mutex acquisition for every window | ||
| // that hit, instead of one per window. | ||
| if !found_windows.is_empty() { | ||
| let mut read_counts = data.read_counts.lock().unwrap(); | ||
| for window in &found_windows { | ||
| *read_counts.entry(*window).or_insert(0) += 1; | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "lock_profiling")] | ||
| { | ||
| let lock_hold_duration = lock_hold_start.elapsed(); | ||
| info!( | ||
| "🔓 Batched exact query lock hold time: {:.2}ms (metric: {}, agg_id: {}, matched: {})", | ||
| lock_hold_duration.as_secs_f64() * 1000.0, | ||
| metric, | ||
| aggregation_id, | ||
| found_windows.len() | ||
| ); | ||
| } | ||
|
|
||
| let query_duration = query_start_time.elapsed(); | ||
| debug!( | ||
| "Batched exact timestamp query took: {:.2}ms ({} windows requested, {} matched, {} entries)", | ||
| query_duration.as_secs_f64() * 1000.0, | ||
| windows.len(), | ||
| found_windows.len(), | ||
| total_entries | ||
| ); | ||
|
|
||
| Ok(results) | ||
| } | ||
|
|
||
| fn get_earliest_timestamp_per_aggregation_id( | ||
| &self, | ||
| ) -> Result<HashMap<u64, u64>, Box<dyn std::error::Error + Send + Sync>> { | ||
|
|
||
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.
query_precomputed_output_exact_batchis duplicated almost line-for-line between this file and per_key.rs (epoch lookup loop, read-count update loop, debug logging), adding ~90 more lines of near-identical logic that must be kept in sync by hand.A future fix to the exact-match resolution order (current_epoch then sealed_epochs newest-to-oldest) or to the read-count bookkeeping could get applied to one copy (per_key.rs:801-831) and forgotten in the other (here, 722-753), silently reintroducing a bug in one of the two lock strategies — the contract test suite may not catch a strategy-specific regression.