[Performance] Category manager / Tag Manager, get all counters via single query, reduce 20, 50, 100 queries (page limit) to 1 #22117

ggppdk · 2018-09-10T17:40:07Z

Pull Request for Issue # .

Summary of Changes

Category manager creates 1 query per category to get the assigned item counts by state
These are used to displays the columns
total published, total unpublished, total archived, total trashed

Testing Instructions

Open 2 Tabs of Category manager and 2 Tabs of Tag manager
Filter the 2 Tabs of Tag manager by "Tag Type" article (click search tools button)
Apply patch
Refresh 2nd TAB of Category manager and 2nd Tab of Tag manager

Both 1st and 2nd tabs should show the same counted items per state

Category managers to test

articles, banners, contact, newsfeeds, and for fieldgroups management

Tags manager:

repeat test for every listed Tag Type, in the Tag Type Filter (click search tools)

Expected result

1 query to get the total assigned items for the categories shown by current page

Actual result

20, 50, 100 queries to get the assigned items for the categories

Documentation Changes Required

None

alikon · 2018-09-10T18:06:00Z

very well done 👍
.....testing right now....

csthomas · 2018-09-10T18:10:11Z

administrator/components/com_content/helpers/content.php

+		// Get relation counts for all category objects with single query
+		$query = $db->getQuery(true)
+			->select('catid, ' . $state_column . ' AS state, count(*) AS count')
+			->from($db->qn('#__' . $related_tbl))


Replace $db->qn by $db->quoteName

use $db->quoteName() where needed

alikon · 2018-09-10T18:14:55Z

the same "optimization" should be done on every "category" banner, contact etc....

Quy · 2018-09-10T18:16:51Z

administrator/components/com_content/helpers/content.php

+
+		// Get relation counts for all category objects with single query
+		$query = $db->getQuery(true)
+			->select('catid, ' . $state_column . ' AS state, count(*) AS count')


Capitalize count(*)

alikon · 2018-09-10T18:21:57Z

even better can be done on some high level "category" CLASS/HELPER

ggppdk · 2018-09-11T18:42:20Z

even better can be done on some high level "category" CLASS/HELPER

yes that was my thought too
that is why those local variables in the first commit for the columns names, that seemed redundant ...

I intended to replace countItems() method everywhere, but i see that countTagItems() has the same problem of 20, 50, 100 queries instead of 1

so i have added a countRelations() method to parent helper class that replace both methods,
actually the methods are not replaced instead they call the parent class method like this
parent::countRelations($items, ...,$config);

alikon · 2018-09-11T18:44:49Z

countTagItems() you can accuse me for that 😨

Quy · 2018-09-11T18:50:30Z

libraries/src/Helper/ContentHelper.php

