-
-
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
implement @time
and @timev
in terms of @timed
#52889
Conversation
Sounds good to me. We'd need to check that we're not hitting unexpected compilation due to this change. |
Do you know a good way to do that? I tried the example from #48024 (comment): ❯ ./julia --startup-file=no -q
julia> double(x::Real) = 2x
double (generic function with 1 method)
julia> calldouble(container) = double(container[1])
calldouble (generic function with 1 method)
julia> calldouble2(container) = calldouble(container)
calldouble2 (generic function with 1 method)
julia> @time @eval calldouble([1.0])
0.009445 seconds (2.48 k allocations: 116.594 KiB, 30.16% compilation time) # master
0.007254 seconds (2.48 k allocations: 116.594 KiB, 30.60% compilation time) # PR
2.0
julia> @time @eval calldouble2(1.0)
0.002668 seconds (3.81 k allocations: 194.422 KiB, 93.59% compilation time) # master
0.003196 seconds (3.81 k allocations: 194.422 KiB, 95.07% compilation time) # PR
2.0 which at least looks similar, though I don't know if it means anything. |
Needs news for the new fields |
do we like these field names? we could do |
|
Co-authored-by: inky <git@wo-class.cn>
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.
LGTM. Avoids duplication and the consistency in outputs seems good to me
closes #47056
I suspect there was some reason this wasn't done in the first place, but I figured opening a PR could be a way to discuss that.
Needs tests still for the new@timed
fields.