WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 21 months ago

Last modified 19 months ago

#16574 closed enhancement (duplicate)

Lazy load ancestors

Reported by: nacin Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Performance Keywords: has-patch needs-testing
Focuses: Cc:

Description

It should be pretty easy to lazy load ancestors using a magic get function on 'ancestors', rather than querying for them individually via _get_post_ancestors().

johnjamesjacoby presented a use case where get_post_field() was triggering ancestors queries, with no way to avoid that if you know you need that information. Not sure how to solve that specific problem without thinking bigger.

Attachments (8)

16574.diff (827 bytes) - added by johnjamesjacoby 3 years ago.
Use get_posts in _get_posts_ancestors()
16574.2.diff (863 bytes) - added by johnjamesjacoby 3 years ago.
Post type agnostic, and make diff do what it's supposed to
16574.3.diff (777 bytes) - added by johnjamesjacoby 3 years ago.
Just use get_post instead of get_posts
16574.4.diff (6.4 KB) - added by scribu 2 years ago.
Introduce 'post_ancestors' cache group
16574.5.diff (7.8 KB) - added by scribu 2 years ago.
Introduce _WP_Post_Wrapper class
16574.6.diff (6.9 KB) - added by johnjamesjacoby 2 years ago.
Add isset() check to magic get()
16574.7.diff (7.9 KB) - added by scribu 2 years ago.
avoid Inception
16574.8.diff (7.4 KB) - added by scribu 2 years ago.
&__get()

Download all attachments as: .zip

Change History (42)

comment:1 westi3 years ago

Can we have more detail on the use case:

  • Maybe Some code
  • What we are trying to avoid

At the moment this ticket has a very specific solution proposed but little detail on the problem - which makes it hard to know if this is the best solution or not.

comment:2 nacin3 years ago

The use case is get_post_field( 'post_type', $post_id ) or some other query that doesn't require populating the ancestors. It could even be a straight get_post() call -- really, the use case doesn't have anything to do with the suggestion here other than it gave me the idea.

Being able to lazy load ancestors can save a number of queries. They're fast, but with a complex hierarchy and a number of get_post() calls, they can add up. Then only the queries are triggered when needed.

Thinking about it some more, this will require a real WP_Post object -- #12267. And is probably only one of a few improvements that can come out of that.

comment:3 johnjamesjacoby3 years ago

+1 - This saves countless ancestral queries in the bbPress plugin where a simple look into the posts table with get_post_field() is needed without the need for walking up a tree of post_parent's.

johnjamesjacoby3 years ago

Use get_posts in _get_posts_ancestors()

comment:4 johnjamesjacoby3 years ago

In the mean time, using get_posts in _get_post_ancestors() rather than hitting the DB directly with an uncached query cuts down on cache misses and DB hits in the double digits in the bbPress plugin, particularly in situations where there are Forum, Topic, or Reply widgets in sidebars, and/or subforums are involved.

comment:5 johnjamesjacoby3 years ago

  • Keywords has-patch added

johnjamesjacoby3 years ago

Post type agnostic, and make diff do what it's supposed to

comment:6 filosofo3 years ago

How about instead we deprecate the post object's ancestors property with a magic __get method, and then use a working get_post_ancestors() function to get the ancestors in core WP?

That way, we maintain backwards-compatibility for those who are referencing the ancestors post property in plugins, but we are explicit about actually querying the post ancestors, in core (and as recommended practice).

Resource-intensive queries like that should be as explicit as possible, for code readability and debugging purposes. Hiding the query in a magic __get method is slightly slower, and it makes it harder to identify the conditions in which such queries will occur.

In other words, as a band-aid to handle deprecated properties, it's fine, but let's not make it standard practice, particularly for retrieving external resources (database records) as opposed to e.g. internal object properties, which I think was the intent of __get.

comment:7 johnjamesjacoby3 years ago

Tested my above patch further, and $post->ancestors gets polluted in between other calls to get_posts(), so that isn't a viable alternative quite yet.

