Skip to:
Content

BuddyPress.org

Opened 22 months ago

Last modified 17 months ago

#7666 new enhancement

Load template loop functions only when the template loop is initialized

Reported by: r-a-y Owned by:
Milestone: Under Consideration Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch dev-feedback 2nd-opinion
Cc:

Description

A lot of our codebase is for template functions.

We do not need to load this code if a template loop isn't initialized.

For example, you can't use the bp_activities() or bp_get_activity_item_id() functions without initializing the activity template loop, right? So it would make sense to only load this if the bp_has_activities() function was called.

Attached patch is just an example of offshoring template functions used for the bp_has_activities() template loop; it's a 56KB file savings and doesn't break unit tests.

Mutiply this technique for all our template loops across all components (especially for the Groups component) and we can definitely trim more fat.

This would be noticeable on multisite installs when BP isn't on the root blog.

Attachments (1)

7666.bp_has_activities.patch (117.1 KB) - added by r-a-y 22 months ago.

Download all attachments as: .zip

Change History (7)

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


22 months ago

#2 @DJPaul
22 months ago

  • Milestone changed from Awaiting Review to Under Consideration

Recording what @johnjamesjacoby said on Slack:

I’m reluctant to get behind this, for a few reasons I never thought I’d say.
*We can’t lazy/auto load functions
*Modern PHP versions mitigate a lot of this
*Back compat for whos-knows-what in widgets means it’s hard to _stop_ including stuff that’s always been around
*bp_is_active() is the component layer of this kind of can-i-use-this API. A bp_function_exists() API to handle it (even internally) would add foreign complexity
*Compelling benchmarks to prove what the impact is would convince me. If we score a 25% memory reduction with zero core breakage (for example) would be hard to say no to, even with some potential breakage

#3 @r-a-y
22 months ago

My technique doesn't require new APIs. It just uses a require_once line near the end of the loop.

There shouldn't be a backward-compatibility issue unless developers use functions that are not meant to be used outside the loop. We can try to be conservative with what is offshored though.

It's hard to do benchmarks without writing a patch for the whole codebase, but it would save loading thousands of lines of code. The example patch already saves more than 2000+ lines of code alone and that is just for the bp_has_activities() loop.

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

#4 @DJPaul
22 months ago

We have recent tickets where people have tried to use template loop functions outside of a template loop, and have asked for us to add optional arguments to those functions (for IDs, etc), so that they can be used outside a loop. This suggests we have API architecture problems somewhere for those things (I can dig out the ticket references later) but also suggests that the change proposed here is definitely high-risk.

#5 @DJPaul
17 months ago

  • Keywords 2nd-opinion added

I think the risk of this change is too high; #7652 is a discussion ticket about the opposite (having template loop functions used everywhere) because feedback we have is that people already do it. It wouldn't surprise me if people were manually setting a global to use a specific template function.

#6 @r-a-y
17 months ago

I stand by my comments, especially:

We can try to be conservative with what is offshored though.

If we feel a certain function is highly in-use, then we don't offload it. We have a lot of functions specific to the template loop that are never used outside the loop.

Note: See TracTickets for help on using tickets.