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

Use numeric level id in generated methods #54

Closed
wants to merge 5 commits into from

Conversation

sergle
Copy link
Contributor

@sergle sergle commented Jul 26, 2018

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

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
Copy link
Contributor Author

sergle commented Jul 26, 2018

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
Copy link
Contributor

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
Copy link
Member

autarch commented Jul 29, 2018

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.

@@ -129,9 +129,10 @@ sub accepted_levels {
}

sub _should_log {
my $self = shift;
my ( $self, $level, $level_id ) = @_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use multiple my $x = shift; lines.

@autarch
Copy link
Member

autarch commented Jul 29, 2018

Also thanks for working on this!

@preaction
Copy link
Contributor

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.


my $msg_level = $level_id // $self->_level_as_number($level);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'//' should be replaced for 5.8 compatibility

@sergle
Copy link
Contributor Author

sergle commented Aug 10, 2018

Is something more required from my side?

@autarch
Copy link
Member

autarch commented Aug 19, 2018

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

Thanks for working on this!

@autarch autarch closed this Aug 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants