WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 10 years ago

#10779 closed enhancement (fixed)

optimised unzip_file()

Reported by: dd32 Owned by: dd32
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Filesystem API Keywords: needs-testing featured
Focuses: Cc:

Description (last modified by dd32)

Patch contains

  • Optimised unzip_file() function
    • Reduced the number of IO operations required (mainly is_dir())
    • Simplifies logic to make reading easier.

---
Also note, I played with extracting a file at a time from the ZIP, and writing to the filesystem directly, While it decreased the memory usage from ~12MB to 3MB for the entire unzip operation, It increased the operation time from 5s to 70s, So i've left it to consume memory in order to speed things up.

Attachments (2)

10779.diff (11.6 KB) - added by dd32 11 years ago.
10779.2.diff (11.5 KB) - added by dd32 11 years ago.
s/for/foreach on a loop for readability

Download all attachments as: .zip

Change History (18)

@dd32
11 years ago

#1 @dd32
11 years ago

  • Description modified (diff)

@dd32
11 years ago

s/for/foreach on a loop for readability

#2 @ryan
11 years ago

It might be too late now to put the unzip_file() changes into 2.9. Can we separate out the phpdoc changes?

#3 @dd32
11 years ago

  • Description modified (diff)
  • Keywords needs-testing added
  • Milestone changed from 2.9 to 3.0
  • Summary changed from file.php: PHPDoc + optimised unzip_file() to optimised unzip_file()

The phpdoc has gone in with another ticket, I forgot i had submitted it here already.

#4 @dd32
11 years ago

  • Status changed from new to accepted

I'm going to refresh this patch, and do furthur testing of the changes in the near future.

#5 @Denis-de-Bernardy
11 years ago

  • Cc Denis-de-Bernardy added

#6 @hakre
11 years ago

So to wait with tests?

#7 @dd32
11 years ago

...What?

Yes, The updated patch will be resubmitted once i retest its working with the latest changes to the codebase.

#8 @dd32
11 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

#9 @dd32
10 years ago

Just did some benchmarks on the code.

First item is an array of the number of times each function is called. Then a total of the calls:

Before:
array(6) {
  ["connect"]=>
  int(1)
  ["mkdir"]=>
  int(83)
  ["put_contents"]=>
  int(753)
  ["exists"]=>
  int(836)
  ["chmod"]=>
  int(836)
  ["is_dir"]=>
  int(1754)
}
Total: 4263

After:
array(6) {
  ["connect"]=>
  int(1)
  ["is_dir"]=>
  int(3)
  ["mkdir"]=>
  int(83)
  ["put_contents"]=>
  int(753)
  ["exists"]=>
  int(836)
  ["chmod"]=>
  int(836)
}
Total: 2512

is_dir() is expensive on FTP systems, and all of those exists() calls can be removed by letting chmod() either suceed, or fail. instead of checking if it'll fail (if it exists) and then letting it either suceed or fail.

#10 @dd32
10 years ago

Oh, that was extracting the WordPress 2.9.1 archive on a system using the Direct method..

#11 @dd32
10 years ago

(In [12794]) Optimised unzip_file(), reduces is_dir() IO calls. See #10779

#12 @dd32
10 years ago

  • Keywords needs-testing added; needs-patch removed

Commited this for now, Will see how it fares for others.

For those wanting to test it, just run a few Plugin installs and upgrades and see how you go.

#13 @Denis-de-Bernardy
10 years ago

  • Keywords bug-hunt added

#14 @dd32
10 years ago

See #11979, Broke it for one person, with one particular archive.. Havnt yet been able to reproduce it

#15 @Denis-de-Bernardy
10 years ago

  • Keywords featured added; bug-hunt removed

#16 @dd32
10 years ago

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

Closing as fixed. Open new tickets for any issues encountered.

Note: See TracTickets for help on using tickets.