houseabsolute / Log-Dispatch

Dispatches messages to one or more outputs
https://metacpan.org/release/Log-Dispatch/
Other
12 stars 29 forks source link

Use numeric level id in generated methods #54

Closed sergle closed 6 years ago

sergle commented 6 years ago

Added numeric log level id to arguments of generated debug(), info(),... methods. Then it used to avoid extra string-to-id lookups and validation.

sergle commented 6 years ago

We use Log::Dispatch in our application. I did the profiling by Devel::NYTProf and found that a lot of time spent in logging part, converting log-level name to number. Here I attached our changes.

Benchmark results (ubuntu 16.04 on i7-2600 CPU @ 3.40GHz)

_ Rate log log_opt
log 68923/s -- -9%
log_opt 76119/s 10% --

Benchmark script

use strict;
use Benchmark qw(cmpthese);
use Log::Dispatch;
use Log::Dispatch::Null;
my $logger = Log::Dispatch->new;
$logger->add( Log::Dispatch::Null->new(min_level => 'debug') );
cmpthese(-1, {
    log_opt => sub { $logger->debug('Test') },
    log => sub { $logger->log(level => 'debug', message => 'Test') },
});
preaction commented 6 years ago

Thanks for contributing! This looks like a good idea to me: A 10% performance improvement is great!

I'm not sure that I like the additional argument to the would_log method, especially since it's only used internally. would_log is a public method, and undocumented / private arguments to public methods sounds like a good way to create problems for users. Could we create a private method that gets called with the number?

autarch commented 6 years ago

I agree with @preaction. It's not a good design to pass private params to public methods. It's better to split the method into two, one public and one private. Other methods in the same class can call the private method with the optimized params. I think in this case you need to split both the log and the would_log methods.

autarch commented 6 years ago

Also thanks for working on this!

preaction commented 6 years ago

Okay, this is getting closer, but we can simplify the changes a bit more:

Instead of needing to copy the very similar code in both would_log (public) and _would_log (private), you should call the private one from the public one: Find the level ID inside would_log, and then use that level ID to call _would_log which does the actual work of finding an output that would log.

You can do the same with _log_with_id: The log method should call _log_with_id with the right level ID. _log_with_id probably doesn't need the level name, so you can even save some variable creation there by not passing it in.

For the _should_log and _should_log_id in Log::Dispatch::Output, no changes were necessary: _should_log is already private (it begins with a _), so adding the additional level ID argument is fine.

sergle commented 6 years ago

Is something more required from my side?

autarch commented 6 years ago

I did some additional work on this and made a new PR at #55

Thanks for working on this!