Significant performance improvements

[pupi1985]

I finally found some time to work on what was detailed in #760. I specifically wanted to focus on the third conclusion in there: don't recalculate the caches each time.

I spent a lot of time decoding what conditions fired the recalculation and how to prevent it. In many scenarios, recalculation could be avoided completely, in some others recalculation was missing (which is a bug) and in all scenarios there was a great performance boost.

The test site had this structure:

• Questions: 1,534,171
• Answers: 513,495
• Comments: 66,107
• Questions hidden: 438,250
• Answers hidden: 12
• Comments hidden: 0
• Questions, answers and comments queued: 0
• Notes: 353

MySQL query cache was disabled (although it didn't change much when it was enabled, anyway).

Here is the detail of the time spent for some operations (numbers below are measured in seconds and milliseconds are included):

Current Q2A version

Closing a question: 0.115 (Caches were not being updated here because of a bug!) Opening a question: 0.101 (Caches were not being updated here because of a bug!)

Hiding a question: 27.690 Reshowing a question: 28.030

Hiding a answer: 15.850 Reshowing a answer: 16.090

Give vote on answer: 7.640

Select answer: 8.160 Deselect answer: 8.200

Some of the operations are so slow that they risk an execution timeout

Current Q2A version + extra indexes shown in referenced post

Closing a question: 0.113 (Caches were not being updated here because of a bug!) Opening a question: 0.105 (Caches were not being updated here because of a bug!)

Hiding a question: 1.84 Reshowing a question: 1.87

Hiding a answer: 0.945 Reshowing a answer: 0.941

Give vote on answer: 0.490

Select answer: 0.529 Deselect answer: 0.489

Time decreased in ~90% compared to current Q2A version

This branch (no indexes created)

Closing a question: 0.091 Opening a question: 0.087

Hiding a question: 0.104 Reshowing a question: 0.110

Hiding an answer: 0.117 Reshowing an answer: 0.112

Give vote on answer: 0.109

Select answer: 0.108 Deselect answer: 0.100

Time decreased in ~98% compared to current Q2A version

As I made several changes I wanted to make sure I wasn't changing the original logic (except for the bugfix). So I created a lot of tests to check the changes resulted in the same logic as before. I also had to create a way to execute database dependent tests as there was none.

[svivian]

Thanks for your hard work, looks great! I'll take a closer look when I get a moment.

[svivian]

Which version of MySQL are you using? I've found that CAST(content AS INT) (in db/options.php) doesn't work, it should be CAST(content AS UNSIGNED). Just wondering if it's different for different versions.

[pupi1985]

I'm using MariaDB and it works fine there. That must be the issue

[svivian]

OK, looks like 'unsigned' works in MariaDB so I've changed that.

Next thing... the database tests have a serious flaw, the phpunit config file is ignored (or overridden?) and it uses the main qa-config.php file, wiping the database. Luckily it was only my test site (and I had a backup). Checking into it now.

Edit: I don't think the phpunit config was ever used when running the tests locally. It was only used when running Travis CI as a stand-in so it could be included (but the values never used). I can include phpunit-qa-config.php from the test autoloader, but that gives warnings since Q2A still tries to include the normal qa-config.php and redefine the constants.

[pupi1985]

Just wanted to check if you have followed the updated steps in the README.md file

[svivian]

Pretty sure I did, but I’ll double check in case I missed something. But I didn’t see any reference to the phpunit-qa-config.php file anywhere (i.e. it’s not included at any point)

On Tue, 11 Jul 2023 at 21:56, Gabriel Zanetti @.***> wrote:

Just wanted to check if you have followed the updated steps in the README.md file

— Reply to this email directly, view it on GitHub https://github.com/q2a/question2answer/pull/910#issuecomment-1631497953, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABSYD2UGARBHLBQ3LWES5LXPW4YFANCNFSM5CYIG26A . You are receiving this because you commented.Message ID: @.***>

[pupi1985]

Oh, I've just run a quick search on my local environment and found a tests.sh file:

#!/bin/bash
mv qa-config.php qa-config.php.bak
cp qa-tests/phpunit-qa-config.php qa-config.php

echo "Running phpunit..."
echo
echo phpunit --bootstrap qa-tests/autoload.php qa-tests
echo
phpunit --bootstrap qa-tests/autoload.php qa-tests

rm qa-config.php
mv qa-config.php.bak qa-config.php

I think this answers the question :man_facepalming:

[svivian]

That would make sense :) I’ll look at replicating that in the phpunit setup, as I can’t see a way around the issue of redefining constants.

[svivian]

OK I finally found a solution (see c41f2cd54fe08166bd36f02e6523262550d0d7c0). It was a bit complicated as we need to change the config file at the very beginning and restore at the very end (can't do it using setUpBeforeClass as Q2A is already loaded by this point). Anyway it's possible using register_shutdown_function.

I also fixed a couple of issues along the way. Even when we use the database we need to disable auto-connect otherwise it would try to read options before setting up the test tables. Also when recreating tables it deleted ALL tables in the database - I've changed it to delete only the tables from the test instance (e.g. delete qa_test_* but not qa_*).

I have to say, besides those setup issues you've done a fantastic job with the tests themselves - they're detailed and thorough. And of course the original performance issue itself. Thanks for all the hard work!

[pupi1985]

Thank you, Scott. I haven't had the chance to test those tweaks you've made but they look good.

For the future, and to simplify testing (in this case copying files for tests to run properly), the constants shouldn't exist as such, but rather as a config array that can be set during the bootstrap stage of Q2A or the tests. So making Q2A behave in one way or another, should be as simple as setting a different value in the config array.

[svivian]

the constants shouldn't exist as such, but rather as a config array

Yes I’ve thought about this a bit. We should probably get rid of all the constants and move them to a single config object that can be passed around/retrieved when needed. Or modified for testing.

[svivian]

I've run into some issues since merging this into dev. I first noticed that a lot of actions like posting answers/comment and voting were increasing the cached users count.

Running the tests still works perfectly fine on the bugfix branch, but on dev I get some errors like these:

1) DbCachedOptionsTest::test__qa_answer_set_status_normal_to_hidden_two_answers_with_selected_and_upvoted_answer Failed asserting that 12 is identical to 13.

2) DbCachedOptionsTest::test__qa_answer_set_status_normal_to_hidden_two_answers_with_two_upvoted_answers Failed asserting that 13 is identical to 12.

So somewhere along the way the merge upset something. I've taken a look through the code to see what's different between bugfix and dev - so far all I see is a few calls to qa_db_hiddencount_update (added a while back), and removing those doesn't fix the issues. If you have some time would you be able to take a look?

[svivian]

OK managed to find the issue, it was due to the qa_db_affected_rows function not working with the new database system. I replaced all those calls and got various functions to return the number of rows affected. All seems to be working great now!