-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
stat for time spent copying generate index contents #33187
Conversation
e49029d
to
a02f41e
Compare
Codecov Report
@@ Coverage Diff @@
## master #33187 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 786 786
Lines 211596 211606 +10
=======================================
+ Hits 173777 173851 +74
+ Misses 37819 37755 -64 |
@@ -516,6 +517,7 @@ impl BucketMapHolderStats { | |||
.swap(0, Ordering::Relaxed), | |||
i64 | |||
), | |||
("copy_us", self.copy_us.swap(0, Ordering::Relaxed), i64), |
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.
Wdyt about a more descriptive name for this datapoint? Would startup_insert_us
work? Or maybe startup_copy_us
?
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.
yeah, moved these to generate_index stats. Reduces overall noise.
@@ -68,6 +70,8 @@ pub struct BucketMapHolder<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> | |||
/// Note startup is an optimization and is not required for correctness. | |||
startup: AtomicBool, | |||
_phantom: PhantomData<T>, | |||
|
|||
pub(crate) startup_stats: Arc<StartupStats>, |
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.
Why are the startup stats also added here? Is this needed? Or maybe for a future use?
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.
because the holder holds the accessible one. it gets retrieved from here.
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.
each bin holds a reference to the common one. I guess they could all reference the common one instead.
Problem
Speeding up generate index/startup time.
We spend a ton of time copying data.
Summary of Changes
Add a metric for that time.
Fixes #