johnjamesjacoby3 years ago

Just use get_post instead of get_posts

comment:8 johnjamesjacoby3 years ago

Using get_post here also reduces cache misses with no adverse effects for things like single blog post views, where the next and previous posts are automatically added to wp_head, and then loaded again in the content area. Without this method, ancestor queries aren't cached. Same goes for pages in nav menus, etc...

comment:9 johnjamesjacoby3 years ago

As a general update, 16574.3.diff is still good. I've been running several .org installs with it since posted with only positive results. It doesn't add any fancy magic functions, but it does at least give ancestor queries the opportunity to hit the WP cache instead of always hitting the DB.

Any install with a page widget and a page menu would see a noticeable decrease in DB hits with this patch alone. The bbPress plugin sees a dramatic decrease (of sometimes 10+ queries) because it relies heavily on the hierarchy to manage the post relationships between forums/topics/replies.

If there are alternatives (magic functions, etc...) I'd love to collaborate on them to test the benefits.

comment:10 johnjamesjacoby3 years ago

  • Keywords needs-testing 3.3-early added; 3.2-early removed

Keeping in future release, removing 3.2-early keyword and adding 3.3-early keyword. Also added needs-testing keyword, because it needs testing.

Specifically looking to see if this causes an infinite loop situation, where an ancestor might call get_post() which calls _get_post_ancestors() which calls get_post() etc...

comment:11 scribu2 years ago

It seems to me that using a separate cache group just for post ancestors would fix a lot of problems related to cache pollution.

comment:12 scribu2 years ago

We could remove all the fragile logic around _get_post_ancestors() from get_post() and move the main work to get_post_ancestors().

Last edited 2 years ago by scribu (previous) (diff)

comment:13 scribu2 years ago

Only negative implication would be that you can't use $post->ancestors directly. You would have to call get_post_ancestors() instead, which I don't think is too much to ask.

comment:14 scribu2 years ago

  • Keywords 3.3-early removed
  • Milestone changed from Future Release to 3.4

16574.4.diff introduces the 'post_ancestors' cache group and merges _get_post_ancestors() into get_post_ancestors().

scribu2 years ago

Introduce 'post_ancestors' cache group

comment:15 nacin2 years ago

Doesn't seem like it would need a new group all together. The key in the posts cache group could simply be ancestors-$id.

That said, breaking compatibility with $post->ancestors is definitely not something we can get away with right now. Perhaps after the success of WP_Theme, someone (me?) might take a crack at a proper WP_Post in a future release, which could alleviate back compat issues.

I did very quick grep in the plugins directory for 'post->ancestors' and stopped it after seeing at least 50 plugins (100s of instances) using it.

comment:16 scribu2 years ago

Doesn't seem like it would need a new group all together. The key in the posts cache group could simply be ancestors-$id.

Why would that be better?

I did very quick grep in the plugins directory for 'post->ancestors' and stopped it after seeing at least 50 plugins (100s of instances) using it.

Sigh...

scribu2 years ago

Introduce _WP_Post_Wrapper class

comment:17 scribu2 years ago

16574.5.diff introduces a _WP_Post_Wrapper class that only deals with $post->ancestors.

johnjamesjacoby2 years ago

Add isset() check to magic get()

comment:18 johnjamesjacoby2 years ago

Gah! __

Replying to scribu:

16574.5.diff introduces a _WP_Post_Wrapper class that only deals with $post->ancestors.

Creating a whole new class for this one thing is overkill. It does fix the problem, but introduces two others:


First issue happens in the default BuddyPress theme:

Notice: Indirect modification of overloaded property _WP_Post_Wrapper::$object_id has no effect in /wp/wp-includes/nav-menu.php on line 591

Something with

$original_object = get_post( $menu_item->object_id );

is causing the object_id to be overloaded. Not sure where or why yet, if the problem is on BuddyPress's end or not. Should note there are no errors as is before 16574.5.diff is applied.


Second issue happens because of missing isset() in your magic __get()

