Skip to:
Content

BuddyPress.org

Opened 7 years ago

Closed 9 months ago

#7666 closed enhancement (maybelater)

Load template loop functions only when the template loop is initialized

Reported by: r-a-y's profile r-a-y Owned by:
Milestone: 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 7 years ago.

Download all attachments as: .zip

Change History (8)

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


7 years ago

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

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

#7 @espellcaste
9 months ago

  • Milestone Under Consideration deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.