Skip to content

Use NullLogger when available #14682

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

Closed
wants to merge 1 commit into from
Closed

Use NullLogger when available #14682

wants to merge 1 commit into from

Conversation

ogizanagi
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Use Psr\Log\NullLogger when available in order to avoid littering code with if ($this->logger) { } blocks.

Changes were made only in component and bundles where psr\log is declared as a dependency in composer.json, or at least symfony/http-kernel which requires it.

If this is accepted for merging, some changes might remain to do in newest branches.

@fabpot
Copy link
Member

fabpot commented May 18, 2015

The idea behind the current implementation was speed.

@lyrixx
Copy link
Member

lyrixx commented May 18, 2015

Some insight about performance:

https://blackfire.io/profiles/compare/a5e26195-64ad-4b99-bf3d-e03a2ed4a3c0/graph

Code:

<?php

require __DIR__.'/vendor/autoload.php';

use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;

class Before
{
    public function __construct(LoggerInterface $logger = null)
    {
        $this->logger = $logger;
    }

    public function doSomething()
    {
        if ($this->logger) {
            $this->logger->debug('we are doing something.');
        }

        $a = 1;
        $b = 1;
        $c = $a + $b;

        if ($this->logger) {
            $this->logger->debug('we have done something.');
        }

        return $c;
    }
}

class After
{
    public function __construct(LoggerInterface $logger = null)
    {
        $this->logger = $logger ?: new NullLogger;
    }

    public function doSomething()
    {
        $this->logger->debug('we are doing something.');

        $a = 1;
        $b = 1;
        $c = $a + $b;

        $this->logger->debug('we have done something.');

        return $c;
    }
}

$logger = new NullLogger;

for ($i=0; $i < 1000; $i++) {
    // $s = new Before();
    $s = new After();
    $s->doSomething();
}

@jakzal
Copy link
Contributor

jakzal commented May 18, 2015

@lyrixx hehe, i've done something similar, but without creating object in a loop

<?php

require_once __DIR__.'/vendor/autoload.php';

class Foo
{
    public $logger = null;

    public function __construct($logger = null)
    {
        $this->logger = $logger;
    }

    public function call()
    {
        if (null !== $this->logger) {
            $this->logger->notice('hello');
        }
    }
}

class Bar
{
    public $logger = null;

    public function __construct($logger = null)
    {
        $this->logger = $logger ?: new \Psr\Log\NullLogger();
    }

    public function call()
    {
        $this->logger->notice('hello');
    }
}

$name = isset($argv[1]) ? $argv[1] : null;

switch ($name) {
    case 'foo':
        $foo = new Foo();
        break;
    case 'fool':
        $foo = new Foo(new \Monolog\Logger('foo'));
        break;
    case 'bar':
        $foo = new Bar();
        break;
    case 'barl':
        $foo = new Bar(new \Monolog\Logger('foo'));
        break;
    default:
        throw new \InvalidArgumentException('Accepted arguments: foo|fool|bar|barl');

}

for ($i = 0; $i < 100000; $i++) {
    $foo->call();
}

null-logger

@ogizanagi
Copy link
Contributor Author

Alright, let's close this, then :)

@javiereguiluz
Copy link
Member

Can we rethink the results provided by the benchmarks?

In the data shown by @jakzal the performance degrades 600ms when logging 100,000 times. How many log calls do you get in development env? ~200 at most? And in production? Most of the requests log nothing or less than 10 lines.

Applying the same results shown by @jakzal, logging 100 lines would add 0.6ms overhead.

@lyrixx
Copy link
Member

lyrixx commented Aug 24, 2015

@javiereguiluz Once upon a time, you shared an (issue|article|blog post) about lots of micro optimisations that make SQLite much much faster. I think here is a good example on how to optimize symfony.

And so, finally, even if I use this "hack" in my own code base, now I think it's better to keep the current code in symfony for performance reason.

@javiereguiluz
Copy link
Member

@lyrixx yes, but please let's keep in mind for the future that if we make 100,000 iterations of a new proposed feature in a benchmark, the result is probably going to be much slower.

@fabpot
Copy link
Member

fabpot commented Aug 24, 2015

I don't see why we are even talking about this topic again. That's a non issue, changing the current way does not fix anything and AFAIK, it does not bring any new features.

@ogizanagi
Copy link
Contributor Author

The only debate remaining to me is: does it prevent us to use this pattern for new features ? (For consistency reasons)
This PR was mainly motivated by #14673 (comment) when I opened it.

@nicolas-grekas
Copy link
Member

IMO, Yes. NullLogger is not for the framework for performance reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants