Add Aperf Stats #187
Add Aperf Stats #187
Conversation
There was a problem hiding this comment.
I'm not sure the non-time-series graphs are needed, given how tiny their times are. Once we're capturing the log output, we'll have that information available that way, too.
A good learning here is that process collection time is, by a huge margin, the biggest opportunity for improvement.
src/data.rs
Outdated
| pub fn init_data_type(&mut self, param: InitParams) -> Result<u128> { | ||
| let start = Instant::now(); |
There was a problem hiding this comment.
I don't think these functions should be measuring and returning their own time. It should be measured by the caller.
There was a problem hiding this comment.
Yeah. Makes sense.
| @@ -19,6 +19,7 @@ DataTypes.set('flamegraphs', {name: 'flamegraphs', hideClass: '', trueId: '', ca | |||
| DataTypes.set('top_functions', {name: 'topfunctions', hideClass: '', trueId: '', callback: topFunctions}); | |||
| DataTypes.set('processes', {name: 'processes', hideClass: '', trueId: '', callback: processes}); | |||
| DataTypes.set('perfstat', {name: 'perfstat', hideClass: '', trueId: '', callback: perfStat}); | |||
| DataTypes.set('aperfstat', {name: 'aperfstat', hideClass: '', trueId: '', callback: aperfStat}); | |||
There was a problem hiding this comment.
All new data types need to be implemented with support for the hiding of all zeros/N/A graphs, which IIRC also means including support for pairing graphs in side-by-side comparisons.
There was a problem hiding this comment.
We use the emptyOrCallback() which does that.
There was a problem hiding this comment.
Are you sure about that? Your example report disagrees. All of the lines here that don't specify a hideClass and trueId don't support the things I'm saying should be required on all new data types going forward (when possible).
There was a problem hiding this comment.
Oh, the attached report has no graphs at all, which falls into a different category. Still, I'm ~fairly sure that the hideClass and trueId being empty means this does not support the features I am asking for
There was a problem hiding this comment.
Saw offline that this works, yay
src/lib.rs
Outdated
| let collect_time: u64 = datatype.collect_data()? as u64; | ||
| let print_time: u64 = datatype.write_to_file()? as u64; | ||
| aperf_collect_data | ||
| .data | ||
| .insert(name.clone() + "-collect", collect_time); | ||
| aperf_collect_data | ||
| .data | ||
| .insert(name.clone() + "-print", print_time); |
There was a problem hiding this comment.
Encapsulating the measurements like this can be made more robust by taking advantage of passing a function to a helper
aperf_collect_data.measure(name.clone() + "-collect", |datatype| datatype.collect_data())
I'm sure there's syntax errors in there, but it's just an example
such a measure function can do the timing and the appending of the data to self (aperf_collect_data). It removes the need for repeated timing code and avoids the risk of, for example in this case, of the collect and print times getting mixed up. It's also less code in every place doing this.
There was a problem hiding this comment.
Yeah. Changed it to have a measure() which does the timing and updates the self.data.
janaknat
force-pushed
the
aperf-stats
branch
2 times, most recently
from
a32c1dd to
c045f15
Compare
src/lib.rs
Outdated
| F: Fn(&mut data::DataType) -> Result<()>, | ||
| { | ||
| let start_time = time::Instant::now(); | ||
| func(datatype)?; |
There was a problem hiding this comment.
Could you not just have this be generic, taking a no-args function and executing it?
That should be possible. The caller creates a lambda that captures everything.
There was a problem hiding this comment.
Yeah. Changing it.
src/lib.rs
Outdated
| aperf_collect_data.measure( | ||
| name.clone() + "-print", | ||
| |d: &mut data::DataType| -> Result<()> { | ||
| d.write_to_file()?; | ||
| Ok(()) | ||
| }, | ||
| datatype, | ||
| )?; |
There was a problem hiding this comment.
I think this should be able to be
aperf_collect_data.measure(
name.clone() + "-print",
|| -> Result<()> {
d.write_to_file()
}
);
if measure() is made to take a no-args function. I guess it would still have to be typed to return Result<()> since we do have to keep failure handling working
Collect the time it takes the various aperf data types to gather the data and print them to a file. This also adds a tab in the report.
| func()?; | ||
| let func_time: u64 = (time::Instant::now() - start_time).as_micros() as u64; | ||
| self.data.insert(name, func_time); | ||
| Ok(()) |
There was a problem hiding this comment.
It would be slightly better here to record the time even in failure cases, like
let res = func();
// get and store time taken
res
Collect the time it takes the various aperf data types to gather the data and print them to a file. This also adds a tab in the report.
aperf_report_stats.tar.gz
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.