-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Report any recompilation in time macro #45015
Report any recompilation in time macro #45015
Conversation
Currently the report is a little wordy. But the two %'s need explaining, given the 2nd is % of the first
It could just be two separate categories, so the above would show as
And in an imaginary case, one might have
|
1615895
to
7abaab3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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.
I like your alternative display proposal and would recommend switching to it, but other than that and a little small stuff, this LGTM.
c831fb9
to
fe528c1
Compare
Updated to two separate categories for
They are a little visually similar, especially when only one shows. I guess if that turns out to be confusing some formatting can be added |
fe528c1
to
a6f76be
Compare
An example for Flux v0.13.0:
(requires an update to ChainRules, which uses an internal that changed here) |
Very cool change! The new wording feels more confusing than the former to me, because recompilation is compilation, and so it's not clear whether the second number is a percentage of the first or not. The "of which" makes that clearer. "X% precompilation, Y% recompilation" could disambiguate, but it risks confusion with the (somewhat orthogonal if I understand correctly) notion of package precompilation |
Perhaps the refined alternative is
|
seems a reasonable approximation |
52% of total or 52% of 55.57%? |
52% of 55.57% Yeah, there must be a concise way to express this clearly.. |
Best I can come up with is 13.176179 seconds (21.10 M allocations: 1.468 GiB, 3.85% gc time, 55.57% compilation time: 52% of which was recompilation) i.e. ~half of the compile time was recompilation |
This last option seems very concise but also clear IMO |
Great. I think I might make 100% print as "all". 13.176179 seconds (21.10 M: 1.468 GiB, 3.85% gc time, 55.57% compilation time: all of which was recompilation) And 0% just 13.176179 seconds (21.10 M: 1.468 GiB, 3.85% gc time, 55.57% compilation time) |
Maybe this is just as clear? The : may be doing the heavy lifting. 13.176179 seconds (21.10 M: 1.468 GiB, 3.85% gc time, 55.57% compilation time: all recompilation) 13.176179 seconds (21.10 M: 1.468 GiB, 3.85% gc time, 55.57% compilation time: 52% recompilation) 13.176179 seconds (21.10 M: 1.468 GiB, 3.85% gc time, 55.57% compilation time) |
Or get rid of the outer parentheses to use them here: 13.176179 seconds, 21.10 M (1.468 GiB) allocs, 3.85% gc time, 55.57% compilation time (52% recompilation) Still not completely clear to me whether the 52% is of the 55% or of 100... Not sure adding "of which" takes up that much more space (there would be value in staying within 80 columns, but there's no chance of that happening with the extra information) |
I wouldn't use the word |
There seems to be slightly more support for the current format over others, so I'll merge and adjustments can be made if improvements are identified in the wild. |
`@time` now shows if any of the compilation time was spent recompiling invalidated methods. The new percentage is % of the compilation time, not the total execution time. (cherry picked from commit 7074f04)
local compile_elapsedtime = cumulative_compile_time_ns_before() | ||
local compile_elapsedtimes = cumulative_compile_time_ns() |
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: doesn't this need to call cumulative_compile_timing(true)
here as well, to enable compilation timing?
Otherwise, I think this is currently wrong: i think the transformation here isn't enabling compilation time tracking, so you'll probably always see 0
compilation time, unless compilation time tracking is already enabled?
I guess we must not have any tests for this, specifically? :(
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.
Ah yes. My bad. Fix here. Haven't run the tests yet, might need work #46100
@IanButterworth this is super cool! :) 🎉 It seems like maybe there's a small bug here, where we aren't disabling Notice how in this example, both of the last two compilations of julia> foo() = bar()
foo (generic function with 1 method)
julia> bar() = 2
bar (generic function with 1 method)
julia> @time @eval foo()
0.000281 seconds (805 allocations: 52.906 KiB, 48.30% compilation time)
2
julia> bar() = 3
bar (generic function with 1 method)
julia> @time @eval bar()
0.000241 seconds (392 allocations: 26.141 KiB, 31.56% compilation time)
3
julia> @time @eval foo()
0.000379 seconds (454 allocations: 28.562 KiB, 50.04% compilation time: 100% of which was recompilation)
3
julia> bar() = 4
bar (generic function with 1 method)
julia> @time @eval foo()
0.000293 seconds (798 allocations: 52.406 KiB, 47.01% compilation time: 100% of which was recompilation)
4 What do you think? Should I open a separate issue to discuss this? |
Seems worth an issue, yeah |
Thanks. Opened #46101. |
It may be helpful if
@time
indicates if any of the compilation time was recompilation, i.e. of invalidated methods. If the % is high the user could use the SnoopCompile tooling to see what's happening, for instance.I'm not sure the heuristic is correct or broad-enough hereUpdated after review. It appears to work with the example from the SnoopCompile manual.Note the final report here (updated)