? ? Success

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
18 Oct 2020

Summary of Changes

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

Testing Instructions

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,

Actual result BEFORE applying this Pull Request

Debug Bar does not show logs but rendering time still huge.
At administrator/index.php?option=com_config I got around 3s

Expected result AFTER applying this Pull Request

Debug Bar does not show logs and rendering time is smaller.
At administrator/index.php?option=com_config I got around 270ms

Documentation Changes Required

none

avatar Fedik Fedik - open - 18 Oct 2020
avatar Fedik Fedik - change - 18 Oct 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Oct 2020
Category Installation Libraries Front End Plugins
1175dcd 18 Oct 2020 avatar Fedik phpcs
avatar Fedik Fedik - change - 18 Oct 2020
Labels Added: ?
e380943 18 Oct 2020 avatar Fedik phpcs
avatar HLeithner
HLeithner - comment - 18 Oct 2020

I would say we should target this for 3.10 (but didn't looked at it now)

avatar SharkyKZ
SharkyKZ - comment - 18 Oct 2020

Going forward trigger_error() should be used to trigger deprecation warnings.

avatar Fedik
Fedik - comment - 18 Oct 2020

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.

avatar SharkyKZ
SharkyKZ - comment - 18 Oct 2020

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.

avatar Fedik
Fedik - comment - 18 Oct 2020

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 😄

avatar SharkyKZ
SharkyKZ - comment - 18 Oct 2020

Screenshot_2020-10-18 Maximum function nesting level of '256' reached, aborting (500 Whoops, looks like something went wron

avatar Fedik
Fedik - comment - 18 Oct 2020

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 ;)

avatar SharkyKZ
SharkyKZ - comment - 18 Oct 2020

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.

avatar HLeithner
HLeithner - comment - 18 Oct 2020

I think that's one of the rare cases we could use Factory::getContainer()->get('application');

avatar SharkyKZ
SharkyKZ - comment - 18 Oct 2020

🤦‍♂️

avatar Fedik
Fedik - comment - 18 Oct 2020

@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

avatar Fedik
Fedik - comment - 18 Oct 2020

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.

avatar wilsonge
wilsonge - comment - 19 Oct 2020

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?

avatar Fedik
Fedik - comment - 19 Oct 2020

Has anyone actually tried figuring out what our performance issues are?

call Log::add() 3000 times, even if log_deprecated set to Off

avatar Fedik
Fedik - comment - 19 Oct 2020

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 :)

avatar joomla-cms-bot joomla-cms-bot - change - 24 Oct 2020
Category Installation Libraries Front End Plugins Administration Installation Libraries Front End Plugins
avatar Fedik
Fedik - comment - 24 Oct 2020

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

avatar Fedik
Fedik - comment - 2 Dec 2020

do we have a chance to have it in, before next beta? :)

avatar JonasAtZwetschke JonasAtZwetschke - test_item - 20 Jan 2021 - Tested successfully
avatar JonasAtZwetschke
JonasAtZwetschke - comment - 20 Jan 2021

I have tested this item successfully on 3ccf3aa


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31143.

avatar gostn gostn - test_item - 22 Jan 2021 - Tested successfully
avatar gostn
gostn - comment - 22 Jan 2021

I have tested this item successfully on 3ccf3aa


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31143.

avatar richard67 richard67 - change - 22 Jan 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 22 Jan 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31143.

avatar rdeutz
rdeutz - comment - 10 Mar 2021

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.

avatar rdeutz rdeutz - change - 10 Mar 2021
Title
[4.0] Log deprecated only when explicitly requested
[4.1] Log deprecated only when explicitly requested
avatar rdeutz rdeutz - edited - 10 Mar 2021
avatar Fedik Fedik - change - 29 Mar 2021
The description was changed
avatar Fedik Fedik - edited - 29 Mar 2021
avatar Fedik Fedik - change - 20 Aug 2021
Labels Added: ? ?
Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 20 Aug 2021
Category Installation Libraries Front End Plugins Administration Administration Libraries Front End Plugins
avatar Fedik
Fedik - comment - 20 Aug 2021

conflict fixed :)

avatar bembelimen bembelimen - change - 20 Aug 2021
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: ? ?
avatar bembelimen bembelimen - close - 20 Aug 2021
avatar bembelimen bembelimen - merge - 20 Aug 2021
avatar bembelimen
bembelimen - comment - 20 Aug 2021

Thanks

Add a Comment

Login with GitHub to post a comment