+		// The relation query does not return a value for cases without relations of a particular state / condition, set zero as default
+		foreach ($items as $item)
+		{
+			foreach($counter_names as $n)


Add space before (

Thanks, done this and other CS errors

HLeithner · 2018-09-11T19:09:16Z

We have a PR for such a function for j4 #21667 I will rework it to do the same optimization as this PR. It's really need I ask my self why I didn't do this.

Atm I wait for #20693 to get merged to get the direction how to solve the component naming issue.

ggppdk · 2018-09-11T19:20:03Z

That PR has a completely different purpose than PR #21667,
PR #21667 tries to make a reusable method in a trait

Furthermore this PR not only replaces multiple queries for category assignments with 1 query (this is the purpose of this PR)
but this PR also fixes same issue with Tag counting which PR #21667 does not even touch

A note
the code here is agnostic in regards to which state / condition exist
thus it can be rather easily ported to J4

ggppdk · 2018-09-11T19:25:51Z

Of course if you would want to do same optimization inside PR
#21667 for J4 then please do, as i lack time to do more work now
thanks

Quy · 2018-09-11T19:37:45Z

libraries/src/Helper/ContentHelper.php

+	 *
+	 * @return  stdClass[]
+	 *
+	 * @since   3.6


Change to __DEPLOY_VERSION__

thanks, lol, i remember adding DEPLOY_VERSION for 2 methods,
but then the 2 methods became 1 because 80% of their code was the same

alikon · 2018-09-11T19:52:19Z

please, can I ask you if can we concentrate our effort on 3.x series for a while !!
if we can improve performance in the current staging....it will'be a real gain
4.x ..... it will come later......

ggppdk · 2018-09-11T20:11:39Z

please, can I ask you if can we concentrate our effort on 3.x series for a while !!
if we can improve performance in the current staging....it will'be a real gain
4.x ..... it will come later......

sure, just i mention that the configuration object of the method countRelations() is structured having in mind to make easy

to be used by J3 components with custom states / custom states subsets
to be used (after ported) with a custom subset of "conditions" for J4 too

alikon · 2018-09-11T20:21:07Z

sorry for my idiosyncrasy... i just want to plant my foots on earth and not on mars

libraries/src/Helper/ContentHelper.php

laoneo · 2018-09-12T06:40:17Z

20, 50, 100 queries to get the assigned items for the categories

This function is used only in the back end category list view. As far as I can see it, it is executed only once. But please prove that I'm wrong.

ggppdk · 2018-09-12T06:52:34Z

This function is used only in the back end category list view. As far as I can see it, it is executed only once. But please prove that I'm wrong.

Sure here is the proof
please notice the existing code inside
helper classes
e.g. for categories of articles ($items contains category objects)

		foreach ($items as $item)
		{
			$item->count_trashed = 0;
			$item->count_archived = 0;
			$item->count_unpublished = 0;
			$item->count_published = 0;
			$query = $db->getQuery(true);
			$query->select('state, count(*) AS count')
				->from($db->qn('#__content'))
				->where('catid = ' . (int) $item->id)
				->group('state');
			$db->setQuery($query);
			$articles = $db->loadObjectList();
		...
		}

and then also i tested to confirm that they are executed, and then spent considerable time for this PR also revising code for tags assignments too

You can confirm the multiple queries by looking at the Debug queries slider,

and then also inside the items loop after:
$articles = $db->loadObjectList();

you can add

echo 'Executed : ' . $query . '<br><br>';

ggppdk · 2018-09-12T06:54:39Z

I speak of the multiple sql queries (20, 50, 100) that a single call to the methods will do

i never said that the method is called multiple times

laoneo · 2018-09-12T07:32:20Z

Now I got you.

laoneo · 2018-09-12T07:34:22Z

What's the idea behind the config variable?

HLeithner · 2018-09-12T08:19:37Z

ggppdk

Furthermore this PR not only replaces multiple queries for category assignments with 1 query (this is the purpose of this PR)
but this PR also fixes same issue with Tag counting which PR #21667 does not even touch

I didn't noticed that you changed the complete PR to more then changing the DB query sorry I missed this I talked about this part.

ggppdk · 2018-09-12T09:11:16Z

What's the idea behind the config variable?

basically it makes possible the method to be reusable
we now have 1 method in the parent class and 5 countItems() and 3 counTagItems() methods in descendants helpers classes call it

also copying this from my previous answer

sure, just i mention that the configuration object of the method countRelations() is structured having in mind to make easy

to be used by J3 components with custom states / custom states subsets

to be used (after ported) with a custom subset of "conditions" for J4 too

e.g. banner has different table name and different column name than content

also the counting for fields in fieldgroups does not use 'catid' column, the column name is 'group_id'

alikon · 2018-09-16T08:07:10Z

I have tested this item ✅ successfully on 0028cc8

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22117.}

Quy · 2018-09-27T11:19:14Z

I have tested this item ✅ successfully on 3d7db8c

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22117.}

Quy · 2018-09-27T11:19:57Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22117.}

csthomas · 2018-09-29T11:22:46Z

@ggppdk Can you explain why you have:

	public static function countItems(&$items, $config = null)
	{
		$config = $config ?: (object) array(
			'related_tbl'   => 'banners',
			'state_col'     => 'state',
			'group_col'     => 'catid',
			'relation_type' => 'category_or_group',
		);
		return parent::countRelations($items, $config);
	}

IMO this would be better:

	public static function countItems(&$items)
	{
		$config = (object) array(
			'related_tbl'   => 'banners',
			'state_col'     => 'state',
			'group_col'     => 'catid',
			'relation_type' => 'category_or_group',
		);
		return parent::countRelations($items, $config);
	}

If someone would like to change some parameter then should use BannersHelper::countRelations($items, $config) directly.

ggppdk · 2018-09-29T14:06:16Z

@csthomas

indeed if someone would want to count relations with a custom table / custom case,
then someone could call somenameHelper::countRelations($items, $config)

which is the method inherited by the parent class

So indeed it seems to me that it can be as you say,
we do not need $config = null variable in methods

public static function countItems(&$items, $config = null)
public static function countTagItems(&$items, $extension, $config = null)

they can be (have same signature like before this)

public static function countItems(&$items)
public static function countTagItems(&$items)

ggppdk · 2018-09-30T11:58:28Z

Please remove RTC

in last CS commit the table aliases are no longer added correctly (i think they are not, i will test)
plus i will remove the unused / redundant parameter as suggested by @csthomas

ghost · 2018-09-30T12:19:15Z

Status back on Pending as stated above.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22117.}

…nterRelations() method in ContentHelper class

ggppdk · 2018-10-12T21:42:59Z

Ok
the table aliases were correct
so just rebased, removed unused $config variable for methods (as suggested by @csthomas),
and make a CS fix required by appveyor

