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

[mreishus]

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

@@ -728 +728 @@
                $uploads = wp_get_upload_dir();
 
                if ( false === $uploads['error'] ) {
+
+                       $url = "http://$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
+                       error_log("path join: Called from $url");
+                       $t1 = microtime( true );
+
                        $file = path_join( $uploads['basedir'], $file );
+
+                       $t2 = microtime( true );
+                       $e = ( $t2 - $t1 ) * 1000;
+                       error_log("Elasped $e");
+                       error_log( print_r( [
+                               $uploads['basedir'],
+                               $file,
+                       ], true ) );
                }
        }

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.

[desrosj]

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

[antpb]

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.

[davidbaumwald]

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

[kirasong]

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.

[antpb]

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.

[desrosj]

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.

[kirasong]

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