Skip to:
Content

Opened 19 months ago

Closed 17 months ago

Last modified 14 months ago

#6853 closed defect (bug) (fixed)

Autoload BP classes

Reported by: boonebgorges Owned by:
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc: espellcaste@…

Description

A few versions ago, we moved most of our classes into their own files. Let's take advantage of this by autoloading.

Attachments (4)

6853.diff (8.7 KB) - added by boonebgorges 19 months ago.
6853.02.patch (7.9 KB) - added by r-a-y 19 months ago.
6853.2.diff (31.7 KB) - added by boonebgorges 19 months ago.
6853.03.patch (30.4 KB) - added by r-a-y 19 months ago.

Download all attachments as: .zip

Change History (25)

@boonebgorges
19 months ago

#1 @boonebgorges
19 months ago

  • Keywords 2nd-opinion has-patch added

6853.diff is a proof of concept. In my very light testing, BP's memory footprint decreases by between 1MB and 1.5MB by switching to autoload, since the only classes loaded into memory are those that are actually needed to display the page.

The POC is a bit ugly because we don't have real namespaces, and our "namespaces" are inconsistently applied. (Though, to be honest, the $irregular_map lookup in the autoload() method is probably *faster* than the path concatenation. It's just harder to maintain in the long run.)

SPL can be disabled in PHP 5.2, so I've left a fallback in place, which uses the existing bp-{$component}-classes.php file.

Any gotchas here? One concern is that the autoload logic - which looks for classes named BP_{$component}_{$foo} against a whitelist of BP components - might hit a false positive in a plugin that has a class name like BP_Groups_Whatever. This *shouldn't* happen because the plugin should be loading its own classes manually before they call them, but there could be a conflict if both BP and the plugin register their own autoload functions.

Is it crazy to do this without PHP 5.3, with mandatory SPL and namespaces? Seems to me it's worth considering, given the memory improvements.

#2 @r-a-y
19 months ago

Looks good!

02.patch replaces a few older instances of autoload with do_autoload.

Is it crazy to do this without PHP 5.3, with mandatory SPL and namespaces? Seems to me it's worth considering, given the memory improvements.

Since the BP codebase is getting larger and larger, we should try to optimize wherever we can, so big +1!

If we could ignore PHP 5.2, it would probably be better to follow a PSR standard. But, hey, we're tied to WordPress -- for better or worse.

One concern is that the autoload logic - which looks for classes named BP_{$component}_{$foo} against a whitelist of BP components - might hit a false positive in a plugin that has a class name like BP_Groups_Whatever.

Good call. It might be good to write a sample plugin using the BP_Component class to see if we can trip it up.

#3 follow-up: @slaFFik
19 months ago

Having BP_{$component}_{$foo} in 3rd party plugins is actually a bad practice. You know, prefix all the things and such. So in my opinion it should not be a blocking concern, as bad practices are a point of potential failures anyway (regularly, and developers should know that).

@r-a-y I see var_dump()s in your patch: src/bp-loader.php::541,590

#4 @DJPaul
19 months ago

There are admin classes we ought to support if we're going to do this, as well as template classes (that need splitting out into individual files -- there's another ticket for that).

Other than those two BP_Group classes, the others are all in the Core component, right? I am tempted to suggest we should rename those but that idea probably needs to wait until we discover what odd cases exist in the admin and template classes.

@r-a-y
19 months ago

#5 @r-a-y
19 months ago

@slaFFik - Oops! :) Patch is updated to remove the debug lines.

#6 @espellcaste
19 months ago

  • Cc espellcaste@… added

#7 in reply to: ↑ 3 @boonebgorges
19 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 2.5

Replying to slaFFik:

Having BP_{$component}_{$foo} in 3rd party plugins is actually a bad practice. You know, prefix all the things and such. So in my opinion it should not be a blocking concern, as bad practices are a point of potential failures anyway (regularly, and developers should know that).

I agree with this, and I don't think it'll be a major issue.

Replying to DJPaul:

There are admin classes we ought to support if we're going to do this, as well as template classes (that need splitting out into individual files -- there's another ticket for that).
Other than those two BP_Group classes, the others are all in the Core component, right? I am tempted to suggest we should rename those but that idea probably needs to wait until we discover what odd cases exist in the admin and template classes.

