Opened 10 years ago
Closed 2 years ago
#1799 closed defect (bug) (fixed)
Introduce 'bump' functions to replace costly recounts
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.6 | Priority: | highest omg sweet tea |
Severity: | major | Version: | 2.0 |
Component: | General - Performance | Keywords: | needs-unit-tests |
Cc: | vpundir@…, pippin@…, stephen@…, nashwan.doaqan@…, danielbachhuber, UmeshSingla |
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)
Change History (84)
#3
@
10 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.
#5
@
9 years ago
- Milestone changed from 2.2 to Future Release
Functions are in, they just aren't being utilized. Punting to Future Release.
This ticket was mentioned in IRC in #bbpress-dev by netweb. View the logs.
8 years ago
#18
@
8 years ago
In 1799.3.patch refreshed patch only against new /trunk
#19
@
8 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.
8 years ago
#22
@
7 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).
#24
@
7 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.
7 years ago
#27
@
7 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.
7 years ago
This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.
7 years ago
This ticket was mentioned in Slack in #bbpress by netweb. View the logs.
7 years ago
This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.
6 years ago
#32
@
6 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.
#35
@
6 years ago
BTW, it could be efficient to recalculate for a bunch of topics/replies every - say - 15 seconds, for very active forums.. no?
#37
@
6 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.
6 years ago
#39
@
6 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.
6 years ago
#41
@
6 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.
#42
@
6 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.
#44
@
6 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.
#47
@
6 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 ofintval
when validating voice ids pulled from the database during calls tobbp_update_topic_voice_count()
.
#49
@
5 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].
#53
follow-up:
↓ 54
@
5 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.
#54
in reply to:
↑ 53
@
5 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
@
5 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
@
5 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):
- Adds forum/topic/reply sub-actions for
transition_post_status
- 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.
bbp_transitioned_*
functions are then hooked onto our late stage action hooks (bbp_new_reply
,bbp_trashed_reply
, etc.). These hooks are needed becausetransition_post_status
is called before any post meta is added for new forums/topic/replies.- 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. - Count bumping functions are then hooked onto the
bbp_transitioned_*
actions with a priority of 20 to allow room forbbp_update_*()
andbbp_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.
5 years ago
#58
@
5 years 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 withbbp_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.
(In [3826]) Add 'bump' functions for forum, topic, reply, and voice counts.