Skip to:
Content

Opened 14 months ago

Last modified 3 months ago

#7218 reopened enhancement

Only load component action and screen code when we're on the component's page

Reported by: r-a-y Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch early
Cc: leho@…

Description

We should only load a component's action and screen code when we're on the component's page.

This can save us some memory.

Attached patch does this for the activity component. I've introduced a new method called late_includes() that runs on the 'bp_setup_canonical_stack' hook because that is the place where we've ensured that $bp->current_component and $bp->current_action is fully set up.

In the patch, I've moved some code that is not specific to the activity component's action or screen code to bp-activity-functions.php temporarily until I can figure out a better place to put them. Perhaps, create a new file called bp-activity-hooks.php?

Attachments (8)

7218.members.patch (23.9 KB) - added by r-a-y 6 months ago.
For the Members component - refreshed for 2.9
7218.akismet.patch (4.9 KB) - added by r-a-y 6 months ago.
Refreshed for 2.9
7218.02.patch (40.5 KB) - added by r-a-y 6 months ago.
For the Activity component - refreshed for 2.9. Also conditionally loads activity RSS feeds and notification settings.
7218.friends.patch (10.4 KB) - added by r-a-y 6 months ago.
For the Friends component - refreshed for 2.9. Also conditionally loads the activity code only if the Activity component is active.
7218.groups.patch (47.8 KB) - added by r-a-y 5 months ago.
For the Groups component - refreshed for 2.9. Also conditonally loads notification settings and the activity and legacy forums code if those components are active.
7218.groups-autoload.patch (145.6 KB) - added by r-a-y 4 months ago.
Groups screen class autoloader
7218.late-includes.patch (941 bytes) - added by r-a-y 3 months ago.
7218.activity-autoload.patch (82.1 KB) - added by r-a-y 3 months ago.

Download all attachments as: .zip

Change History (38)

#1 @DJPaul
14 months ago

Interesting idea. Might break code that does a function_exist for something in the affected files, but might not. I don't think this'd affect a huge number of sites.

There's also AJAX handlers or form submission handlers that may be in these locations. For example, bp_ajax_get_suggestions needs to work from anywhere.

If the idea is to be a memory optimisation, we should benchmark this a bit with the current patch to see what impact it has (or doesn't). But I don't know how to reliably measure memory usage for PHP.

#2 @DJPaul
14 months ago

  • Milestone changed from Awaiting Review to Under Consideration

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


14 months ago

#6 @mercime
13 months ago

  • Status changed from new to reopened

#7 @DJPaul
13 months ago

I am excited about the potential of this ticket, albeit it with the concerns I stated previous that probably just need time to test and work through, and figure out what to do.

@r-a-y If we want to progress this, perhaps we could start by relocating functions that we feel are in the wrong files?

#8 @r-a-y
13 months ago

If we want to progress this, perhaps we could start by relocating functions that we feel are in the wrong files?

Sounds like a good plan. For things like Akismet, AJAX and the like, do we stuff them in bp-activity-functions.php or create a new file for hooks such as bp-activity-hooks.php?

Akismet could also be loaded conditionally with bp_is_active( 'activity', 'akismet' ). It's already in another file. Maybe we can move the Akismet-related code in bp-activity-actions.php to bp-activity-akismet.php. I'll experiment and report back.

Last edited 13 months ago by r-a-y (previous) (diff)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


13 months ago

#10 @r-a-y
13 months ago

In 11102:

Activity: Move Akismet loader to bp-activity-akismet.php.

Previously, bp_activity_setup_akismet() was placed in
bp-activity-actions.php. The actions file isn't a proper place to put
this code.

This commit moves this function to bp-activity-akismet.php and also
removes some duplicate conditional code. BP_Activity_Component already
checks if Akismet is loaded, so no need to re-duplicate this logic.

This is part of some prep work to conditionally load a component's action
and screen code when necessary.

See #7218.

#11 @r-a-y
13 months ago

r11102 moved the Akismet loader to bp-activity-akismet.php. akismet.patch moves some additional hardcoded Akismet code from bp-activity-functions.php over. There are still a few hardcoded instances left in bp-activity-functions.php, but I left them for now.


02.patch is basically the same as 01.patch, but fixes an issue with the activity email options not showing up on the "Settings > Email" screen. Had to move the activity email <table> code to bp-activity-notifications.php. Patch also moves the suggestions AJAX hook out of bp-activity-actions.php to bp-activity-functions.php as mentioned in comment:1.

Last edited 13 months ago by r-a-y (previous) (diff)

#12 @DJPaul
12 months ago

  • Milestone changed from Under Consideration to 2.8

@r-a-y Let's look at this in 2.8, and decide if and how we want to do it. I know you are keen on it, but I think we should have a dev-chat discussion at some point because it's somewhat of a structural change.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


11 months ago

#14 @lkraav
11 months ago

  • Cc leho@… added

#15 @r-a-y
10 months ago

I'm still keen on this.

Let me know what I can do to make this happen! :)

#16 @boonebgorges
9 months ago

I like this idea, and I think that limiting it to action/screen handlers is a good way to limit the possibility of compatibility breaks.

