WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11559 closed enhancement (fixed)

split HTTP Class and Functions

Reported by: dd32 Owned by: dd32
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Optimization Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

Slight "optimization" i'm thinking about.

Like is done with SimplePie, Only include the Class files when the API functions are called..

ie.

function wp_remote_get() {
  include_once ABDPATH . '/wp-includes/class.http.php';
  ....
}

just to prevent loading the entire HTTP class on pageloads which do not need it..

I'm not sure on the performance issues related, ie. is another file going to make it slower than just parsing the entire file..

Its done with the Feeds and a few Backpress files i believe..

What do people think?

Attachments (1)

11559.diff (145.1 KB) - added by sivel 4 years ago.
First attempt at separation, if indeed we are interested in doing this.

Download all attachments as: .zip

Change History (19)

comment:1 Denis-de-Bernardy4 years ago

I use the approach in each and every one of my plugins. The key front end stuff is always loaded. Less used stuff is only loaded on a per-need basis. It works great.

For WP files, I can picture us doing the same for:

  • HTTP, SimplePie, Mailer, IXR, etc. (the various class-*.php files)
  • Localization if the language is the WP default

Localization stuff could be discarded entirely, and treated as a set of functions that merely return the string when the language is the one used by default in WP.

comment:2 Denis-de-Bernardy4 years ago

  • Keywords 2nd-opinion removed
  • Type changed from defect (bug) to enhancement

comment:3 dd324 years ago

SimplePie, Mailer, IXR, etc.

I think they are already conditionally loaded. Internationalisation isnt though.

comment:4 jacobsantos4 years ago

Well, actually it would make it slower in some cases and a little bit faster in others. Using require_once will offset any improvements <5.2.x.

Since I use an opcode cache, I would be upset if this was done.

comment:5 dd324 years ago

Since I use an opcode cache, I would be upset if this was done.

Wouldnt that mean next-to-nothing in terms of performance then? The file is already pre-'compiled' in memory. Its much the same as the rest of WordPress only loading the files needed..

Potentially, if you're worried about *_once overheads, We could use if ( ! class_exists('WP_HTTP') ) include .... or a similar 'faster' check..

For a function which should only end up being called once every couple of hundred page loads (as a minimum hopefully)..

comment:6 hakre4 years ago

unless we can not use SPL autoload, we must stick with that stinky require_once() for stabilities sake I suggest. For those who know what they do can switch over to include_once or include even.

comment:7 jacobsantos4 years ago

Well, in general when you include a file around a conditional the opcode caching extensions may or may not keep that file cached, since it doesn't know if it will stay part of the script with each request.

The problem is that also that the PHP engine has to go back to the file, load it, parse it, compile it and then run it after stopping the execution. Each time this is done, it adds overhead. Albeit a small amount relative to the machine, but still it can be noticeable when you do it many times.

Well, that said, BackPress already separates the functions and classes. I do not like it at all, but eventually that is going to find its way back into WordPress.

comment:8 follow-up: dd324 years ago

in general when you include a file around a conditional the opcode caching extensions may or may not keep that file cached

Interesting, I didnt realise that. If you wanted to forcably include it for caching reasons, that'd be a easy enough work around though. Since plugins are included inside a if conditional, does that mean something similar? or are they smart enough to realise it in that case?

comment:9 in reply to: ↑ 8 jacobsantos4 years ago

Replying to dd32:

Interesting, I didnt realise that. If you wanted to forcably include it for caching reasons, that'd be a easy enough work around though. Since plugins are included inside a if conditional, does that mean something similar? or are they smart enough to realise it in that case?

Well, I think that is the problem in any case. If the opcode cache extension doesn't know which path is going to be taken, then well it can do two things really. It can load the file and cache it where the require or include is. However, since we are also talking about a loop, I'm not sure it would even bother, since it would have to break the loop and it can't do that.

A simple conditional might be okay, but I believe the problem is that with conditionals, it is unknown when they will come into affect, so if they cache the opcodes in the file, they'll be keeping extra opcodes that may not be needed in the next request and slow down execution.

Of course, I could be wrong and they may just store the opcodes per file instead of all together. I believe PHP executions is to store all of the opcodes together, so in this case it would be my fear, but some opcode caching extensions extend the PHP engine further and might be able to do it per file and per modified time (since given that opcode caching extensions also keep track of modified file time or just expires the files after a certain time).

What I do know is that conditionals trip up opcode caching extensions. To what extent will have to be asked of someone who works on one of them. I do offer some conjecture until that time.

sivel4 years ago

First attempt at separation, if indeed we are interested in doing this.

comment:10 sivel4 years ago

  • Cc matt@… added
  • Keywords has-patch needs-testing dev-feedback added

I had not yet seen a patch for this, so if we are still interested see the attached patch. Seems to work ok for me. Would likely need more testing.

comment:11 sivel4 years ago

Since we have the chance I am wondering if perhaps we shouldn't rename to reflect the naming conventions used in BackPress. The current patch is close, however we were thinking of naming as http.php for the functions and class-http.php for the classes.

If we follow BackPress it would move to functions.wp-http.php and class.wp-http.php. The only thing we need to think of here is breaking things that use BackPress.

Thoughts? I'll try to ping Peter (westi) with this when I see him next.

comment:12 nacin4 years ago

I like moving the classes out, leaving the functions in http.php, and modifying _wp_http_get_object() to account for the need to include the new file. I don't like renaming http.php in case that is relied on specifically.

I see two options, class-http.php and class.wp-http.php, that's up to westi's call.

sivel, if you want to diff only _wp_http_get_object() (though even that probably isn't necessary), then the committer can handle the cut-paste job.

comment:13 sivel4 years ago

Yeah, really the 1 line modification for _wp_http_get_object() doesn't need to be in a patch. It's already in the patch attached.

comment:14 dd324 years ago

  • Owner set to dd32
  • Status changed from new to accepted

comment:15 dd324 years ago

For now, I'm going to go with the WordPress classes filename convention.

I think we can sync it if/when we include Backpress as an external or otherwise.

comment:16 automattor4 years ago

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

(In [13274]) Split WP_Http classes into separate file. Fixes #11559

comment:17 westi4 years ago

This looks great.

As far as I am concerned the naming you have picked is fine.

For me the important things are:

  • Consistency with other files in this project
  • That the name makes it obvious what is in the file - i.e. the class- one has the class in it.

comment:18 hakre4 years ago

Related: #14814

Note: See TracTickets for help on using tickets.