Skip to:
Content

Opened 9 months ago

Last modified 5 days 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: 2.9 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch dev-feedback
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 (5)

7218.members.patch (23.9 KB) - added by r-a-y 12 days ago.
For the Members component - refreshed for 2.9
7218.akismet.patch (4.9 KB) - added by r-a-y 12 days ago.
Refreshed for 2.9
7218.02.patch (40.5 KB) - added by r-a-y 12 days 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 12 days 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 6 days 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.

Download all attachments as: .zip

Change History (25)

#1 @DJPaul
9 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
9 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.


9 months ago

#6 @mercime
8 months ago

  • Status changed from new to reopened

#7 @DJPaul
8 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
8 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 8 months ago by r-a-y (previous) (diff)

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


8 months ago

#10 @r-a-y
7 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
7 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 7 months ago by r-a-y (previous) (diff)

#12 @DJPaul
7 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.


6 months ago

#14 @lkraav
6 months ago

  • Cc leho@… added

#15 @r-a-y
5 months ago

I'm still keen on this.

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

#16 @boonebgorges
4 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
3 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
4 weeks ago

  • Keywords dev-feedback added

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


4 weeks ago

@r-a-y
12 days ago

For the Members component - refreshed for 2.9

@r-a-y
12 days ago

Refreshed for 2.9

@r-a-y
12 days ago

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

@r-a-y
12 days 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
6 days 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
6 days 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.


6 days ago

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


5 days ago

Note: See TracTickets for help on using tickets.