? Success
Pull Request for # 9036

User tests: Successful: Unsuccessful:

avatar shur
shur
1 Feb 2016

I discovered that Joomla generates lots of queries requesting article tags. Often tag queries are unnecessary when tags are not used at all on a certain page, but these queries are still retrieved.

Full description and how to test see here: #9036

avatar shur shur - open - 1 Feb 2016
avatar shur shur - change - 1 Feb 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Feb 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 1 Feb 2016
Category Tags
avatar brianteeman brianteeman - change - 1 Feb 2016
Rel_Number 0 9036
Relation Type Pull Request for
avatar alikon alikon - test_item - 1 Feb 2016 - Tested successfully
avatar alikon
alikon - comment - 1 Feb 2016

I have tested this item :white_check_mark: successfully on bde9169


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

avatar infograf768
infograf768 - comment - 1 Feb 2016

@shur getItemTags() is used in many places in J. Could you have a look?

avatar shur
shur - comment - 1 Feb 2016

getItemTags() is used without any conditions in generic articles model:
components/com_content/models/articles.php

getItems function from this model is often used to get a list of articles in many places such as:
category blog page and also in many content modules:

  • mod_articles_category
  • mod_articles_latest
  • mod_articles_news
  • mod_articles_popular

In my PR I suggest a test with one module on a page (mod_articles_category), but you can use any of these content modules to do the same tests. Probably, using several different modules on a page will help to see the problem better because none of these content modules display tags, but create lots of unnecessary queries to get tag information (1 per one article).

avatar joomdonation
joomdonation - comment - 1 Feb 2016

I think we can improve this PR further so that it will also improve performance if users choose to show tags in the menu parameter.

I come up with this changes staging...joomdonation:patch-4

  1. I added a new method getItemsTags in JHelperTags to alllow getting tags of multiple items

  2. In ContentModelArticles class, instead of creating a new JHelperTags object and calling getItemTags for each item, I call getItemsTags method (outside the for each loop) to get tags of all requested articles, then inside for each loop, just assigned the tags for the each article

With this change, it helps reduce the number of queries and also memory (note the tags property of $item object now is just a stdClass object instead of JHelperTags object)

Could we discuss about this additional change ?

avatar andrepereiradasilva
andrepereiradasilva - comment - 1 Feb 2016

Although i think this PR improves a lot for those not using tags, i also agree with @joomdonation i think this can be improved even for those using tags.

IMHO, for performance, anything that can be fetched outside the for cycles and in one query, should be done that way.

(note the tags property of $item object now is just a stdClass object instead of JHelperTags object)

I don't now exactly where the tags property is used, but will this part be B/C?

avatar joomdonation
joomdonation - comment - 1 Feb 2016

It is used to just get tags list like this https://github.com/joomla/joomla-cms/blob/staging/components/com_content/views/category/tmpl/blog_item.php#L29-L31

If we worry about B/C, we can still create JHelperTags object for each item, just don't call getItemTags method to get the tags data.

avatar andrepereiradasilva
andrepereiradasilva - comment - 1 Feb 2016

right

avatar alikon
alikon - comment - 1 Feb 2016

I think we can improve this PR further so that it will also improve performance if users choose to show tags in the menu parameter.

very well make another #pr i will be happy to test....

but "stay on topic" and test this simple one .... is a huge perfomance boost for those don't use tags

this could be easly committed,
your improvement maybe can take some more times to be commited....

avatar andrepereiradasilva andrepereiradasilva - test_item - 1 Feb 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 1 Feb 2016

I have tested this item :white_check_mark: successfully on bde9169

A lot less queries now.

@alikon you are right we should stay on the topic.
@joomdonation please do a PR. I will be happy to test too.


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

avatar joomdonation
joomdonation - comment - 2 Feb 2016

OK. I will make a new PR after this one is merged. Since we have two successful tests, could we get this PR merged ? It is nice to have it in Joomla 3.5.0

avatar shur
shur - comment - 2 Feb 2016

@alikon
@andrepereiradasilva
Thanks for the test

@joomdonation
I like your idea to combine all single queries into one. Though in my opinion creating another similar getItemsTags function is excessive. Probably adding an isarray check for $id parameter in the current getItemTags function will be enough?

avatar joomdonation
joomdonation - comment - 2 Feb 2016

@shur The reason I propose to make a new method because I don't like the old code, want to leave it as how it is for B/C purpose. I could not understand why the tags property of $item object (article in this case) need to be an object of JHelperTags class...

Actually, while discussing with @andrepereiradasilva about it, we thought that the getItemsTags can just be a static method.....

So after this PR is merged (it is simple and I hope it will be in 3.5.0), I will create a new PR and we can discuss about it further.

avatar infograf768 infograf768 - change - 2 Feb 2016
Status Pending Ready to Commit
avatar infograf768
infograf768 - comment - 2 Feb 2016

RTC. Thanks.


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

avatar joomla-cms-bot joomla-cms-bot - change - 2 Feb 2016
Labels Added: ?
avatar roland-d roland-d - change - 16 Feb 2016
Milestone Added:
avatar roland-d roland-d - reference | 036567e - 16 Feb 16
avatar roland-d roland-d - merge - 16 Feb 2016
avatar roland-d roland-d - close - 16 Feb 2016
avatar roland-d roland-d - change - 16 Feb 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-02-16 17:36:40
Closed_By roland-d
avatar roland-d roland-d - close - 16 Feb 2016
avatar joomla-cms-bot joomla-cms-bot - close - 16 Feb 2016
avatar joomla-cms-bot joomla-cms-bot - change - 16 Feb 2016
Labels Removed: ?
avatar shur shur - head_ref_deleted - 16 Feb 2016

Add a Comment

Login with GitHub to post a comment