WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#31066 new defect (bug)

wp_upload_dir calls wp_mkdir_p on each invocation

Reported by: rmccue Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: has-patch needs-unit-tests needs-testing close
Focuses: performance Cc:

Description

Every time wp_upload_dir is called, wp_mkdir_p is also called. For normal filesystems, this is not a huge issue, but using external filesystems (S3, etc) potentially means an external request on every call.

Ideally, I'd actually like to avoid calling wp_mkdir_p at all there. S3 (along with a few other external FSes) doesn't actually have a concept of directories; rather they're just part of the filename. Adding a upload_dir_requires_creation filter to wp_upload_dir would help with this, as plugins could then switch this off if they know the underlying FS doesn't need it, or they already handle it themselves.

Attachments (1)

31066.diff (2.6 KB) - added by kovshenin 3 years ago.

Download all attachments as: .zip

Change History (6)

#1 @SergeyBiryukov
3 years ago

Previously noted in [30658].

@kovshenin
3 years ago

#2 @kovshenin
3 years ago

  • Keywords has-patch added

I just witnessed a profile of a page that rendered close to a hundred thumbnails, which is not that many if you think about it, but attempting to create an uploads directory 300 times in a single request is definitely overkill, not only for high-latency and slow filesystems.

In 31066.diff I added a new $mkdir argument to wp_upload_dir(). Initially I wanted it to default to false, but given that many plugins expect the function to create the uploads directory, I settled on true by default. Adjusted a couple of media functions which clearly shouldn't be creating any directories.

Also added a wp_mkdir_p_override filter to allow plugins to override the whole function, or disable it completely with __return_true.

#3 @prettyboymp
2 years ago

I've been running into performance issues with an image heavy site on a setup with the uploads dir mounted on networked storage. I traced it down to the wp_mkdir_p and found this ticket. I've been running @kovshenin's 31066 patch for a few days now and haven't had any issues since.

One improvement that I think could be made to help even further improve performance for plugins that use wp_upload_dir() would be to store a static result from it's wp_mkdir_p call since PHP won't cache the file_exist calls.

#4 @johnbillion
2 years ago

  • Focuses performance added
  • Keywords needs-unit-tests needs-testing added
  • Milestone changed from Awaiting Review to Future Release

#5 @joemcgill
2 years ago

  • Keywords close added

As of r36565, we can pass a second parameter to wp_upload_dir() to skip the call to wp_mkdir_p() or make use of a new function, wp_get_upload_dir(), which essentially does the same. Additionally, wp_upload_dir() now saves the value returned by wp_mkdir_p() to persistent cache so it only runs once per page load (at most).

Also, r36569 updates several internal media functions which are only used to display images so they now use the new wp_get_upload_dir() function to avoid the wp_mkdir_p() call.

See #34359

Note: See TracTickets for help on using tickets.