-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Conversation
The idea behind the current implementation was speed. |
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();
} |
@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();
} |
Alright, let's close this, then :) |
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. |
@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. |
@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. |
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. |
The only debate remaining to me is: does it prevent us to use this pattern for new features ? (For consistency reasons) |
IMO, Yes. NullLogger is not for the framework for performance reasons. |
Use
Psr\Log\NullLogger
when available in order to avoid littering code withif ($this->logger) { }
blocks.Changes were made only in component and bundles where
psr\log
is declared as a dependency incomposer.json
, or at leastsymfony/http-kernel
which requires it.If this is accepted for merging, some changes might remain to do in newest branches.