Refactor/speed up endpoint performance

[pauloiankoski]

Description

Optimize list table endpoints performance replacing expensive queries with simple ones.

Affects

• On DonationsAdminPage.php, DonorsAdminPage.php and SubscriptionsAdminPage.php there is a method getForms that was triggering a request to the ListDonationForms endpoint what needed to load the entire model. We are replacing that request with a simple DB query returning only the necessary columns.
• When calling the method count() in a query it used to just append COUNT(*) as count to the query then get that value from the results. We are now removing any other column from the SELECT statement so that the query only results the count column.
• On every list table endpoint we have a counter method that returns the total number of items based on the current arguments so that we calculate paginations. It used to make use of the prepareQuery() method but that attaches many extra meta data not necessary for the counting method. We are replacing the prepareQuery() call with a custom ModelQueryBuilder where only the necessary meta data for that endpoint is now being attached.

Visuals

Database containing 3440 donations.

Before

After

Pre-review Checklist

• Acceptance criteria satisfied and marked in related issue
• Relevant @unreleased tags included in DocBlocks
• Includes unit tests
• Reviewed by the designer (if follows a design)
• Self Review of code and UX completed

[jonwaldstein]

@pauloiankoski nice optimization, I just pointed out places that you don't need to use the ModelQueryBuilder and instead should just use QueryBuilder or DB::table(). The ModelQueryBuilder's main purpose is to return a model instance when you call get() or getAll(). Since your doing manual queries and calling count() in the end, there would be no reason to use the ModelQueryBuilder.

[jonwaldstein]

Great job man!

[JasonTheAdams]

@jonwaldstein @pauloiankoski I'll be honest, this PR actually makes me a little nervous. The point of making the Model-oriented list tables was to further decouple (albeit not 100%) the list tables from the database structure. The bit in here for clearing the selects on count() is fine, but in other places we're coupling ourselves right back to the database.

[jonwaldstein]

@JasonTheAdams from what I understand, the main thing is not using the models for getting total counts specifically. I'm fine with that optimization as we don't need to join a bunch of tables just to get the total records number. Maybe some of these simple queries can just be moved into the repository itself so they're not called directly in the list tables.

[JasonTheAdams]

@jonwaldstein @pauloiankoski Did you explore using SQL_CALC_FOUND_ROWS and FOUND_ROWS() so we don't have to reconstruct the query for the count? It seems like the count is just being used to calculate the total rows for the query?

[pauloiankoski]

@JasonTheAdams According to the most recent docs, I believe we are in line with the most recent recommendations:

The SQL_CALC_FOUND_ROWS query modifier and accompanying FOUND_ROWS() function are deprecated as of MySQL 8.0.17; expect them to be removed in a future version of MySQL. As a replacement, considering executing your query with LIMIT, and then a second query with COUNT(*) and without LIMIT to determine whether there are additional rows.

@jonwaldstein At first, I had this idea of adding a parameter to the prepareQuery method, something like $columnsForAttachedMetaQuery where we could modify the metadata being attached. The selects would be removed on the count() method as they already are now.

[jonwaldstein]

@pauloiankoski thanks, we wouldn't want to do that though because those prepareQuery methods on the repository are meant to prepare the query to be used specifically as a model and not be overwritten for any other reason.

However I was thinking we could just have some simple methods on the repositories that do counts like:
give()->donationForms->getTotalCount($query) or even another option I just thought of, is we could have a modified count() method in the ModelQueryBuilder that knows how to strip out everything it doesn't need to run a simple count statement.

[pauloiankoski]

@jonwaldstein As I have to check some conditions related to the filters we have on the tables, I'd need to have an instance of QueryBuilder back in that method we are using to count.

Detach the meta from prepareQuery would be an option but IHMO, it's a bigger effort undo things than to create a new query. Then, the other solution that came up to me was to have a initQuery method that does just the basic: start the queryBuilder object and set the minimum requirements for that domain, like table and post_type where necessary. The prepareQuery would use that method as the starter point to add the other clauses like select and attachMeta.

I'd be able to use that function for the counting. It's an empty query builder where I can easily run the count method. PS: As a good developer, naming functions is not one of my strengths.

I would love to also hear your thoughts @JasonTheAdams.

[JasonTheAdams]

@pauloiankoski Out of curiosity, for the count query, if all we do is remove all other selects upon count() and just stick with the exact same query as it otherwise being used, where does that put us in terms of time? I know the joins do slow things down, but it seemed like the major time consumer was the what-having stuff we found yesterday.

[pauloiankoski]

@JasonTheAdams After a long battle against the system over the last couple of days, I was finally able to run the tests.

I ran the queries 3 times for each amount and tabulated the median value (Y axis in log scale):

This is not science but I do believe the results are meaningful. What I can read from the results are:

• Running on the local environment is much faster than on any server so exact numbers should not be considered but what they represent to the whole;
• Using having on the main query reduces the query time by 60% on average;
• The main query as limiting results to 30 took a stable amount of time for all queries;
• The counting query consistently doubles as the number of rows in the database also doubles. I can see a 1:1 ratio there;
• Restricting the amount of left join on the counting query reduces the query time by 75% on average;
• Using having on the counting query slightly increases the query time;