Make WordPress Core

Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#56924 closed defect (bug) (fixed)

get_attached_file(): New call to path_join() can have poor performance on NFS file systems

Reported by: mreishus's profile mreishus Owned by:
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: Media Keywords: commit dev-reviewed
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In changeset [53934], this change was made to get_attached_file():

-                       $file = $uploads['basedir'] . "/$file";
+                       $file = path_join( $uploads['basedir'], $file );

Each call to path_join() calls path_is_absolute(), which calls realpath(). Most of the time this is not a problem, as realpath runs very fast, on the order of 0.003 milliseconds. However, if your uploads directory is on an NFS fileshare, I'm seeing calls to realpath() be 1000-2000x slower, or on the order of 2ms-4ms.

One call to a 4ms realpath() is not an issue, but get_attached_file() can run on each post in a list in some APIs, causing significant slowdowns. Additionally, the path_join() file can be called on the same file several times in a row, each creating a new call to realpath() without caching or memoizing the results.

Rough debugging code:

  • src/wp-includes/post.php

    diff --git a/src/wp-includes/post.php b/src/wp-includes/post.php
    index a67d7a79a1..f728e80a7a 100644
    a b function get_attached_file( $attachment_id, $unfiltered = false ) { 
    728728               $uploads = wp_get_upload_dir();
    729729
    730730               if ( false === $uploads['error'] ) {
     731
     732                       $url = "http://$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
     733                       error_log("path join: Called from $url");
     734                       $t1 = microtime( true );
     735
    731736                       $file = path_join( $uploads['basedir'], $file );
     737
     738                       $t2 = microtime( true );
     739                       $e = ( $t2 - $t1 ) * 1000;
     740                       error_log("Elasped $e");
     741                       error_log( print_r( [
     742                               $uploads['basedir'],
     743                               $file,
     744                       ], true ) );
    732745               }
    733746       }

Have a site with some posts and some images in those posts. Browse around and look for the calls, for example, while visiting media library. Note that the speeds will appear fast unless there are network-mounted filesystems using NFS being queried.

Change History (11)

#1 @desrosj
19 months ago

  • Milestone changed from Awaiting Review to 6.1

Thanks for the report @mreishus. Moving to the 6.1 milestone to investigate.

#2 @SergeyBiryukov
19 months ago

  • Description modified (diff)

This ticket was mentioned in Slack in #hosting-community by mike. View the logs.


19 months ago

#4 @antpb
19 months ago

In 54712:

Media: Reverts get_attached_file() changes for normalized Windows paths.

Based on feedback from network storage configurations there was a noticed slowdown due to the usage of the path_join() function. This needs more time to find a workaround.

Follow-up to [53934].
Props mreishus, SergeyBiryukov, desrosj, mikeschroder.
Reverts [53934].
See #56924.

#5 @davidbaumwald
19 months ago

  • Keywords commit dev-feedback added

Flagging for a revert in the 6.1 branch as well, pending a second committer's review.

#6 @kirasong
19 months ago

  • Keywords dev-reviewed added; dev-feedback removed

I left a note to this extent in Slack, but +1 for reverting / backporting for 6.1.

This will give time for folks to investigate / benchmark without time pressure, and hopefully find a better performing solution for the original issue in #36308.

#7 @antpb
19 months ago

In 54713:

Media: Reverts get_attached_file() changes for normalized Windows paths.

Based on feedback from network storage configurations there was a noticed slowdown due to the usage of the path_join() function. This needs more time to find a workaround.

Follow-up to [53934].
Props mreishus, SergeyBiryukov, desrosj, mikeschroder.
Reverts [53934] in the 6.1 Branch.
See #56924.

#8 @desrosj
19 months ago

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

I've reopened #36308 to continue working on a better solution for this.

Since this ticket is specifically about the regression in 6.1, I'm going to close this out.

#9 @desrosj
19 months ago

  • Component changed from General to Media

This ticket was mentioned in Slack in #core by desrosj. View the logs.


19 months ago

#11 @kirasong
19 months ago

Additional props to @juberstine @desmith for quick last-minute feedback on this ticket. Thank you!!

Note: See TracTickets for help on using tickets.