User tests: Successful: Unsuccessful:
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
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Category | ⇒ | Tags |
Rel_Number | 0 | ⇒ | 9036 |
Relation Type | ⇒ | Pull Request for |
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:
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).
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
I added a new method getItemsTags in JHelperTags to alllow getting tags of multiple items
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 ?
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?
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.
right
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....
I have tested this item 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.
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
@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?
@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.
Status | Pending | ⇒ | Ready to Commit |
RTC. Thanks.
Labels |
Added:
?
|
Milestone |
Added: |
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-02-16 17:36:40 |
Closed_By | ⇒ | roland-d |
Labels |
Removed:
?
|
I have tested this item successfully on bde9169
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9038.