i retested, everything seems corrects,
but this needs again testing

csthomas · 2018-10-22T06:35:01Z

I have tested this item ✅ successfully on 3a580e4ba0f8ba273cf85fa63cfeda8ab8c30a8.

There is a problem with mark this test as success on github.
I got error "Bad credentials" at
https://issues.joomla.org/tracker/joomla-cms/22117

csthomas · 2018-10-22T06:37:32Z

I have tested this item successfully but I got error "Bad credentials" at https://issues.joomla.org/tracker/joomla-cms/22117

Quy · 2018-10-24T02:57:45Z

I have tested this item ✅ successfully on 23a580e

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22117.}

Quy · 2018-10-24T02:57:57Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22117.}

joomla-cms-bot added the PR-staging label Sep 10, 2018

csthomas reviewed Sep 10, 2018

View reviewed changes

Quy reviewed Sep 10, 2018

View reviewed changes

ggppdk force-pushed the single_counter_query branch from dae4586 to f10fb8b Compare September 11, 2018 18:36

ggppdk changed the title ~~[Performance] Category manager, get all counters via single query, reduce 20, 50, 100 queries (page limit) to 1~~ [Performance] Category manager / Tag Manager, get all counters via single query, reduce 20, 50, 100 queries (page limit) to 1 Sep 11, 2018

Quy reviewed Sep 11, 2018

View reviewed changes

csthomas reviewed Sep 11, 2018

View reviewed changes

libraries/src/Helper/ContentHelper.php Show resolved Hide resolved

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 27, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 30, 2018

ggppdk and others added 6 commits October 13, 2018 00:02

Categoy manager, get all counters via single query

ed2fce2

Replace countItems() and countTagItems() methods with a reusuable cou…

d595766

…nterRelations() method in ContentHelper class

CS

c7928a4

Since deploy version

8e0246f

Better ordering for states to property names array

a3c390e

Coding style (#6)

f6591df

ggppdk force-pushed the single_counter_query branch from 3d7db8c to f6591df Compare October 12, 2018 21:02

ggppdk added 2 commits October 13, 2018 00:10

Removed unneeded parameter from counting methods

12bac13

CS

23a580e

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 24, 2018

mbabker added this to the Joomla 3.9.1 milestone Nov 1, 2018

mbabker merged commit 8b48522 into joomla:staging Nov 1, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 1, 2018

[Performance] Category manager / Tag Manager, get all counters via single query, reduce 20, 50, 100 queries (page limit) to 1 #22117

[Performance] Category manager / Tag Manager, get all counters via single query, reduce 20, 50, 100 queries (page limit) to 1 #22117

Conversation

ggppdk commented Sep 10, 2018 • edited Loading

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

alikon commented Sep 10, 2018

csthomas Sep 10, 2018

Choose a reason for hiding this comment

alikon Sep 10, 2018

Choose a reason for hiding this comment

alikon commented Sep 10, 2018

Quy Sep 10, 2018

Choose a reason for hiding this comment

alikon commented Sep 10, 2018 • edited Loading

ggppdk commented Sep 11, 2018 • edited Loading

alikon commented Sep 11, 2018

Quy Sep 11, 2018

Choose a reason for hiding this comment

ggppdk Sep 11, 2018

Choose a reason for hiding this comment

HLeithner commented Sep 11, 2018

ggppdk commented Sep 11, 2018 • edited Loading

ggppdk commented Sep 11, 2018

Quy Sep 11, 2018

Choose a reason for hiding this comment

ggppdk Sep 11, 2018

Choose a reason for hiding this comment

alikon commented Sep 11, 2018

ggppdk commented Sep 11, 2018

alikon commented Sep 11, 2018

laoneo commented Sep 12, 2018

ggppdk commented Sep 12, 2018

ggppdk commented Sep 12, 2018

laoneo commented Sep 12, 2018

laoneo commented Sep 12, 2018

HLeithner commented Sep 12, 2018

ggppdk commented Sep 12, 2018

alikon commented Sep 16, 2018

Quy commented Sep 27, 2018

Quy commented Sep 27, 2018

csthomas commented Sep 29, 2018

ggppdk commented Sep 29, 2018

ggppdk commented Sep 30, 2018

ghost commented Sep 30, 2018

ggppdk commented Oct 12, 2018

csthomas commented Oct 22, 2018

csthomas commented Oct 22, 2018

Quy commented Oct 24, 2018

Quy commented Oct 24, 2018

ggppdk commented Sep 10, 2018 •

edited

Loading

alikon commented Sep 10, 2018 •

edited

Loading

ggppdk commented Sep 11, 2018 •

edited

Loading

ggppdk commented Sep 11, 2018 •

edited

Loading