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

Time phases #938

Merged
merged 1 commit into from
Aug 31, 2017
Merged

Time phases #938

merged 1 commit into from
Aug 31, 2017

Conversation

jhod0
Copy link
Contributor

@jhod0 jhod0 commented Aug 30, 2017

New module time_phases with an RAII timer which prints to stderr, and command-line flag --time-phases to enable it.

Fixes #933

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks good!

One missing piece is that we need to add the --time-phases flag to the computed command line flags in bindgen::Builder::command_line_flags if the option is set.

I also have a few nitpick comments inline below.

Finally, can you please rebase and squash all the commits into a single commit? https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing is a great resource if you need a refresher.

Thanks @jhod0 ! I'm excited to have this feature :)

@@ -378,6 +384,7 @@ impl<'ctx> BindgenContext<'ctx> {
parsed_macros: Default::default(),
replacements: Default::default(),
collected_typerefs: false,
time_phases: options.time_phases,
Copy link
Member

Choose a reason for hiding this comment

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

We should just be able to do self.options().time_phases; no need to make a time_phases member on BindgenContext.

src/lib.rs Outdated
@@ -63,7 +63,9 @@ mod clang;
mod features;
mod ir;
mod parse;
//mod raii_timer;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: remove commented out code.

src/lib.rs Outdated
try!(parse(&mut context));
{
// TODO this is a dummy value for now, should be command line flag
//let time_passes = false;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: these comments can both be deleted.



/// RAII timer to measure how long phases take.
// TODO does it need output parameter? or always print to stdout
Copy link
Member

Choose a reason for hiding this comment

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

Always printing to stderr is fine.

impl<'a> Timer<'a> {
/// Creates a Timer with the given name, and starts it.
///
/// By default, the output stream is None, and nothing is printed
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is out of date, possible referring to a time when the output was configurable.

src/lib.rs Outdated
@@ -774,6 +776,13 @@ impl Builder {
self
}

/// Set whether or not to time bindgen phases, and print
/// information to stderr.
pub fn time_phases(mut self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with our other on/off features and options, this should have a doit: bool parameter, and set self.options.time_phases = doit;

src/lib.rs Outdated
mod regex_set;
mod time_phase;
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename the module to timer.

@@ -731,6 +745,7 @@ impl<'ctx> BindgenContext<'ctx> {
/// Iterate over all items and replace any item that has been named in a
/// `replaces="SomeType"` annotation with the replacement type.
fn process_replacements(&mut self) {
let _t = self.timer("process_replacements");
Copy link
Member

Choose a reason for hiding this comment

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

Yep, I see you figured out how to do RAII variables that are only used for their Drop without getting unused variable warnings :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a learning process :)

@@ -3478,6 +3478,7 @@ impl CodeGenerator for ObjCInterface {
}

pub fn codegen(context: &mut BindgenContext) -> Vec<P<ast::Item>> {
let _t = context.timer("codegen");
Copy link
Member

Choose a reason for hiding this comment

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

This is going to make codegen's timer include all of our analyses that get run from gen, which we don't really want here.

Let's move the timer into the closure passed to gen below.

initial timer branch commit

added time_phases builder parameter

create timers for BindgenContex analyses, codegen

time parse

added docstrings

added fitzgens suggestions

add --time-phases to bindgen::Builder::command_line_flags
@jhod0
Copy link
Contributor Author

jhod0 commented Aug 31, 2017

@fitzgen I think all fixed

@fitzgen
Copy link
Member

fitzgen commented Aug 31, 2017

@bors-servo r+

Perfect! Thanks @jhod0 !

@bors-servo
Copy link

📌 Commit e3c31dd has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit e3c31dd with merge e841f6f...

bors-servo pushed a commit that referenced this pull request Aug 31, 2017
Time phases

New module `time_phases` with an RAII timer which prints to stderr, and command-line flag `--time-phases` to enable it.

Fixes #933
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing e841f6f to master...

@bors-servo bors-servo merged commit e3c31dd into rust-lang:master Aug 31, 2017
@jhod0 jhod0 deleted the time_phases branch August 31, 2017 17:11
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.

4 participants