If we wanted something a bit more idiomatic than the "late includes" strategy, we could move the guts of the screen functions into classes and turn the procedural functions into wrappers. The classes would then be autoloaded. Not quite the memory savings as what you're proposing, but also a bit less subject to breakage (and no special cases needed for unit tests).

#17 @DJPaul
8 months ago

  • Milestone changed from 2.8 to 2.9

We never really gave much feedback on this, let's try to pick it up for 2.9.

#18 @hnla
6 months ago

  • Keywords dev-feedback added

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


6 months ago

@r-a-y
6 months ago

For the Members component - refreshed for 2.9

@r-a-y
6 months ago

Refreshed for 2.9

@r-a-y
6 months ago

For the Activity component - refreshed for 2.9. Also conditionally loads activity RSS feeds and notification settings.

@r-a-y
6 months ago

For the Friends component - refreshed for 2.9. Also conditionally loads the activity code only if the Activity component is active.

@r-a-y
5 months ago

For the Groups component - refreshed for 2.9. Also conditonally loads notification settings and the activity and legacy forums code if those components are active.

#20 @r-a-y
5 months ago

I've refreshed patches for the Activity, Friends, Groups and Members components.

For the Activity component, I moved the RSS feed code to a new file called bp-activity-feeds.php and loading that only on activity RSS pages.

For the Members component, I moved the registration and activation screen code to a new file called bp-members-register.php and loading that only on those pages.

For the Groups component, I moved the legacy forum screen code to bp-groups-forums.php and am loading this if the Legacy Forums component is active.

For the Friends component, the activity code is only loaded if the Activity component is active.

For all components, the action and screen code is only loaded if necessary. Also, I am conditionally loading the <table> markup used on the "Settings > Email" page as chances are this code isn't going to be used outside of that page.


I haven't done this for the Messages, Settings and Notifications components yet, but I will once we've decided we like this approach.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


5 months ago

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


5 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


5 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


4 months ago

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


4 months ago

#26 @r-a-y
4 months ago

@boone or @jjj - Can one (or both) of you take a look at this and let me know if this is viable?

I'm already doing this approach in a few plugins with success. This would most benefit multisite users since we wouldn't be loading a bunch of unnecessary code on every sub-site.

Last edited 4 months ago by r-a-y (previous) (diff)

#27 @boonebgorges
4 months ago

  • Keywords dev-feedback removed

General approach looks good to me. bp_is_active() checks for intercomponent dependencies is definitely good, regardless of the screen function stuff. For screen functions, I still think that autoloading would be more elegant, but that's something we can approach in the future, and shouldn't block these improvements https://buddypress.trac.wordpress.org/ticket/7218#comment:16

Let's be sure to mention the changes in the beta announcement, and/or urge plugin devs via bpdevel to test for compatibility issues.

Also, I am conditionally loading the <table> markup used on the "Settings > Email" page as chances are this code isn't going to be used outside of that page.

This code is loaded during the save routine in addition to the screen function, for validation reasons. I haven't checked myself, but you should be sure that this validation continues to work. https://buddypress.trac.wordpress.org/browser/trunk/src/bp-settings/bp-settings-functions.php?marks=98#L83

@r-a-y
4 months ago

Groups screen class autoloader

#28 @r-a-y
3 months ago

A few dev chats ago, we talked about moving the screen and action code to classes so we can utilize class autoloading.

I've done this for the Activity and Groups components in the groups-autoload.patch and activity-autoload.patch patches.

The result is the filesize for bp-activity-actions.php and bp-activity-screens.php is now only 6KB individually, instead of 28KB and 16KB respectively.

The filesize for bp-groups-actions.php and bp-groups-screens.php is now only 7KB and 10KB respectively, instead of 22KB and 53KB.

This cuts down the Activity and Groups screen code by 75%!

activity-autoload.patch also requires late-includes.patch due to offshoring the activity RSS feed code until we are on an actual feed page.

Existing unit tests pass. Let me know what you think.

Last edited 3 months ago by r-a-y (previous) (diff)

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


3 months ago

#30 @boonebgorges
3 months ago

Thanks for working on this, @r-a-y! I think it's moving in the right direction.

You opted for a separate class for each router. Any specific reason why? I'd imagined maybe a single BP_Activity_Actions class, with (static?) methods for each router. This would keep our number of files lower. It would also prevent us from doing lots of logic inside of a __construct() method, which makes it easier to imagine writing tests for these at some point in the future.

What do you think? I don't feel strongly about it.

#31 @r-a-y
3 months ago

You opted for a separate class for each router. Any specific reason why?

If we added a singular BP_Activity_Actions class, we would run into the same problems as before (albeit less so).

Individual classes ensures that we only load the code we need to run when we hit a specific action.

It would also prevent us from doing lots of logic inside of a construct() method, which makes it easier to imagine writing tests for these at some point in the future.

For writing tests, we could pass parameters to the constructor. I did these for a few of the activity classes because some of the functions like bp_activity_action_delete_activity() required a return value.

Last edited 3 months ago by r-a-y (previous) (diff)

#32 @r-a-y
3 months ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

Bumping to 3.0 and marking as early.

Note: See TracTickets for help on using tickets.