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

New category count feature performance degrade #9420 #9421

Closed
wants to merge 4 commits into
base: staging
from

Conversation

Projects
None yet
7 participants
@alikon
Contributor

alikon commented Mar 14, 2016

Pull Request for Issue #9420 .

New category count feature performance degrade

Summary of Changes

Sliptted count queries

Testing Instructions

see #9420 .

New category count feature performance degrade #9420
New category count feature performance degrade
@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 14, 2016

Contributor

confused - what is this pr supposed to do? Remove the category count feature

Contributor

brianteeman commented Mar 14, 2016

confused - what is this pr supposed to do? Remove the category count feature

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 14, 2016

Contributor

ignore me - i guess I tested too quickly - you are still working- sorry

Contributor

brianteeman commented Mar 14, 2016

ignore me - i guess I tested too quickly - you are still working- sorry

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Mar 14, 2016

Contributor

sorry brian the connection goes down suddenly ....
can you test now

Contributor

alikon commented Mar 14, 2016

sorry brian the connection goes down suddenly ....
can you test now

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 14, 2016

Contributor

I have tested this item successfully on 7c8dafe

With my data from #9420

before

Database queries total: 31358.46 ms

after

Database queries total: 276.19 ms

WOW!!!


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

Contributor

brianteeman commented Mar 14, 2016

I have tested this item successfully on 7c8dafe

With my data from #9420

before

Database queries total: 31358.46 ms

after

Database queries total: 276.19 ms

WOW!!!


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

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Mar 14, 2016

Contributor

i'm sure we can do even better ;)

Contributor

alikon commented Mar 14, 2016

i'm sure we can do even better ;)

@Wertos

This comment has been minimized.

Show comment
Hide comment
@Wertos

Wertos Mar 14, 2016

Before

Total SQL-queries: 21420.74 ms

After

Total SQL-queries: 14.74 ms

Database: 1.4Gb
Materials: >82000
Categories: 46

Wertos commented Mar 14, 2016

Before

Total SQL-queries: 21420.74 ms

After

Total SQL-queries: 14.74 ms

Database: 1.4Gb
Materials: >82000
Categories: 46

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 14, 2016

Contributor

@alikon can you look at the codestyle issues in Travis please and then this can be merged


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

Contributor

brianteeman commented Mar 14, 2016

@alikon can you look at the codestyle issues in Travis please and then this can be merged


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

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Mar 14, 2016

This PR has received new commits.

CC: @brianteeman, @Wertos


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

joomla-cms-bot commented Mar 14, 2016

This PR has received new commits.

CC: @brianteeman, @Wertos


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 14, 2016

Contributor

RTC now that travis is happy - awesome stuff


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

Contributor

brianteeman commented Mar 14, 2016

RTC now that travis is happy - awesome stuff


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

@joomla-cms-bot joomla-cms-bot added the RTC label Mar 14, 2016

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Mar 14, 2016

Contributor

We need to do the same thing to all the other category helpers (e.g. banners https://github.com/alikon/joomla-cms/blob/patch-61/administrator/components/com_banners/helpers/banners.php#L197, newsfeeds etc.)

Contributor

wilsonge commented Mar 14, 2016

We need to do the same thing to all the other category helpers (e.g. banners https://github.com/alikon/joomla-cms/blob/patch-61/administrator/components/com_banners/helpers/banners.php#L197, newsfeeds etc.)

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Mar 14, 2016

Member
  • let's not forget hathor.
Member

infograf768 commented Mar 14, 2016

  • let's not forget hathor.
@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Mar 14, 2016

Contributor

We honestly should be doing this in view.html.php - so these variables are not set in the default.php then no Hathor changes required

Contributor

wilsonge commented Mar 14, 2016

We honestly should be doing this in view.html.php - so these variables are not set in the default.php then no Hathor changes required

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Mar 14, 2016

Contributor

@wilsonge Would be ideal in the view.html.php but then you can't override it anymore. Perhaps in a JLayout?

Contributor

roland-d commented Mar 14, 2016

@wilsonge Would be ideal in the view.html.php but then you can't override it anymore. Perhaps in a JLayout?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Mar 14, 2016

Contributor

Why should you be able to override a database query?

Contributor

wilsonge commented Mar 14, 2016

Why should you be able to override a database query?

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Mar 14, 2016

Contributor

@wilsonge because that is what is causing the performance degredation or does the sql change already account for that?

Contributor

roland-d commented Mar 14, 2016

@wilsonge because that is what is causing the performance degredation or does the sql change already account for that?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge
Contributor

wilsonge commented Mar 14, 2016

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Mar 14, 2016

Contributor

In that case the code can remain in the model or not?

Contributor

roland-d commented Mar 14, 2016

In that case the code can remain in the model or not?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Mar 14, 2016

Contributor

It could go in getItems in the model I think yes - not the getQuery where it currently resides tho

Contributor

wilsonge commented Mar 14, 2016

It could go in getItems in the model I think yes - not the getQuery where it currently resides tho

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Mar 14, 2016

Contributor

It is in the getListQuery() but yes, perhaps getItems() is better.

Contributor

roland-d commented Mar 14, 2016

It is in the getListQuery() but yes, perhaps getItems() is better.

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Mar 14, 2016

Contributor

I have written a PR that uses this but moves the code out of the com_content view and also does it across all the extensions with this feature. Closing in favour of #9429

Contributor

wilsonge commented Mar 14, 2016

I have written a PR that uses this but moves the code out of the com_content view and also does it across all the extensions with this feature. Closing in favour of #9429

@wilsonge wilsonge closed this Mar 14, 2016

@joomla-cms-bot joomla-cms-bot removed the RTC label Mar 14, 2016

@alikon alikon deleted the alikon:patch-61 branch Mar 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment