Skip to content
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

Add Variable statistics #21

Merged
merged 1 commit into from Jan 11, 2019
Merged

Add Variable statistics #21

merged 1 commit into from Jan 11, 2019

Conversation

lqd
Copy link
Member

@lqd lqd commented Dec 12, 2018

This adds simple profiling statistics in the spirit of #5, but inspired by Soufflé's profiler: adding the statistics per identifiable round.

For each round, there's the stable and recent tuple counts for each variable, as CSV, output to a io::Write of the user's choice.

We can talk about what precise data (or format) we'd like to see here. In the future, we can also add a self-profiling option to tally up the time each join operation took to create the tuples.

Here's how it looks right now
Variable,Round,Stable count,Recent count
"subset",1,0,10
"requires",1,0,3
"potential_errors",1,0,0
"subset",2,10,0
"requires",2,3,3
"potential_errors",2,0,0
"subset",3,10,0
"requires",3,6,3
"potential_errors",3,0,0

@lqd
Copy link
Member Author

lqd commented Dec 12, 2018

(IIRC the CI failure is due to current infra nightly build issues, or in general rustfmt not being available on the latest nightly or similar)

@lqd
Copy link
Member Author

lqd commented Dec 19, 2018

(The CI issue was indeed temporary)

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

@nikomatsakis nikomatsakis self-assigned this Dec 21, 2018
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea! Sorry for being slow.

src/lib.rs Outdated
@@ -437,6 +452,22 @@ impl<Tuple: Ord> VariableTrait for Variable<Tuple> {

!self.recent.borrow().is_empty()
}

#[cfg(feature = "stats")]
fn dump_stats(&self, round: u32) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should take a &mut Write to write to; I'd like to be able to dump it to a file or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though what I might like best would be to accumulate the stats into some sort of data structure that you can dump later, but .. I'm not sure why. I guess I was thinking it might be nice to have the output in CSV format or something so you can pull it into a spreadsheet and graph it or whatever.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/lib.rs Outdated
@@ -162,6 +173,10 @@ impl Iteration {
trait VariableTrait {
/// Reports whether the variable has changed since it was last asked.
fn changed(&mut self) -> bool;

#[cfg(feature = "stats")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the feature flag is worthwhile. It doesn't seem to add any cost if you don't invoke dump_stats

@lqd
Copy link
Member Author

lqd commented Jan 9, 2019

I'm working on dumping the stats to a file, so I'll close this PR until then.

@lqd lqd closed this Jan 9, 2019
@lqd lqd reopened this Jan 9, 2019
@lqd lqd changed the title Add Variable statistics under a feature flag Add Variable statistics Jan 9, 2019
let mut result = false;
for variable in self.variables.iter_mut() {
if variable.changed() {
result = true;
}

if let Some(ref mut stats_writer) = self.debug_stats {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the intended usage here is that we invoke

iteration.record_stats_to(writer);

and then it will dump out the data as it goes?

That seems pretty nice, yeah.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usage-wise: exactly. Regarding the data dumping as it goes: there's also the other possibility of keeping the stats in memory (which would also be good in order to add stats about the per-variable join durations). Whichever you and @frankmcsherry prefer.

@nikomatsakis nikomatsakis merged commit 34055ab into rust-lang:master Jan 11, 2019
@lqd lqd deleted the stats branch January 12, 2019 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants