Time phases #938
Time phases #938
Conversation
There was a problem hiding this comment.
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 :)
src/ir/context.rs
Outdated
| @@ -378,6 +384,7 @@ impl<'ctx> BindgenContext<'ctx> { | |||
| parsed_macros: Default::default(), | |||
| replacements: Default::default(), | |||
| collected_typerefs: false, | |||
| time_phases: options.time_phases, | |||
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Nitpick: these comments can both be deleted.
src/time_phase.rs
Outdated
|
|
||
|
|
||
| /// RAII timer to measure how long phases take. | ||
| // TODO does it need output parameter? or always print to stdout |
There was a problem hiding this comment.
Always printing to stderr is fine.
src/time_phase.rs
Outdated
| 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 |
There was a problem hiding this comment.
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 { | |||
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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"); | |||
There was a problem hiding this comment.
Yep, I see you figured out how to do RAII variables that are only used for their Drop without getting unused variable warnings :)
There was a problem hiding this comment.
It's a learning process :)
src/codegen/mod.rs
Outdated
| @@ -3478,6 +3478,7 @@ impl CodeGenerator for ObjCInterface { | |||
| } | |||
|
|
|||
| pub fn codegen(context: &mut BindgenContext) -> Vec<P<ast::Item>> { | |||
| let _t = context.timer("codegen"); | |||
There was a problem hiding this comment.
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
|
@fitzgen I think all fixed |
|
@bors-servo r+ Perfect! Thanks @jhod0 ! |
|
📌 Commit e3c31dd has been approved by |
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
|
☀️ Test successful - status-travis |
New module
time_phaseswith an RAII timer which prints to stderr, and command-line flag--time-phasesto enable it.Fixes #933