Introduce 'bump' functions to replace costly recounts

[johnjamesjacoby]

Right now when a new topic or reply is created, we run a bunch of queries to recalculate the topic and reply counts for topics and forums. It'll be more efficient to get the current value and just bump it based on the action (new topic-reply/split-topic/move-topic/spam-trash-topic/etc...)

[johnjamesjacoby]

(In [3826]) Add 'bump' functions for forum, topic, reply, and voice counts.

• Functions not used yet
• See #1799

[johnjamesjacoby]

Punt to 2.2.

[johnjamesjacoby]

Current approach, in my imagination:

• All _update_ functions should be split into two.
• _update_ functions become wrappers for meta updates.
• New _recalculate_ functions to actually get new information.
• Leverage post meta to write more efficient meta-queries, to avoid using nested custom database queries for child post ID's.
• Yes, meta-queries are more performant than the current implementation. Need to explain queries and post some benchmarks to confirm.

[johnjamesjacoby]

Functions are in, they just aren't being utilized. Punting to Future Release.

[johnjamesjacoby]

Replace direct count queries with WP_Query() objects

[MZAWeb]

I implemented the bumps for user's counts in #1694

[johnjamesjacoby]

(In [4610]) Clean up phpdoc for bbp_get_total_users(). See #1799.

[johnjamesjacoby]

(In [4611]) Add some brackets to improve readability in /topics/functions.php. See #1799.

[johnjamesjacoby]

Refresh

[johnjamesjacoby]

(In [4654]) Update bump count functions to use integer values. See #1799.

[johnjamesjacoby]

Punting to 2.4.

[johnjamesjacoby]

No time. Moving to 2.5.

[johnjamesjacoby]

Bump to 2.6.

[netweb]

In ​1799.3.patch refreshed patch only against new /trunk

[netweb]

Related #2353 (I just closed this a duplicate of this ticket)

• @vibol's bbp_update_forum_reply_count_query_log.txt​ is a helpful resource
• @netweb's alternate query ticket:2353#comment:5

Most likely my alt query is irrelevant based on the updated query in 1799.3.patch, untested at this stage)

[thebrandonallen]

First pass at implementing bump functions for topics/replies.

• Introduces increase/decrease functions similar to the user bump functions
• Moves bumps to related actions
• Works with hamming/spamming/trashing/untrashing, while preserving delete actions, and work in the dashboard

