Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#6865 closed enhancement (fixed)

`bp_activity_get_actions()` should only sort when necessary

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch needs-unit-tests
Cc:

Description

Sorting is an expensive operation (see #6864) so we should refrain from doing it when it's not needed. bp_activity_get_actions() sorts each component's actions each time the function is called.

But this is generally not necessary. Sometimes post-type-specific actions are loaded dynamically when bp_activity_get_actions() is called. In these cases, the sort is necessary. Otherwise, the sort order will always remain the same.

Attachments (5)

6865.diff (1.6 KB) - added by boonebgorges 5 years ago.
6865.02.patch (1.7 KB) - added by imath 5 years ago.
6865.2.diff (1.5 KB) - added by boonebgorges 5 years ago.
6865.unittests.patch (3.0 KB) - added by imath 5 years ago.
6865.unittests.02.patch (3.8 KB) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (13)

@boonebgorges
5 years ago

#1 follow-up: @boonebgorges
5 years ago

6865.diff is a simple way to avoid unnecessary sorts.

The one thing I'm unsure about is whether this will cover the *initial* sort - ie, the first time bp_activity_get_actions() is called. We might need a one-time sort routine to run after all native components have registered their actions, in addition to the just-in-time sorting recommended by 6865.diff. @imath, do you have any thoughts about this?

#2 @DJPaul
5 years ago

Nice catch

#3 in reply to: ↑ 1 ; follow-up: @imath
5 years ago

  • Keywords needs-unit-tests added

Replying to boonebgorges:

@imath, do you have any thoughts about this?

My first thought is i agree saving overload is very important, but i think we must be careful to make sure things are still working the way they should :)

I've tested your patch and the result is the activity actions are no more sorted. An easy way to reproduce is to activate the blogs component, and you'll see that comments with a 10 priority is above posts which have a 5 priority, and this is wrong 5 should be displayed before 10.

I've made some quick tests by using 2 times the bp_activity_get_actions() function and the only way i've succeeded in making sure $_actions can be different than $bp->activity->actions is in .02.patch.

I think we should have unit tests here, it could help us to see if we need an initial sort.

Last edited 5 years ago by imath (previous) (diff)

@imath
5 years ago

@boonebgorges
5 years ago

#4 in reply to: ↑ 3 ; follow-up: @boonebgorges
5 years ago

Replying to imath:

My first thought is i agree saving overload is very important, but i think we must be careful to make sure things are still working the way they should :)

Details, details ;)

I'm not sure why your approach in 6865.02.patch works, but I guess my original patch didn't work because of the loose comparison of objects. A simple flag like $do_sort in 6865.2.diff works better, and is probably easier to understand.

In my testing, the initial call to bp_activity_set_actions() always sorts *unless there are no post types being tracked in the activity stream*. For this reason, I added a second flag: $sorted. I'm not sure what a unit test for this would look like - we can wipe the $bp->activity->actions object and reinitialize it, but we'll need to shut down post type tracking temporarily in order to trigger the scenario in which $sorted is necessary.

In a quick test where I called bp_activity_get_actions() about 500 times, wall time for the function is 1.33 seconds before the patch, and .194 seconds with the patch. (And much faster still with the changes to bp_sort_by_key() suggested in #6864.)

#5 in reply to: ↑ 4 @imath
5 years ago

Replying to boonebgorges:

I'm not sure why your approach in 6865.02.patch works

Actually it doesn't work that great :)

The unit tests in 6865.unittests.patch are what i've thought of. They are passing in trunk and with your patch 6865.2.diff and my 6865.02.patch is failing the 3rd test

But correct me if i'm wrong, your patch should have no impact if a post type is registered, actions should always be sorted each time bp_activity_get_actions() is used no ?
What happens if a plugin is not using a post type for his action and is setting his action with a lower position than let's activity_update ? Would actions be sorted ?

#6 @imath
5 years ago

What happens if a plugin is not using a post type for his action and is setting his action with a lower position than let's activity_update ? Would actions be sorted ?

i have my reply, actions wouldn't be sorted with 6865.2.diff :(

See test_bp_activity_get_actions_sort_no_post_type_registered() in 6865.unittests.02.patch

Last edited 5 years ago by imath (previous) (diff)

#7 @boonebgorges
5 years ago

Thanks for the tests, @imath.

I realize now that the patch I uploaded 6865.2.diff doesn't actually contain the static $sorted workaround I described above. Whoops. This $sorted was meant to address exactly what your test demonstrates.

I repatched and was about to upload, but then I got discouraged. Setting flags seems very fragile and bad practice. The problem is that sorting should happen only when necessary, which is to say when a new action is added. So I decided instead to move the sorting to bp_activity_set_action(). This means that sorting happens a bit more frequently than it otherwise might (every time an action is added - maybe 20 times during a pageload), but each sort is only on the affected component, rather than all components. And subsequent calls to bp_activity_get_actions() - even hundreds of them - will result in no additional sorting. This seems to me to be a good tradeoff.

#8 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 10515:

Sort activity actions when they're added, instead of when they're fetched.

Sorting activity actions can be an expensive process, so should not be done
any more frequently than necessary. And it's only necessary to sort when a
new action has been added. So instead of sorting in bp_activity_get_actions(),
we now do it in bp_activity_set_action().

Props boonebgorges, imath.
Fixes #6865.

Note: See TracTickets for help on using tickets.