Skip to:
Content

Opened 7 years ago

Last modified 10 months ago

#1799 new defect

Introduce 'bump' functions to replace costly recounts

Reported by: johnjamesjacoby Owned by: johnjamesjacoby
Milestone: 2.6.1 Priority: highest
Severity: major Version: 2.0
Component: General - Performance Keywords: has-patch needs-testing needs-unit-tests
Cc: vpundir@…, pippin@…, stephen@…, nashwan.doaqan@…

Description

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

Attachments (16)

1799.patch (18.9 KB) - added by johnjamesjacoby 6 years ago.
Replace direct count queries with WP_Query() objects
1799.2.patch (15.4 KB) - added by johnjamesjacoby 6 years ago.
Refresh
1799.3.patch (18.2 KB) - added by netweb 5 years ago.
1799.4.patch (12.2 KB) - added by thebrandonallen 4 years ago.
1799.5.patch (13.0 KB) - added by thebrandonallen 4 years ago.
Refresh for current trunk
1799.6.diff (18.7 KB) - added by netweb 4 years ago.
1799.7.patch (22.6 KB) - added by thebrandonallen 3 years ago.
1799.8.patch (35.5 KB) - added by thebrandonallen 3 years ago.
no voice counts
1799.9.patch (35.7 KB) - added by thebrandonallen 3 years ago.
Tests fixes, coding standards, no voice counts
1799.10.patch (33.5 KB) - added by netweb 3 years ago.
1799.11.diff (36.3 KB) - added by netweb 3 years ago.
Latest patch with PHPDoc tweaks
1799.12.voice_counts.patch (22.7 KB) - added by thebrandonallen 3 years ago.
1799.13.voice_counts.patch (24.2 KB) - added by thebrandonallen 3 years ago.
1799.14.voice_counts.diff (14.8 KB) - added by thebrandonallen 2 years ago.
1799.15.diff (88.5 KB) - added by thebrandonallen 2 years ago.
1799.16.diff (96.6 KB) - added by johnjamesjacoby 23 months ago.

Download all attachments as: .zip

Change History (80)

#1 @johnjamesjacoby
7 years ago

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

  • Functions not used yet
  • See #1799

#2 @johnjamesjacoby
7 years ago

  • Milestone changed from 2.1 to 2.2
  • Owner set to johnjamesjacoby

Punt to 2.2.

#3 @johnjamesjacoby
7 years ago

  • Priority changed from normal to highest
  • Severity changed from normal to major

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.

#4 @sooskriszta
6 years ago

  • Cc vpundir@… added

#5 @johnjamesjacoby
6 years ago

  • Milestone changed from 2.2 to Future Release

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

@johnjamesjacoby
6 years ago

Replace direct count queries with WP_Query() objects

#6 @johnjamesjacoby
6 years ago

  • Milestone changed from Future Release to 2.3

#7 @MZAWeb
6 years ago

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

#8 @mordauk
6 years ago

  • Cc pippin@… added

#9 @johnjamesjacoby
6 years ago

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

#10 @johnjamesjacoby
6 years ago

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

@johnjamesjacoby
6 years ago

Refresh

#11 @netweb
6 years ago

  • Cc stephen@… added

#12 @johnjamesjacoby
6 years ago

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

#13 @johnjamesjacoby
6 years ago

  • Milestone changed from 2.3 to 2.4

Punting to 2.4.

#14 @johnjamesjacoby
6 years ago

  • Milestone changed from 2.4 to 2.5

No time. Moving to 2.5.

#15 @johnjamesjacoby
5 years ago

  • Milestone changed from 2.5 to 2.6

Bump to 2.6.

#16 @johnjamesjacoby
5 years ago

  • Component changed from Back-end to Performance Improvements

This ticket was mentioned in IRC in #bbpress-dev by netweb. View the logs.


5 years ago

@netweb
5 years ago

#18 @netweb
5 years ago

In 1799.3.patch refreshed patch only against new /trunk

#19 @netweb
5 years ago

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)

This ticket was mentioned in IRC in #bbpress-dev by sc0ttkclark. View the logs.


5 years ago

#21 @alex-ye
5 years ago

  • Cc nashwan.doaqan@… added

#22 @thebrandonallen
4 years ago

  • Keywords has-patch needs-testing added

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

#23 @netweb
4 years ago

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

#24 @thebrandonallen
4 years ago

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

This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.


4 years ago

@thebrandonallen
4 years ago

Refresh for current trunk

#26 @thebrandonallen
4 years ago

Refreshing patch for current trunk, so it applies cleanly.

@netweb
4 years ago

#27 @netweb
4 years ago

  • Keywords needs-unit-tests added

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.

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


4 years ago

This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.


4 years ago

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


4 years ago

This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.


4 years ago

#32 @thebrandonallen
3 years ago

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.

#33 @netweb
3 years ago

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

#34 @netweb
3 years ago

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.

#35 @eddr
3 years ago

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

#36 @netweb
3 years ago

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]

#37 @netweb
3 years ago

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

This ticket was mentioned in Slack in #bbpress by tharsheblows. View the logs.


3 years ago

#39 @tharsheblows
3 years ago

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

This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.


3 years ago

#41 @thebrandonallen
3 years ago

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
3 years ago

no voice counts

@thebrandonallen
3 years ago

Tests fixes, coding standards, no voice counts

#42 @thebrandonallen
3 years ago

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.

#43 @netweb
3 years ago

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
3 years ago

#44 @netweb
3 years ago

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
3 years ago

Latest patch with PHPDoc tweaks

#45 @netweb
3 years ago

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

#46 @netweb
3 years ago

In 6037:

Replies: Fix @param PHPDoc in bbp_insert_topic_update_counts()

See #1799

#47 @thebrandonallen
3 years ago

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().

#48 @thebrandonallen
3 years ago

Updating patch to add tests for bbp_insert_topic and bbp_insert_reply.

#49 @thebrandonallen
2 years ago

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

#51 @johnjamesjacoby
2 years ago

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.

#52 @johnjamesjacoby
2 years ago

  • Milestone changed from 2.6 to 2.7

Let's complete the rest of this in 2.7.

#53 follow-up: @johnjamesjacoby
2 years ago

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.

Last edited 2 years ago by johnjamesjacoby (previous) (diff)

#54 in reply to: ↑ 53 @thebrandonallen
2 years ago

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.

#55 @johnjamesjacoby
2 years ago

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!

#56 @thebrandonallen
2 years ago

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.

This ticket was mentioned in Slack in #bbpress by jjj. View the logs.


23 months ago

#58 @johnjamesjacoby
23 months ago

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

#59 @johnjamesjacoby
19 months ago

  • Milestone changed from 2.7 to 2.6
  • Type changed from enhancement to defect

Moving to 2.6.

See #3132, #3135.

#60 @johnjamesjacoby
11 months ago

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.

#61 @johnjamesjacoby
11 months ago

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.

#62 @johnjamesjacoby
11 months ago

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.

This ticket was mentioned in Slack in #bbpress by jjj. View the logs.


10 months ago

#64 @johnjamesjacoby
10 months ago

  • Milestone changed from 2.6 to 2.6.1

Let's worry about this after 2.6.

Note: See TracTickets for help on using tickets.