On a, mostly, default VVV install (opcache disabled), with 20 forums, ~100,000 topics, and ~1,200,000 replies, this patch, alone, reduced new reply time to 1/4 to 1/3 (an average 3/10) that of an unpatched bbPress. It should also be noted that this was before the recent memory bump to 1GB (​https://github.com/Varying-Vagrant-Vagrants/VVV/pull/439).

Average new reply post time without the patch was ~18.54 seconds. With the patch it was ~5.39 seconds. New topics, and spamming/unspamming/trashing/untrashing both topics/replies saw similar results. New reply tests on both patched/unpatched bbPress were run twice to prime the pumps, then post times were recorded 50 times each before being averaged.

I've tried to catch every scenario, but this definitely needs eyes and plenty of testing. I'd be interested to see how this patch compares to the tweaked queries sc0ttclark is running (​#bbpress-dev IRC).

[netweb]

Which patch did you test? and/or Did you mean to upload a patch and it vanished?

[thebrandonallen]

What was wrong with me yesterday? Neither would be correct. I didn't upload a patch at all ('_')

[thebrandonallen]

Refresh for current trunk

[thebrandonallen]

Refreshing patch for current trunk, so it applies cleanly.

[netweb]

Patch ​1799.6.diff is a refresh of 1799.5.diff with added unit tests for proposed new functions.

ToDo: Just need to create some tests now that actually test this new proposed functionality.

[thebrandonallen]

Been sitting on this for a while, and for no good reason. Here are the topic voice count bumping functions. The tools portion of patch works, but there's probably room for improvement.

[netweb]

In 5953:

Tests: More tests for the common component query functions:

• test_bbp_query_post_parent__in() for bbp_query_post_parent__in()
• test_bbp_get_public_child_last_id() for bbp_get_public_child_last_id()

See #1799

[netweb]

In 5954:

Performance: Replace direct SQL queries with WP_Query() objects in the following functions:

• bbp_update_forum_topic_count()
• bbp_update_forum_topic_count_hidden()
• bbp_update_forum_reply_count()
• bbp_get_public_child_last_id()
• bbp_get_public_child_count()
• bbp_get_public_child_ids()

Props johnjamesjacoby.
See #1799.

[eddr]

BTW, it could be efficient to recalculate for a bunch of topics/replies every - say - 15 seconds, for very active forums.. no?

[netweb]

In 5955:

Tests: Fix failing BBP_Tests_Topics_Functions_Update_Topic_Last_Thing tests

• Pass the active id as the 2nd parameter to bbp_update_topic_last_active_id() in test_bbp_update_topic_last_active_id()
• Pass the reply id as the 2nd parameter to bbp_update_topic_last_reply_id() in test_bbp_update_topic_last_reply_id()

This fixes the current two failing unit tests by passing what should be optional paremeters to their respceptive functions.

See #1799, [5954]

[netweb]

Per the original patch proposals (1799.3.patch​, 1799.2.patch​, and 1799.patch​) was to also replace direct MySQL queries in bbp_get_all_child_ids() with WP_Query() objects as per [5954].

Currently this cannot be achieved because WP_Query() ignores bbPress spam custom post status (possibly others)

See: ​WP_Query#Status_Parameters and ​wordpress.org/support/topic/wp_query

Follow up ticket: #2894 Replace direct SQL query in bbp_get_all_child_ids() with WP_Query() object

[tharsheblows]

I am currently running the non unit-test bits of 1799.6.diff and [5954] on a production site and they seem to be working quite well. (The reason I'm not using the voice bumping functions is the theme doesn't use voice counts.)

This is somewhat related: #2915 bbp_forum_query_last_reply_id and bbp_update_forum_reply_count queries too long

[thebrandonallen]

Refreshed patch for latest trunk, along with an extra fix or two. Voice counts are not included in this patch, and fixes for are coding standards needed.

[thebrandonallen]

no voice counts

[thebrandonallen]

Tests fixes, coding standards, no voice counts

[thebrandonallen]

New patch 1799.9.patch​. Removes WP < 4.0 compat code from the tests, since we now require 4.0+. Fixes a few coding standards/PHPDoc issues.

[netweb]

In 6006:

Tests: Include forum category tests in bbp_*_forum_*_count() template functions.

This changeset improves forum template count tests adding category total topic and total reply counts which are calculated differently to forum topic and reply counts.

See #1799

[netweb]

The 1799.10.patch​ is just a refresh of 1799.9.patch​ against /trunk's latest commits.

Will commit this tomorrow, ran out of time today.

[netweb]

Latest patch with PHPDoc tweaks

[netweb]

In 6036:

General Performance: Introduce increase/decrease helper count functions

Previously when a new topic or reply was created, a bunch of queries to recalculate the topic and reply counts for topics and forums were ran. Now these have been replaced with more efficient increase/decrease helper functions to get the current value and just "bump" the count based on the action (new topic-reply/split-topic/move-topic/spam-trash-topic/etc...)

Props thebrandonallen, tharsheblows, netweb
See #1799

[netweb]

In 6037:

Replies: Fix @param PHPDoc in bbp_insert_topic_update_counts()

See #1799

[thebrandonallen]

Here's a new patch for the voice counts. Updated for the latest trunk, and includes a few extra fixes.

• Changes calls to get_post_field( 'post_author' to their respective author id getters.
• Use a strict array_search when unsetting a voice id from the voice ids array.
• Use absint instead of intval when validating voice ids pulled from the database during calls to bbp_update_topic_voice_count().

[thebrandonallen]

Updating patch to add tests for bbp_insert_topic and bbp_insert_reply.

[thebrandonallen]

Attached patch changes the approach to caching voice ids in meta. It now follows the method used to store favorites and subscriptions, where each voice id is it's own entry in post meta.

Voice counts are currently being upgraded alongside favorites and subscriptions. However, I'm not sure using the large install metric (based on users) applies as well here, as it does with favorites and subscriptions.

As it stands, my test install (~1.3 million topics|replies and ~1,000 users) takes ~2.5-3.5 minutes to perform a voice recount/upgrade. Upgrades trend more towards the lower end since there's no _bbp_voice_id rows to delete.

Posting times are still greatly improved, even after [6036].

[johnjamesjacoby]

Awesome!

[johnjamesjacoby]

In 6310:

Voices: Only update voice counts when saving or deleting.

Prevents overzealous recalculations of voice counts when it's not necessary or applicable.

Props thebrandonallen. See #1799.

[johnjamesjacoby]

Let's complete the rest of this in 2.7.

[johnjamesjacoby]

I just figured out how we need to do this:

• Create subactions for when a post goes "public" or "private" according to bbPress's specifications
• Hook subactions onto all of the necessary post_status transition actions
• Create meta-bumper function, to handle the necessary calculations for every meta type & key
• Wrap meta-bumper inside existing functions
• Possibly deprecate duplicates or retired _maybe_bump_ functions
• 1 extra walker function that checks post types up the post_parent tree, and bumps accordingly
• Remove all places where counts are manually updated
• Test topic splits & merges to avoid chaos

I'm not sure why, but #3086 just sparked all of this.

[thebrandonallen]

This was more or less what I was proposing in Slack the other day. You've fleshed it out more than I had at the time.

I'm nearly finished working on a patch for storing deleted post data, which will end up sharing functionality with the public/private transition functionality that will help us get bump counts in order. I'll try to complete it all this weekend and get the patch posted.

[johnjamesjacoby]

This was more or less what I was proposing in Slack the other day. I'll try to complete it all this weekend and get the patch posted.

Awesome!

[thebrandonallen]

I had less time than I had planned, and last week I took some time off. On top of that, this patch was full of twists and turns. However, the end result is that post counts a won't get out of sync when moving a post from one moderated status to another, or one public status to another. Additionally, counts are now bumped rather than recalculated when in the admin.

Attached patch does the following (with tests for everything):

1. Adds forum/topic/reply sub-actions for transition_post_status
2. Attached to these sub-actions are checks to see how the post is being transitioned, adding relevant actions when applicable. The post id and it's transition type are also stored in the bbPress::transitioned_posts array for later use.
3. bbp_transitioned_* functions are then hooked onto our late stage action hooks (bbp_new_reply, bbp_trashed_reply, etc.). These hooks are needed because transition_post_status is called before any post meta is added for new forums/topic/replies.
4. The bbp_transitioned_* functions then check that the transitioned post matches the transition type they are delegated to check. If the check passes, they will fire their action.
5. Count bumping functions are then hooked onto the bbp_transitioned_* actions with a priority of 20 to allow room for bbp_update_*() and bbp_update_*_walker() functions to be run.

This patch removes the need for bbp_insert_topic_update_counts and bbp_insert_reply_update_counts, but for now, I just marked them as deprecated in the documentation, with an @todo so we don't forget about them.

Note: For feature parity, all of this logic is applied to forums, even though none of the functionality is being used, currently.

[johnjamesjacoby]

.16 is an evolution of @thebrandonallen's idea with .15

• Forum statuses are screwed. I named the "Lock" functionality so wrong, and now it's biting us. I tried to rename them and deprecate function names a bit, but it's not awesome.
• I tried to DRY some things a bit, and moved things into handler & helper functions where I could
• New status.php files to handle the transition actions for forums, topics, replies
• Updated the tests to use my changed logic
• Added a legitimate open status with bbp_get_open_status_id() function, etc...

One thing I tried, and noticed still didn't work, was moving a topic via wp-admin.

• Create a forum
• Create a topic in that forum (notice the forum topic count is 1)
• Edit the topic, and set it to "No Forum" and save
• The forum still shows a topic count of 1

I think my patch still isn't perfect, and not ready for core commit, but it's on its way to being so.

I also could see having a common bbp_get_post_statuses_by_type( 'forum', 'public' ) function, which might help alleviate some of the insanity.

[johnjamesjacoby]

Moving to 2.6.

See #3132, #3135.

[johnjamesjacoby]

In 6790:

Admin: improve topic/reply row-action UX.

This change includes a few improvements to how topics and replies are toggled from an admin area list-table:

• Make approve & not-spam links green to match comments UI
• More specifically target admin area links to avoid conflicts with other post types
• Make "Approve" a blanket "publish" action, meaning it will always publish a topic/reply even from spam or trash
• Make sure "Not Spam" will restore to "pending" if it was previously not published
• Bump CSS version

See #1799.

[johnjamesjacoby]

In 6791:

Admin: improve topic/reply row-action UX.

This change includes more improvements to how topics and replies are toggled from an admin area list-table:

• Move "Empty Spam" buttons to their own actions div
• Allow spam/trash/approve/unapprove from any other status, so topics/replies can be more freely moved around
• Add public/non-public functions for replies to match topics

See #1799.

[johnjamesjacoby]

In 6792:

Counts: introduce sub-action for post status transitions.

We'll use this for micro-managing child post counts on a per-type, per-status basis.

See #1799.

[johnjamesjacoby]

Let's worry about this after 2.6.

[johnjamesjacoby]

In 6923:

Counts (meta-data): full audit of forum/topic/reply, public/non-public counts.

This commit is the result of a full count audit, exposing multiple inconsistencies and voids in relation to how public and non-public counts are (re)calculated.

For instance, hidden forum replies are not counted at all, until now. By introducing a new Repair tool, hidden forum reply counts are now counted.

In addition, there were multiple bugs with topic & reply moderation, where the act of approving or unapproving topics or replies would cause the numbers to be inaccurate, or where topics & replies being caught in moderation were still increasing public counts.

It was also possible to, as a Key Master, publicly reply to unapproved topics, which was a completely unanticipated side-effect of allowing Key Masters to do pretty much anything. Going forward, the default reply status is the topic status, but is still beholden to all existing moderation settings and user role capabilities. This results in a more sane user experience, and prevents the unusual circumstance of there being "0 topics and 30 replies" in public-facing forums.

Certain count increase/decrease actions have been reprioritized to avoid collisions and race conditions, proving once again that ya gotta get up to get down.

See #2838. Fixes #1799.