Trying to get property of non-object in /wp/wp-includes/post.php on line 442

16574.6.diff fixes the second issue, above.

Last edited 2 years ago by johnjamesjacoby (previous) (diff)

comment:19 scribu2 years ago

Creating a whole new class for this one thing is overkill.

It paves the way for WP_Post. I didn't name it that in the hopes that people won't start explicitly checking for it, but I'm not sure if that's a wise decision.

$original_object = get_post( $menu_item->object_id );

That's because get_post() takes it's first parameter by reference and $menu_item is now an instance of _WP_Post_Wrapper.

There was a similar notice in get_page_uri(), which is fixed in the patch.

Last edited 2 years ago by scribu (previous) (diff)

comment:20 follow-up: scribu2 years ago

As for the isset() in __get(), I'm not sure if it's a good idea to silently fail if the key doesn't exist.

Last edited 2 years ago by scribu (previous) (diff)

comment:21 scribu2 years ago

Trying to get property of non-object in /wp/wp-includes/post.php on line 442

This seems to indicate that $this->post is not an object, which is really strange. isset() would just mask the problem, instead of fixing the root cause.

scribu2 years ago

avoid Inception

comment:22 follow-up: scribu2 years ago

16574.7.diff avoids wrapping a _WP_Post_Wrapper in another one.

comment:23 in reply to: ↑ 20 johnjamesjacoby2 years ago

Replying to scribu:

As for the isset() in __get(), I'm not sure if it's a good idea to silently fail if the key doesn't exist.

Plugins like BuddyPress and bbPress don't always load a real $post and rely on silent failure. The $post global is sometimes set with dummy (or semi relevant) information to satisfy the bare minimum of WordPress's API that expects it to exist. Silent failure is the way the WP_User magic methods work; given WordPress's pluggable nature, it's fitting that it prevent notices where it can.

Last edited 2 years ago by johnjamesjacoby (previous) (diff)

comment:24 in reply to: ↑ 22 johnjamesjacoby2 years ago

Replying to scribu:

16574.7.diff avoids wrapping a _WP_Post_Wrapper in another one.

With 16574.7.diff BuddyPress's bp-default menu still shown the first notice I posted above.

Version 0, edited 2 years ago by johnjamesjacoby (next)

comment:25 scribu2 years ago

Plugins like BuddyPress and bbPress don't always load a real $post and rely on silent failure.

I don't understand. With a plain stdClass, when the key doesn't exist, you would get a notice as well.

comment:26 follow-up: scribu2 years ago

16574.8.diff makes __get() return by reference, which should fix the "Indirect modification of overloaded property ... has no effect" notice.

scribu2 years ago

&__get()

comment:27 Mamaduka2 years ago

  • Cc georgemamadashvili@… added

comment:28 in reply to: ↑ 26 braydonf2 years ago

Replying to scribu:

16574.8.diff makes __get() return by reference, which should fix the "Indirect modification of overloaded property ... has no effect" notice.

It would be great to have an action/filter to be able to use different classes for each post depending on its post_type that would need to be extended from this WP_Post_Wrapper class.

Last edited 2 years ago by braydonf (previous) (diff)

comment:29 follow-up: scribu2 years ago

@braydonf: This is already outside the scope of this ticket. _WP_Post_Wrapper was just a way to provide back-compat for $post->ancestors.

We should discuss the plan for the WP_Post class in a new ticket or on wp-hackers.

comment:30 in reply to: ↑ 29 braydonf2 years ago

Replying to scribu:

We should discuss the plan for the WP_Post class in a new ticket or on wp-hackers.

Okay, just made a new ticket for this #20259

comment:31 ryan2 years ago

  • Milestone changed from 3.4 to Awaiting Review

comment:32 ryan2 years ago

  • Milestone changed from Awaiting Review to Future Release

comment:33 scribu21 months ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Superseded by #21309

comment:34 johnjamesjacoby19 months ago

Related cache changesets : r21943, r21952.

Note: See TracTickets for help on using tickets.