WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#17177 closed defect (bug) (fixed)

Optimize parse_request for the home page (empty request)

Reported by: duck_ Owned by: ryan
Milestone: 3.3 Priority: high
Severity: normal Version:
Component: Rewrite Rules Keywords: has-patch commit 3.3-early
Focuses: Cc:

Description

When you visit a site's home page the request (WP::$request) is empty. However we still try to match it against every rewrite rule. By default none of the rules will match. This can be particularly bad when verbose page rules are enabled and WordPress is attempting 700+ regex matches (~50 pages) or more for especially large page oriented sites on an empty string.

The only reason against breaking from the loop like we do for wp-app.php in this case I could think of is if a plugin has added an extra rule to match ^$.[1] If we want to allow this then maybe manually check for a rule == $ and still break early.

[1] This can currently be blocked by #17176 as ()(/[0-9]+)?/?$ matches an empty string

Attachments (6)

17177.diff (618 bytes) - added by duck_ 3 years ago.
17177.2.diff (2.8 KB) - added by duck_ 3 years ago.
17177.3.diff (3.2 KB) - added by ryan 3 years ago.
Lose unnecessary isset() checks.
request.patch (482 bytes) - added by dnljms 2 years ago.
Fixes the check for when $req_uri is empty, but a match is still possible.
request2.patch (948 bytes) - added by dnljms 2 years ago.
I think this is a better patch, but it's a bigger change.
17177.request_match.diff (465 bytes) - added by duck_ 2 years ago.

Download all attachments as: .zip

Change History (22)

duck_3 years ago

comment:1 scribu3 years ago

Sort of related: #16736

comment:2 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.2

comment:3 nacin3 years ago

I really like this. I wouldn't mind checking for ^$ just in case though. I can't see anyone doing that, but still.

duck_3 years ago

comment:4 duck_3 years ago

A lot more code movement this time so please read and test carefully.

Moved the empty and wp-app.php checks out of the loop, no need to continually run them if they don't pass the first time. Not really sure why but I don't really like the $matches = array(''); we could add a preg_match there just because, thoughts?

Tested with and without:

add_action( 'init', function() { add_rewrite_rule( '$', 'index.php?p=1' ); });

Passes all the rewrite/query tests as well as my random testing. Might want to double check some of the lesser used rewrite structures, e.g. with slug prefix, pathinfo, etc.

comment:5 scribu3 years ago

  • Keywords commit added

Looks good to me.

comment:6 ryan3 years ago

  • Keywords 3.3-early added
  • Milestone changed from 3.2 to Future Release

Looks good but we waited too long for 3.2. Marking for early 3.3.

comment:7 nacin3 years ago

  • Component changed from Performance to Rewrite Rules
  • Milestone changed from Future Release to 3.3

comment:8 nacin3 years ago

Looking good at a glance.

No need to check isset() before unset().

ryan3 years ago

Lose unnecessary isset() checks.

comment:9 ryan3 years ago

In [18466]:

Optimize parse_request for the home page. Props duck_. see #17177

comment:10 ryan3 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [18467]:

Explain the empty req_uri branch. Props duck_. fixes #17177

comment:12 dnljms2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change breaks when using IIS 6 with IIRF (I know that's a bit odd) as $req_uri is empty for rewritten urls. If you look at the loop, when $req_uri is empty it just uses the existing value of $request_match so there can be a match for empty $req_uri. I'll attach a patch.

dnljms2 years ago

Fixes the check for when $req_uri is empty, but a match is still possible.

dnljms2 years ago

I think this is a better patch, but it's a bigger change.

comment:13 dnljms2 years ago

  • Cc dnljms added

comment:14 ryan2 years ago

  • Priority changed from normal to high

comment:15 duck_2 years ago

Good catch, thanks.

Not sure what happened between my original ticket where I mention WP::$request specifically and the patches which start checking $req_uri. I think we can just replace $req_uri with $request_match in the empty() call in the if-statement. I think this would be clearer than request2.patch (which also unnecessarily sets $request_match on every loop iteration) and, unlike request.patch, use the correct variable name.

duck_2 years ago

comment:16 duck_2 years ago

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

In [19280]:

Use correct variable. We preg_match() against $request_match, so that's what should be checked with empty() too. Fixes #17177.

Note: See TracTickets for help on using tickets.