-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 --trace-compile-timing
arg to add compile timing comments
#54662
add --trace-compile-timing
arg to add compile timing comments
#54662
Conversation
IanButterworth
commented
Jun 3, 2024
•
edited
Loading
edited
6739936
to
b0d7231
Compare
Love the look of this, the only improvement that comes to mind is rounding the timings printed to 3 sig fig. |
case opt_polly: | ||
case opt_polly: |
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.
Unrelated whitespace change?
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, flyby formatting fix
With how short and easy to read these changes are, I'd say this LGTM. |
b0d7231
to
a16e5a6
Compare
Updated to 3 d.p.
|
Just noting that this is quite nice in conjunction with |
Looking at #51106 it's probably better to implement this as |
Now implemented via simply Talking this through with @tecosaur, some other changes: To make the information easier to compare, the info has been moved to before the statement, in an inline comment so that the raw output could still be loaded in a Also the time is printed fixed width left padded, to align the decimal in the same way that our other timing tooling does
|
Now only printing as a comment when sent to file
|
@JeffBezanson I requested a review given the similarity to #51106 |
I would probably always write the comments since it is emitting Julia code after all and it is often nice to copy paste this even from a terminal output. |
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.
It’d be pretty nice if we could add an option that would sort this list by time
These are output as compilation happens, nothing is stored. I believe SnoopCompile already does that kind of thing https://timholy.github.io/SnoopCompile.jl/dev/ |
Having some test code that at least exercises the new command line flag would be good. |
b7e49a8
to
6fc39ef
Compare
6fc39ef
to
df768cc
Compare
3455680
to
3f40180
Compare
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.
double compile_time = jl_hrtime(); | ||
int did_compile = jl_compile_codeinst(codeinst); | ||
compile_time = jl_hrtime() - compile_time; |
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.
@IanButterworth: Can you help me understand why we have so many different variants of measuring this metric (compilation time)?
I was surprised to see we had to add a new start/stop measurement here. We already have a global metric for compilation time, which has timings around inference and llvm codegen. Why didn't we add to the per-method-instance compilation time counters from those places?
Or, conversely, if this spot can correctly capture all compilation time for a given method instance, can we delete those other spots and incremental global compilation time here?
Lines 4390 to 4396 in fe86437
if (jl_atomic_load_relaxed(&jl_measure_compile_time_enabled)) { uint64_t inftime = jl_hrtime() - start; jl_atomic_fetch_add_relaxed(&jl_cumulative_compile_time, inftime); if (is_recompile) { jl_atomic_fetch_add_relaxed(&jl_cumulative_recompile_time, inftime); } } Lines 571 to 572 in a98f371
auto end = jl_hrtime(); jl_atomic_fetch_add_relaxed(&jl_cumulative_compile_time, end - compiler_start_time); Lines 578 to 579 in a98f371
auto end = jl_hrtime(); jl_atomic_fetch_add_relaxed(&jl_cumulative_compile_time, end - compiler_start_time); - ... etc
What's the difference between these approaches? Can we unify them?
Thanks!
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.
Maybe that can be reused, yeah. The timer added here seems lower cost than atomic fetches etc. but perhaps it's overcounting.
…aLang#54662) (#187) Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
…aLang#54662) (#187) Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>