Improve performance of logging macros #28289
Conversation
base/logging.jl
Outdated
| group = if isdefined(Base, :basename) && isa(file, String) | ||
| QuoteNode(splitext(basename(something(file, "")))[1]) | ||
| else | ||
| :(Symbol(splitext(basename(something($file, "")))[1])) |
There was a problem hiding this comment.
maybe memoize this in addition or instead?
Ref = Ref{Symbol}()
:(isassigned($Ref) ? $Ref[] : ($Ref[] = ...))
base/logging.jl
Outdated
| @@ -369,9 +375,10 @@ function current_logstate() | |||
| end | |||
|
|
|||
| # helper function to get the current logger, if enabled for the specified message type | |||
| @noinline function current_logger_for_env(std_level::LogLevel, group, _module) | |||
| function current_logger_for_env(std_level::LogLevel, group, _module) | |||
There was a problem hiding this comment.
We don't want this to inline. It'll bloat code everywhere, but won't improve performance
There was a problem hiding this comment.
Tested with BenchmarkTools, removing @noinline is faster, 75 vs 80 ns.
Isn't the main bloat in env_override_minlevel anyway?
There was a problem hiding this comment.
Of course it'll look faster on a synthetic benchmark – it's doing strictly less work, assuming all of your code fits in the processor cache. I'm surprised the hasenv check here makes a difference though. That's essentially all that env_override_minlevel is doing, maybe it should have an explicit isempty(debug) && return, in case codegen isn't able to handle our x == "" call?
There was a problem hiding this comment.
I tested with a slightly less synthetic benchmark, but OK I get your point.
I had assumed a difference between haskey and getindex; I'll have a closer look.
base/logging.jl
Outdated
| if group == nothing | ||
| group = if isdefined(Base, :basename) && isa(file, String) | ||
| # precompute if we can | ||
| QuoteNode(splitext(basename(something(file, "")))[1]) |
There was a problem hiding this comment.
file is a String, the something call is unnecessary
| # memoized run-time execution | ||
| ref = Ref{Symbol}() | ||
| :(isassigned($ref) ? $ref[] | ||
| : $ref[] = Symbol(splitext(basename(something($file, "")))[1])) |
There was a problem hiding this comment.
need parens around the assignment (currently, this is (a ? b : c) = d)
There was a problem hiding this comment.
nevermind, I forgot the precedence here are special:
julia> :(w = x ? y = 1 : z = 2)
:(w = if x
y = 1
else
z = 2
end)
|
Addressed review comments: added memoization and removed/simplified questionable improvements. |
Improve #28147