WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#10913 closed defect (bug) (fixed)

optimizations are much needed in the FTP filesystem

Reported by: Denis-de-Bernardy Owned by: dd32
Milestone: 3.2 Priority: high
Severity: normal Version: 2.8.4
Component: Filesystem API Keywords: has-patch
Focuses: Cc:

Description

I added some extra tracking in the class to understand why it was timing out on a customer's server. the last message is what verbose mode outputs:

exists(/home/dietp4fn/public_html/)
is_file(/)
exists(/)
is_dir(/)
is_file(/)
exists(/)
is_dir(/)
is_dir(/wp-includes)
exists(/home/dietp4fn/public_html/wp-content)
is_file(/)
exists(/)
is_dir(/)
Found /wp-content

I suspect that is_file(), is_dir(), and exists() could all use a cache, and that is_dir() could use an optimization along the lines of:

if ( preg_match("/\.(php|css|js|txt|html|je?pg|png|gif|po|pot|mo)$/i", $file )
  return true;

Attachments (1)

10913.diff (6.6 KB) - added by aldenta 3 years ago.
Pass known information to copy and delete to save lookup operations

Download all attachments as: .zip

Change History (25)

comment:1 dd325 years ago

Heres some code i use for debugging things (PHP5 only), requires you to rename the class you're debugging (dont forget the construct) but works nicely.

class WP_Filesystem_FTPext {
	var $a;
	var $handle;
	function __construct($args) {
		$this->a = new _WP_Filesystem_FTPext($args);
		$this->handle = fopen(ABSPATH . 'debug.txt', 'w+');
	}
	function __call($name, $args) {
		fwrite($this->handle, 'Calling ' . $name . '(' . implode(', ', $args) . ') = ');
		$res = call_user_func_array( array(&$this->a, $name), $args);
		if ( is_bool($res) )
			$hres = '(bool)' . ($res ? '1' : '0');
		else
			$hres = $res;
		fwrite($this->handle, '<pre>' . htmlentities(print_r($hres, true)) . "\r\n\r\n\r\n</pre>");
		return $res;
	}
	function __get($name) {
		return $this->a->$name;
	}
	function __set($name, $value) {
		return ($this->a->$name = $value);
	}
}

You're right, some of the methods could definately use a cache, But sometimes the cache needs to be erased.

I'd also suggest you try using trunk, i think theres been a few optimizations to certain parts..

comment:2 Denis-de-Bernardy5 years ago

cross-referencing a few notes I added to another ticket:

http://core.trac.wordpress.org/ticket/10611#comment:6

comment:3 ryan4 years ago

  • Milestone changed from 2.9 to Future Release

comment:5 follow-up: dd324 years ago

  • Status changed from new to accepted

This is on the books to be looked at after #10779, which will reduce some of the calls.

if ( preg_match("/\.(php|css|js|txt|html|je?pg|png|gif|po|pot|mo)$/i", $file )
  return true;

::is_file() is used to determine if its a file, and it exists, i believe, not just if the path looks like a file.

comment:6 in reply to: ↑ 5 Denis-de-Bernardy4 years ago

Replying to dd32:

::is_file() is used to determine if its a file, and it exists, i believe, not just if the path looks like a file.

Correct. We'd need to make sure the file exists. To do that, though, we might as well use the local file system, no matter which FS we use.

comment:7 Denis-de-Bernardy4 years ago

  • Keywords bug-hunt added

comment:8 Denis-de-Bernardy4 years ago

  • Keywords featured added; bug-hunt removed

comment:9 dd324 years ago

(In [12998]) Reduce code duplication, Formatting cleanups, Reduce Chmod IO calls. See #10913 for excess IO calls.

aldenta3 years ago

Pass known information to copy and delete to save lookup operations

comment:10 aldenta3 years ago

  • Keywords has-patch added

comment:11 dd323 years ago

  • Keywords 3.2-early added; featured removed

attachment 10913.diff added

Have been talking with aldenta about this this evening, The changes in the patch are very basic and simple, but decrease the amount of FTP operations needed by a huge margin. This also caused a site which previously took >5 minutes to complete sub-30seconds.
There are likely some modifications which can be made to the ftpsockets class as well to speed things up.

Note: The changes here only affect the FTPExt class, the Direct, FTPSockets and SSH classes would need to be updated to support the same set of arguments.

comment:12 stevem3 years ago

I have a number of blogs running on a server with WHM and Pure-FTPd which seems to to be type most affected. Automatic updates hadn't worked since upgrading to version 3.0.

I have now tested the suggested changes.

In order to update from 3.0.4 to 3.0.5 I changed just two files, wp-admin/includes/file.php and wp-admin/includes/class-wp-filesystem-ftpext.php according to 10913.diff, uploaded them and the automatic update ran smoothly and quickly (sub-30 seconds).

Version 0, edited 3 years ago by stevem (next)

comment:13 dd323 years ago

I have now tested the suggested changes.

Thanks for testing!

I'll be getting these changes in as soon as 3.1 is out the door for 3.2, I'll probably bundle the changes into a Plugin as well to help out users for the duration of 3.1

comment:14 stevem3 years ago

More good news :) Uploading just those two changed files again (wp-admin/includes/file.php and wp-admin/includes/class-wp-filesystem-ftpext.php) allowed the update to 3.1 to proceed correctly and quickly.

comment:15 westi3 years ago

  • Cc westi added
  • Keywords commit added
  • Priority changed from normal to high

Plugin: http://wordpress.org/extend/plugins/ftp-upgrade-fix/ by @aldenta.

Bumping priority on this to ensure we don't miss including it in 3.2

comment:16 dd323 years ago

just while I remember this, The other thing I played around with here was changing copy_dir() to use ::move() instead of ::copy(), which reduces the amount of read/writes (both of IO and reading into PHP Memory). Played with, but didn't benchmark.

comment:17 dd323 years ago

  • Milestone changed from Future Release to 3.2

comment:18 dd323 years ago

(In [17525]) Optimisations to WP_Filesystem; Pass known information to called functions. Props aldenta (John Ford) for investigation and patch. See #10913

comment:19 dd323 years ago

Just a note, [17525] is not entirely the same as 10913.diff, I removed a few extra calls and patched a few functions to behave like the rest (direct::copy for example).

comment:20 westi3 years ago

  • Keywords close added; 3.2-early commit removed

Dion - can we close this ticket now?

comment:21 nacin3 years ago

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

comment:22 Denis-de-Bernardy3 years ago

@Nacin: I haven't tested, and I may have missed a commit here or there, but I'm not entirely convinced this is fixed. Based on the two commits in this ticket, it seems to me that we'd still end up with things like:

is_file(/)
exists(/)
is_dir(/)
is_file(/)
exists(/)
is_dir(/)

comment:23 follow-up: dd323 years ago

  • Keywords close removed

Denis: We'll probably end up with that in a few locations still, however, this dramatically reduces the number of them, While benchmarking data isn't on this ticket, in some bad cases it reduced it from ~10,000 operations to less than 5,000 operations, many of which were duplicated is_file() & is_dir() calls.

Without adding an entire Filesystem caching layer which takes into account files being unlink'd and whatnot, this is the best that we'll have in the short term. (so I agree that this ticket should be closed)

comment:24 in reply to: ↑ 23 Denis-de-Bernardy3 years ago

Replying to dd32:

Without adding an entire Filesystem caching layer which takes into account files being unlink'd and whatnot.

... which is incidentally what the ticket was about... :-)

I'm seeing there's a filesystem_method_file filter though, so I'll be able to extend the needed classes if I ever feel like giving it a shot using a plugin...

Note: See TracTickets for help on using tickets.