split HTTP Class and Functions

[dd32]

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?

[Denis-de-Bernardy]

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.

[dd32]

SimplePie, Mailer, IXR, etc.

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

[jacobsantos]

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.

[dd32]

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)..

[hakre]

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.

[jacobsantos]

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.

[dd32]

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?

[jacobsantos]

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.

[sivel]

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

[sivel]

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.

[sivel]

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.

[nacin]

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.

[sivel]

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.

[dd32]

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.

[automattor]

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

[westi]

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.

[hakre]

Related: #14814