Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#6864 closed defect (bug) (fixed)

`bp_sort_by_key()` is slow

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: Core Keywords: 2nd-opinion has-patch
Cc:

Description

bp_sort_by_key() uses create_function() to create a usort() callback. This process is very slow, especially when called hundreds of times on a pageload.

See, eg, when pulling up large numbers of activity items -> bp_activity_generate_action_string() -> bp_activity_get_actions(). In my specific case, 70 activity items are being pulled up, which results in bp_activity_get_actions() being called 70 times. Each time the latter function is called, it causes each component's actions to be sorted (another performance issue - I'll open a separate ticket). With 12 components, this results in bp_sort_by_key() being called 840 times.

In the above situation, with the current create_function() implementation, the total wall time for bp_sort_by_key() is about 1,101,000 microseconds - more than 1 second.

Switching to an anonymous function brings the wall time down to about 27,000 microseconds, an improvement of over 400x. But anonymous functions are only available in PHP 5.3. So I'd like to suggest one of the following:

  1. We use anonymous functions when available (eg, if ( class_exists( 'Closure' ) ))
  2. We break the callback out into its own named function. But then we'll need to put the $key and $type values into a global variable so they're accessible in that function.
  3. We fake PHP 5.3 callbacks using a technique like the last one on this page http://codeblow.com/questions/pass-extra-parameters-to-usort-callback/

Item 1 seems the most straightforward, but seeing as we don't do this anywhere else in BP, I want to be sure that no one has a problem with the approach.

Attachments (2)

6864-class.diff (3.7 KB) - added by boonebgorges 5 years ago.
6864-global.diff (2.7 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (9)

#1 follow-up: @DJPaul
5 years ago

I just tested your first approach because I didn't think it'd get past the parser, and it doesn't work :) https://3v4l.org/qutlJ

#2 in reply to: ↑ 1 @boonebgorges
5 years ago

  • Keywords has-patch added

Replying to DJPaul:

I just tested your first approach because I didn't think it'd get past the parser, and it doesn't work :) https://3v4l.org/qutlJ

Oh, you and your technicalities :-D

Two attachments.

  1. 6864-global.diff implements my suggestion 2 above.
  2. 6864-class.diff implements suggestion 3.

Neither is as fast as a closure, but both are much faster than create_function(). In my quick benchmarks, approach 6864-class.diff is about 10x faster than what we've currently got, while 6864-global.diff is 2-3x faster (the buddypress() access is expensive; it may be faster to have standalone globals, but *shudder*)

I'm recommending we go with 6864-class.diff unless someone has strong feelings against it.

#3 @DJPaul
5 years ago

Looks crazy but go for it

#4 @boonebgorges
5 years ago

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

In 10514:

Improve performance of bp_sort_by_key().

bp_sort_by_key() originally used create_function() to allow sorting by
dynamic $type and $key. However, create_function() has a tendency to
perform poorly. PHP 5.3 anonymous closures are much faster, but are unavailable
as long as we support PHP 5.2. So we introduce a new class that plays a role
similar to an anonymous closure, with a similar speed benefit.

Fixes #6864.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

#6 @DJPaul
4 years ago

  • Component changed from API to Core

#7 @boonebgorges
4 years ago

In 11362:

Use a closure for bp_sort_by_key() callback.

Previously, bp_sort_by_key() was refactored to avoid the use
of create_function(). See #6864. This refactoring needed to be
PHP 5.2-compatible, which required a workaround for the
unavailabilty of closures. This workaround can now be removed.

See #7299.

Note: See TracTickets for help on using tickets.