Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get tag information only when it's necessary #9038

Merged
merged 1 commit into from Feb 16, 2016
Merged

Get tag information only when it's necessary #9038

merged 1 commit into from Feb 16, 2016

Conversation

shur
Copy link
Contributor

@shur shur commented Feb 1, 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

@alikon
Copy link
Contributor

alikon commented Feb 1, 2016

I have tested this item ✅ successfully on bde9169


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

@infograf768
Copy link
Member

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

@shur
Copy link
Contributor Author

shur commented Feb 1, 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).

@joomdonation
Copy link
Contributor

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 ?

@andrepereiradasilva
Copy link
Contributor

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?

@joomdonation
Copy link
Contributor

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.

@andrepereiradasilva
Copy link
Contributor

right

@alikon
Copy link
Contributor

alikon commented Feb 1, 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....

@andrepereiradasilva
Copy link
Contributor

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.


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

@joomdonation
Copy link
Contributor

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

@shur
Copy link
Contributor Author

shur commented Feb 2, 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?

@joomdonation
Copy link
Contributor

@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.

@infograf768
Copy link
Member

RTC. Thanks.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 2, 2016
@roland-d roland-d added this to the Joomla! 3.5.0 milestone Feb 16, 2016
roland-d added a commit that referenced this pull request Feb 16, 2016
Get tag information only when it's necessary
@roland-d roland-d merged commit 036567e into joomla:staging Feb 16, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 16, 2016
@shur shur deleted the patch-13 branch February 16, 2016 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants