Make WordPress Core

Opened 14 years ago

Closed 15 months ago

Last modified 13 months ago

#10886 closed defect (bug) (fixed)

WordPress should not unnecessarily query posts at page load

Reported by: junsuijin's profile junsuijin Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.0 Priority: normal
Severity: normal Version: 2.8.4
Component: Bootstrap/Load Keywords: has-unit-tests has-patch needs-docs has-dev-note
Focuses: performance Cc:

Description

For plugins like BuddyPress, with pages that have no WordPress post content, a query for posts at load time creates unnecessary overhead. I've tested this patch (with WordPress MU) and it removes 3 queries on all pages when the 'NO_QUERY' constant is defined. A very quick scan of page load times also suggests about 20% in time savings on average.

Other plugins like shopping carts and any others that needn't query the WordPress posts table can benefit from this as well.

My previous means of achieving this effect was to simply return null to the 'posts_request' filter, which removes only 2 queries per page.

The devised method uses suggestions from azaozz and sivel, and allows for setting the definition of 'NO_QUERY' using the 'init' action hook. It also still allows for further querying of the posts at a later point if necessary.

I welcome further suggestions about the patch, and any other ways this could be accomplished more effectively.

Attachments (11)

lighter-load.patch (660 bytes) - added by junsuijin 14 years ago.
lighter-load.2.patch (739 bytes) - added by sivel 14 years ago.
New patch that replaces the constant with a filter, and skips wp() as well.
lighter-load.3.patch (643 bytes) - added by junsuijin 14 years ago.
filter used, wp() circumvented, 'wp' hook left intact, commented
10886.diff (601 bytes) - added by westi 14 years ago.
I don't think we should be calling the wp hook from two different places like that. I think I prefer this patch. Does this work for you?
10886.2.diff (563 bytes) - added by sivel 14 years ago.
Adds $query_vars to the filter so that you can use that information to determine how you should return.
10886.2.2.diff (562 bytes) - added by junsuijin 14 years ago.
Adds $query_vars to the filter so that you can use that information to determine how you should return. (fixed grammatical error)
10886.3.diff (721 bytes) - added by MikeHansenMe 8 years ago.
Refresh plus docs
10886.4.diff (803 bytes) - added by tyxla 8 years ago.
Fixing run_wp_main filter documentation
10886.5.diff (850 bytes) - added by DrewAPicture 8 years ago.
Fixed docs
10886.6.diff (1.4 KB) - added by wonderboymusic 8 years ago.
10886.7.diff (1.5 KB) - added by wonderboymusic 8 years ago.

Download all attachments as: .zip

Change History (56)

#1 follow-up: @ryan
14 years ago

Perhaps wp() shouldn't be called at all for those pages.

#2 @usermrpapa
14 years ago

  • Cc steve@… added

#3 in reply to: ↑ 1 @westi
14 years ago

  • Cc westi added

Replying to ryan:

Perhaps wp() shouldn't be called at all for those pages.

Possibly a good idea.

Whatever I think a filter to which you return false to disable stuff may be better here than a define.

We don't want to create something that people put into wp-config.php by accident defines should be reserved for things configured there IMHO

@sivel
14 years ago

New patch that replaces the constant with a filter, and skips wp() as well.

#4 @sivel
14 years ago

  • Cc matt@… added
  • Keywords tested removed

#5 @sivel
14 years ago

It seems I may have misunderstood Ryan's suggestion. I have talked to junsuijin and he should be doing some testing and adding a new patch.

#6 @junsuijin
14 years ago

Building on ryan, westi, and sivel's work/suggestions, this new patch seems to further improve page loading times. More testing indicates that extra queries are caused by setting the 'wp_main' filter to false when site-wide post requests are made later on. This seems a case of benefitting the majority over the minority though, since the people that find the 'wp_main' filter set to false on a page where they want to include something like a site-wide posts widget, can just reset the filter to true.

#7 @junsuijin
14 years ago

  • Keywords tested added

For reference, here is the corresponding BuddyPress ticket:
http://trac.buddypress.org/ticket/967

@junsuijin
14 years ago

filter used, wp() circumvented, 'wp' hook left intact, commented

@westi
14 years ago

I don't think we should be calling the wp hook from two different places like that. I think I prefer this patch. Does this work for you?

#8 follow-up: @sivel
14 years ago

I'd like to have the filter pass $query_vars also so that the $query_vars could be used as a way to determine if you should return false or not. I don't have a current use for this, but I see a possibility of it being helpful.

#9 in reply to: ↑ 8 @westi
14 years ago

Replying to sivel:

I'd like to have the filter pass $query_vars also so that the $query_vars could be used as a way to determine if you should return false or not. I don't have a current use for this, but I see a possibility of it being helpful.

+1

That sounds very sensible.

#10 @sivel
14 years ago

  • Keywords tested removed

The patch as it stands now causes the admin to say there are the correct number of posts/pages/attachments but "hides" them from view in the edit lists. I'm not saying this is bad or good. Just wondering what everyone else has to say about it. Should it be up to the developer to make sure they only filter for the front end by checking if is_admin() is true? Or do we want to assume that we always want to be able to see this in the admin? Or should we add a second filter for the admin? So we would have run_wp_main_front and run_wp_main_admin or something like that.

#11 follow-up: @sivel
14 years ago

Sorry, I should have also stated that this is the result when running the filter using a plugin or in a themes functions.php without context to what is actually being called/loaded.

#12 in reply to: ↑ 11 @westi
14 years ago

Replying to sivel:

Sorry, I should have also stated that this is the result when running the filter using a plugin or in a themes functions.php without context to what is actually being called/loaded.

I think if someone does this in a theme/plugin without checking the context then they deserve to be eaten by those dragons :-)

@sivel
14 years ago

Adds $query_vars to the filter so that you can use that information to determine how you should return.

@junsuijin
14 years ago

Adds $query_vars to the filter so that you can use that information to determine how you should return. (fixed grammatical error)

#13 @junsuijin
14 years ago

I know that BuddyPress hooks to 'wp' in several places. Not doing the 'wp' action simply because WordPress has not run its initial post-grabbing queries, will also break any other plugins that rely on that action hook.
Which is less desirable:
1) "[not] calling the wp hook from two different places?"
2) forcing those who choose to return false to the 'run_wp_main' filter to do_action_ref_array( 'wp', array( (object) array() ) ) (likely in whatever function they use to set the 'run_wp_main' filter to false)?

#14 @westi
14 years ago

I don't know what BuddyPress uses the wp hook for but I think the following is what we should achieve.

  • We are trying to stop something running so we should just stop that.
  • We should not duplicate do_actions or expect plugins to run them if they use the new hook - If we do this I think we have failed.

Therefore it looks like we need to understand if wp is the hook BuddyPress should be using on these pages and we need to move the new filter into the wp.main() function or whether BuddyPress should be hooked in to something else.

#15 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

#16 @mtdewvirus
13 years ago

  • Cc mtdewvirus@… added

#17 @dd32
13 years ago

See Also: #12256

robots.txt requests cause WordPress to query the database for all the homepage posts. It'd be good if there was perhaps a query var to short circuit the internal querying and instead perhaps fake a page template... Or probably better to just offer an appropriate hook.

#18 @Denis-de-Bernardy
13 years ago

  • Cc Denis-de-Bernardy added

#19 @nacin
9 years ago

  • Component changed from Optimization to Bootstrap/Load
  • Focuses performance added
  • Keywords changed from has-patch, needs-testing to has-patch needs-testing

#20 @arnee
9 years ago

Would be nice to see this in 4.0. All workarounds, like the one posted on wp-hackers some time ago, are ugly and lead to errors.

#21 @wonderboymusic
9 years ago

  • Keywords needs-patch needs-refresh added; has-patch needs-testing removed

Patch blows up

@MikeHansenMe
8 years ago

Refresh plus docs

@tyxla
8 years ago

Fixing run_wp_main filter documentation

#22 @MikeHansenMe
8 years ago

  • Keywords has-patch added; needs-patch needs-refresh removed

#23 @MikeHansenMe
8 years ago

10886.4.diff still applies but needs @since doc updated when it goes in.

@DrewAPicture
8 years ago

Fixed docs

#24 @DrewAPicture
8 years ago

  • Keywords commit added
  • Milestone changed from Future Release to 4.4

10886.5.diff updates the @since version and fixes some docs issues.

Moving for commit consideration.

#25 @wonderboymusic
8 years ago

  • Keywords commit removed

#26 @wonderboymusic
8 years ago

Since [21163] happened in 3.5, should probably be something more like 10886.6.diff

This ticket was mentioned in Slack in #core by sergey. View the logs.


8 years ago

#28 @wonderboymusic
8 years ago

In 10886.7.diff:

  • $wp->parse_request() needs to return a bool
  • If it returns false, send headers, but don't do any querying, 404 handling, or setup of globals

If you use these filters:

add_filter( 'template_include', '__return_false' );
add_filter( 'do_parse_request', '__return_false' );

The whole request will only produce 3 db queries (no external cache):

SELECT option_value FROM wp_options WHERE option_name = 'WPLANG' LIMIT 1
SELECT * FROM wp_users WHERE user_login = 'admin'
SELECT user_id, meta_key, meta_value FROM wp_usermeta WHERE user_id IN (1) ORDER BY umeta_id ASC

get_option( 'locale' ) gets called even if WPLANG is defined in wp-config.php or elsewhere.

#30 @wonderboymusic
8 years ago

  • Owner junsuijin deleted
  • Status changed from new to assigned

#31 @wonderboymusic
8 years ago

  • Milestone changed from 4.4 to Future Release

I'll grab this later if I get the energy

This ticket was mentioned in Slack in #core by ocean90. View the logs.


7 years ago

#33 @lukecavanagh
6 years ago

Seems like a solid performance change for 4.8.

#34 @SergeyBiryukov
18 months ago

  • Milestone changed from Future Release to 6.0

This ticket was mentioned in PR #2032 on WordPress/wordpress-develop by pbearne.


18 months ago
#35

  • Keywords has-unit-tests added

Patch refresh that:
Added boolean returns to parse_request()
Add shortcut to stop load not needed calls to main() if parse_request() returns early
Add test to make sure parse_request() return boolean

Trac ticket: (https://core.trac.wordpress.org/ticket/10886

#36 @davidbaumwald
17 months ago

  • Keywords early added

Probably safe to include this earlier in a cycle to make sure it's thoroughly tested.

#37 @spacedmonkey
17 months ago

  • Keywords needs-dev-note added

+1 on the patch. I think we should get this in early 6.0 release.

I did a quick review on plugins that use this filter. See here.Looks like return the value, so I don't see breakages here. But this needs a dev not as it may have side effects.

#38 @spacedmonkey
15 months ago

  • Owner set to spacedmonkey

#39 @spacedmonkey
15 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 52814:

Bootstrap/Load: Stop unnecessary queries when using the do_parse_request filter.

Developers of plugins and themes can use the do_parse_request filter to hot-wire requests and hook in early to render custom pages. However, even through these request may not need post queries and 404 lookups to be run, they run anyway. This can results in unnecessary SQL queries running on these requests. By adding a return value to the parse_request method of the WP class, these queries can now be skipped.

Props junsuijin, ryan, westi, sivel, dd32, wonderboymusic, arnee, tyxla, DrewAPicture, lukecavanagh, SergeyBiryukov, davidbaumwald, Spacedmonkey, pbearne.
Fixes #10886.

#40 @SergeyBiryukov
15 months ago

In 52815:

Docs: Add a @since note for WP::parse_request() about the new return value.

Includes minor code layout fixes for consistency. Additionally, this moves a more general unit test above the more specific one.

Follow-up to [52814].

See #10886.

#41 @lkraav
15 months ago

Was there also some commit in core that demonstrates using $parsed? For example, to optimize the infamous robots.txt loading process, or something else?

#42 @milana_cap
14 months ago

  • Keywords needs-docs added

We could have a code example for Code reference here.

Last edited 14 months ago by milana_cap (previous) (diff)

#44 follow-up: @spacedmonkey
13 months ago

  • Keywords has-dev-note added; early needs-dev-note removed
Note: See TracTickets for help on using tickets.