See #6870, where I've outlined the task. IMO, we should do no renaming at this point - it's better to have a hash of inconsistently named classes than to break backward compatibility for the sake of aesthetics. When the day comes for full PSR-4 compliance, maybe we can talk about changing class names, and building a compatibility layer for it :)

I think it makes sense to wait for #6870 before implementing autoload.

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


19 months ago

#9 @dcavins
19 months ago

Thanks for taking this on. Sounds like a pretty huge efficiency improvement.

#10 @boonebgorges
19 months ago

In 10530:

Rename BP_Walker_Category_Checklist file.

The filename should match the class- standard, for easier loading.

See #6870, #6853.

#11 @boonebgorges
19 months ago

6853.2.diff is an updated patch, after the moves in #6870. I made the necessary additions to $irregular_map, and wrapped all new require statements in a do_autoload check.

Rough benchmarks are showing a pretty consistent memory savings of between 1.5 and 2MB, on a local dev installation that has about a zillion plugins running. On a front-end load, memory usage goes from just under 50MB to just over 48MB. If we assume that, say, half of that 50MB is directly attributable to BuddyPress, then we're reducing BP's memory footprint by 7 or 8 percent with autoloading. And with some further rearrangement in the future (so that components are loaded in a sleeker way) we could get that number down even further.

Comments and feedback are welcome on the technique used. And if anyone sees any gotchas that I'm missing, please speak up. It'd be nifty to put sneak this into 2.5.

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


19 months ago

@r-a-y
19 months ago

#13 @r-a-y
19 months ago

03.patch fixes a fatal error with BP_Component being not found when SPL is disabled. I'm requiring the class in bp-core-loader.php as this is the earliest the class is required.

I'm also doing a class_exists() check in bp-core-component.php as some plugins might be manually requiring /bp-core/bp-core-component.php in their code to extend BP_Component (unlikely, but better to be safe than sorry!).

#14 @boonebgorges
19 months ago

  • Milestone changed from 2.5 to 2.6

Thanks, @r-a-y ! I was already feeling pretty shaky about sneaking this in at the last minute, and your patch is going to seal the deal. Let's do it first thing for 2.6, so we have plenty of time to shake things out.

#15 @boonebgorges
18 months ago

In 10652:

Autoload BuddyPress classes.

Backward compatibility with SPL-disabled installations is included.

See #6853.

#16 @needle
18 months ago

Since [10652], I get a "PHP Fatal error" when checking for the existence of one of my plugin classes called "BP_Groups_CiviCRM_Sync" - i.e. the with the following code:

if ( ! class_exists('BP_Groups_CiviCRM_Sync') ) {
  // do something
}

Does this change now mean that plugin authors cannot (or at minimum should not) name their classes "BP_Componentname_Etc"?

#17 follow-up: @boonebgorges
18 months ago

Thanks for testing against trunk! That's why I put it in early :)

I had a feeling that this would probably happen. In general I'd suggest using a unique prefix for your class/function names. But it's not acceptable for us to fatal like this. The simple answer is to do a file_exists() check before we attempt to autoload. I'd originally avoided this to minimize I/O, but I just did some testing and the added overhead is negligible.

#18 @boonebgorges
18 months ago

In 10653:

When autoloading, check for the existence of a file before including.

This avoids stepping on the toes of plugins that are using BP-ish prefixes for
their class names.

See #6853.

#19 in reply to: ↑ 17 @needle
18 months ago

Replying to boonebgorges:

I had a feeling that this would probably happen. In general I'd suggest using a unique prefix for your class/function names. But it's not acceptable for us to fatal like this. The simple answer is to do a file_exists() check before we attempt to autoload. I'd originally avoided this to minimize I/O, but I just did some testing and the added overhead is negligible.

Thanks, the latest commit fixes the issue for me. I could rename my classes, but I have no idea what the knock-on effects might be. Will follow the naming advice in future.

#20 @DJPaul
17 months ago

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

#21 @DJPaul
14 months ago

  • Component changed from API to Core
Note: See TracTickets for help on using tickets.