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

New recompilation time report from @time isn't distinguishing "regular" compilation of the invalidating dependency. #46101

Open
NHDaly opened this issue Jul 19, 2022 · 2 comments
Labels
observability metrics, timing, understandability, reflection, logging, ...

Comments

@NHDaly
Copy link
Member

NHDaly commented Jul 19, 2022

@IanButterworth this new @time recompilation time tracking is super cool! :) 🎉

It seems like maybe there's a small bug here, where we aren't disabling is_recompile when compiling the transitive dependency that actually caused the invalidation?

Notice how in this example, both of the last two compilations of foo() are 100% of which was recompilation, whereas in the last example, actually half of its time its compiling bar(), which isn't technically recompilation, but rather regular compilation, right? At least, it's treated as regular compilation when just compiling @eval bar() directly. So i think they should be consistent, and the last one should be something like 50% recompilation!

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? Does this seem like a bug to you as well?

Originally posted by @NHDaly in #45015 (comment)

@IanButterworth
Copy link
Member

I think it would be good to get @timholy or @vtjnash's take on this. I'm not an expert on the internals here

is_recompile = jl_atomic_load_relaxed(&mi->cache) != NULL;

@NHDaly
Copy link
Member Author

NHDaly commented Jul 19, 2022

To add more clarity: My concern is that it's currently inconsistent about whether we consider the second time we compile bar() to be recompilation or not.

I think we should be consistent about how we attribute that time, independent of whether we entered it from the top level, or through an invalidation.

@NHDaly NHDaly added the observability metrics, timing, understandability, reflection, logging, ... label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
observability metrics, timing, understandability, reflection, logging, ...
Projects
None yet
Development

No branches or pull requests

2 participants