`bp_activity_get_actions()` should only sort when necessary

[boonebgorges]

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.

[boonebgorges]

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?

[DJPaul]

Nice catch

[imath]

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.

[boonebgorges]

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

[imath]

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 ?

[imath]

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

[boonebgorges]

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.

[boonebgorges]

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.