User tests: Successful: Unsuccessful:
Try to fix performance issue #30997.
I have added proxy method Log::deprecated()
, to use for deprecation log, which doing log only when log_deprecated
is On.
However here a little caveats:
First: The patch add Application dependency to Log class.
Second: The method may be called when Application is not initialized, then the method will do nothing, such logs may be missed in log output. This affect logs from bootup process. (Actually this is true for all bootup logs)
Use of trigger_error()
for log for deprecated messages.
@wilsonge @HLeithner please review
Apply patch.
Enable debug.
In global config enable "Log Deprecated API"
In the debug plugin disable "Log Entries"
Open any page (example /administrator/index.php?option=com_config)
check Debug Bar and note rendering time,
Then in global config disable "Log Deprecated API", refresh the same page,
check Debug Bar and note rendering time,
Debug Bar does not show logs but rendering time still huge.
At administrator/index.php?option=com_config I got around 3s
Debug Bar does not show logs and rendering time is smaller.
At administrator/index.php?option=com_config I got around 270ms
none
Status | New | ⇒ | Pending |
Category | ⇒ | Installation Libraries Front End Plugins |
Labels |
Added:
?
|
Going forward trigger_error()
should be used to trigger deprecation warnings.
Going forward
trigger_error()
should be used to trigger deprecation warnings.
I do not know what a performance of this approach.
In past I seen complains about such approach, but did not tested by myself.
It doesn't automatically solve the issue you're trying to solve. But if Joomla\CMS\Log\Log::add()
calls for deprecation logging are being replaced by anything, it should trigger_error()
. Any changes you need then should be done either in Joomla\CMS\Exception\ExceptionHandler::handleUserDeprecatedErrors()
or whatever it ends up calling.
Also configuration should be read from configuration file. If Joomla\CMS\Factory::getApplication()
or application's get()
method gets deprecated, current code will result in infinite loop.
If Joomla\CMS\Factory::getApplication() or application's get() method gets deprecated, current code will result in infinite loop.
nope
well, can be, but that problem of that guy who will make it deprecated
Yea I already understood what you meant, the same is true if "Log::add" become deprecated, even with trigger_error
.
Just ignore these edge cases ;)
It's not an edge case. You are adding a broken dependency where there should be no dependencies. As for Log::add()
getting deprecated, it's a poor example. You obviously wouldn't make Log:add()
calls inside Log::add()
itself.
I think that's one of the rare cases we could use Factory::getContainer()->get('application');
@SharkyKZ I understood what you talked about, but this does not help to fix anything
About extra dependency I have wrote in the initial comment, I did not found a better way for now.
As another idea: can try adopt Symfony ErrorHandler somehow (for catch PHP errors/deprecation), but I have no idea how it may work, and whether it will help to avoid performance issue.
With this the option log_deprecated
should modify error_reporting()
to enable E_USER_DEPRECATED
Well,forget all,
I think we can switch E_USER_DEPRECATED
on Application bootup, and then read it in Log::deprecated
I try to test how it work, and update PR some time later.
I don't think this is right. We have a category system specifically to ensure full interopability with other logging systems (also the category system on more complicated sites actually allows a lot of granularity via plugins). If we have performance issues we need to fix them - not hide them away. Has anyone actually tried figuring out what our performance issues are?
Has anyone actually tried figuring out what our performance issues are?
call Log::add()
3000 times, even if log_deprecated set to Off
but If you asking why Log::add() become so slow,
there a couple potential reasons: from use of regexp to load debug_backtrace, both is required log features :)
Category | Installation Libraries Front End Plugins | ⇒ | Administration Installation Libraries Front End Plugins |
I have changed much stuff, now it better I hope
Also since now, must be used @trigger_error('blabla message', E_USER_DEPRECATED)
for deprectated stuff. As @SharkyKZ suggested.
@HLeithner @wilsonge check check
upd: it cannot be ported to 3.10, unless this #30317 patch will be ported also
do we have a chance to have it in, before next beta? :)
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
I am moving this to 4.1, we are in beta and this is a new feature. Thank you all who have been working on this and bringing it to the state it is now. But if we want to get 4.0 out of the door we need to make a cut.
Side note: this decision has be discussed in the production department and the maintainers team. It's not just mine even if I support the decision.
Title |
|
Labels |
Added:
?
?
Removed: ? |
Category | Installation Libraries Front End Plugins Administration | ⇒ | Administration Libraries Front End Plugins |
conflict fixed :)
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-08-20 12:51:41 |
Closed_By | ⇒ | bembelimen | |
Labels |
Added:
?
?
Removed: ? ? |
Thanks
I would say we should target this for 3.10 (but didn't looked at it now)