Make WordPress Core

Opened 9 years ago

Closed 8 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:
PR Number:


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

Download all attachments as: .zip

Change History (22)

9 years ago

#1 @scribu
9 years ago

Sort of related: #16736

#2 @nacin
9 years ago

  • Milestone changed from Awaiting Review to 3.2

#3 @nacin
9 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.

9 years ago

#4 @duck_
9 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.

#5 @scribu
9 years ago

  • Keywords commit added

Looks good to me.

#6 @ryan
9 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.

#7 @nacin
9 years ago

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

#8 @nacin
9 years ago

Looking good at a glance.

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

9 years ago

Lose unnecessary isset() checks.

#9 @ryan
9 years ago

In [18466]:

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

#10 @ryan
9 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

#12 @dnljms
8 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.

8 years ago

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

8 years ago

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

#13 @dnljms
8 years ago

  • Cc dnljms added

#14 @ryan
8 years ago

  • Priority changed from normal to high

#15 @duck_
8 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.

#16 @duck_
8 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.