Changeset 278641 in webkit

Timestamp:

Jun 8, 2021, 7:26:34 PM (5 years ago)

Author:

Ben Nham

Message:

Use access instead of stat in some filesystem functions
​https://bugs.webkit.org/show_bug.cgi?id=226667

Reviewed by Chris Dumez.

We are spending a bit more time in WTF::FileSystemImpl functions after the move to
std::filesystem (​https://bugs.webkit.org/show_bug.cgi?id=225255). In particular, several
std::filesystem functions (like the ones called by fileExists and makeAllDirectories) prefer
to use stat() over access() for file existence checks. Since our sandbox has a fast path for
access(path, F_OK) but not for stat, we ended up spending more time in sandbox evaluation in
the kernel after the move to std::filesystem.

Note that the two checks don't do exactly the same thing. access(path, F_OK) only checks for
path existence, while stat(path) additionally fetches metadata, which requires checking the
file-read-metadata permission. But in practice our code was written to be fine with just
checking for existence.

To work around this, I've re-introduced some of the old WTF::FileSystemImpl functions from
FileSystemPosix.cpp. They are the ones that are called by NetworkCache, which seems to be
the biggest consumer of these functions. The rest of the functions are still implemented
using std::filesystem.

• wtf/FileSystem.cpp:

(WTF::FileSystemImpl::fileExists):
(WTF::FileSystemImpl::deleteFile):
(WTF::FileSystemImpl::makeAllDirectories):
(WTF::FileSystemImpl::pathByAppendingComponent):
(WTF::FileSystemImpl::pathByAppendingComponents):

• wtf/PlatformEnableCocoa.h:
• wtf/posix/FileSystemPOSIX.cpp:

(WTF::FileSystemImpl::fileExists):
(WTF::FileSystemImpl::deleteFile):
(WTF::FileSystemImpl::makeAllDirectories):
(WTF::FileSystemImpl::pathByAppendingComponent):
(WTF::FileSystemImpl::pathByAppendingComponents):

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/FileSystem.cpp (modified) (4 diffs)
• wtf/PlatformEnableCocoa.h (modified) (1 diff)
• wtf/posix/FileSystemPOSIX.cpp (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2021-06-08  Ben Nham  <nham@apple.com>
+
+        Use access instead of stat in some filesystem functions
+        https://bugs.webkit.org/show_bug.cgi?id=226667
+
+        Reviewed by Chris Dumez.
+
+        We are spending a bit more time in WTF::FileSystemImpl functions after the move to
+        std::filesystem (https://bugs.webkit.org/show_bug.cgi?id=225255). In particular, several
+        std::filesystem functions (like the ones called by fileExists and makeAllDirectories) prefer
+        to use stat() over access() for file existence checks. Since our sandbox has a fast path for
+        access(path, F_OK) but not for stat, we ended up spending more time in sandbox evaluation in
+        the kernel after the move to std::filesystem.
+
+        Note that the two checks don't do exactly the same thing. access(path, F_OK) only checks for
+        path existence, while stat(path) additionally fetches metadata, which requires checking the
+        file-read-metadata permission. But in practice our code was written to be fine with just
+        checking for existence.
+
+        To work around this, I've re-introduced some of the old WTF::FileSystemImpl functions from
+        FileSystemPosix.cpp. They are the ones that are called by NetworkCache, which seems to be
+        the biggest consumer of these functions. The rest of the functions are still implemented
+        using std::filesystem.
+
+        * wtf/FileSystem.cpp:
+        (WTF::FileSystemImpl::fileExists):
+        (WTF::FileSystemImpl::deleteFile):
+        (WTF::FileSystemImpl::makeAllDirectories):
+        (WTF::FileSystemImpl::pathByAppendingComponent):
+        (WTF::FileSystemImpl::pathByAppendingComponents):
+        * wtf/PlatformEnableCocoa.h:
+        * wtf/posix/FileSystemPOSIX.cpp:
+        (WTF::FileSystemImpl::fileExists):
+        (WTF::FileSystemImpl::deleteFile):
+        (WTF::FileSystemImpl::makeAllDirectories):
+        (WTF::FileSystemImpl::pathByAppendingComponent):
+        (WTF::FileSystemImpl::pathByAppendingComponents):
+
 2021-06-08  Devin Rousso  <drousso@apple.com>
 

trunk/Source/WTF/wtf/FileSystem.cpp

@@ -523 +523 @@
 #if HAVE(STD_FILESYSTEM) || HAVE(STD_EXPERIMENTAL_FILESYSTEM)
 
-bool fileExists(const String& path)
-{
-    std::error_code ec;
-    // exists() returns false on error so no need to check ec.
-    return std::filesystem::exists(toStdFileSystemPath(path), ec);
-}
-
-bool deleteFile(const String& path)
-{
-    std::error_code ec;
-    auto fsPath = toStdFileSystemPath(path);
-
-    auto fileStatus = std::filesystem::symlink_status(fsPath, ec);
-    if (ec || fileStatus.type() == std::filesystem::file_type::directory)
-        return false;
-
-    // remove() returns false on error so no need to check ec.
-    return std::filesystem::remove(fsPath, ec);
-}
-
 bool deleteEmptyDirectory(const String& path)
 {
@@ -598 +578 @@
 }
 
-bool makeAllDirectories(const String& path)
-{
-    std::error_code ec;
-    std::filesystem::create_directories(toStdFileSystemPath(path), ec);
-    return !ec;
-}
-
 std::optional<uint64_t> volumeFreeSpace(const String& path)
 {
@@ -729 +702 @@
 }
 
-String pathByAppendingComponent(const String& path, const String& component)
-{
-    return fromStdFileSystemPath(toStdFileSystemPath(path) / toStdFileSystemPath(component));
-}
-
-String pathByAppendingComponents(StringView path, const Vector<StringView>& components)
-{
-    auto fsPath = toStdFileSystemPath(path);
-    for (auto& component : components)
-        fsPath /= toStdFileSystemPath(component);
-    return fromStdFileSystemPath(fsPath);
-}
-
 Vector<String> listDirectory(const String& path)
 {
@@ -755 +715 @@
 }
 
+#if !ENABLE(FILESYSTEM_POSIX_FAST_PATH)
+
+bool fileExists(const String& path)
+{
+    std::error_code ec;
+    // exists() returns false on error so no need to check ec.
+    return std::filesystem::exists(toStdFileSystemPath(path), ec);
+}
+
+bool deleteFile(const String& path)
+{
+    std::error_code ec;
+    auto fsPath = toStdFileSystemPath(path);
+
+    auto fileStatus = std::filesystem::symlink_status(fsPath, ec);
+    if (ec || fileStatus.type() == std::filesystem::file_type::directory)
+        return false;
+
+    // remove() returns false on error so no need to check ec.
+    return std::filesystem::remove(fsPath, ec);
+}
+
+bool makeAllDirectories(const String& path)
+{
+    std::error_code ec;
+    std::filesystem::create_directories(toStdFileSystemPath(path), ec);
+    return !ec;
+}
+
+String pathByAppendingComponent(const String& path, const String& component)
+{
+    return fromStdFileSystemPath(toStdFileSystemPath(path) / toStdFileSystemPath(component));
+}
+
+String pathByAppendingComponents(StringView path, const Vector<StringView>& components)
+{
+    auto fsPath = toStdFileSystemPath(path);
+    for (auto& component : components)
+        fsPath /= toStdFileSystemPath(component);
+    return fromStdFileSystemPath(fsPath);
+}
+
+#endif
+
 #endif // HAVE(STD_FILESYSTEM) || HAVE(STD_EXPERIMENTAL_FILESYSTEM)
 

trunk/Source/WTF/wtf/PlatformEnableCocoa.h

@@ -221 +221 @@
 #endif
 
+#if !defined(ENABLE_FILESYSTEM_POSIX_FAST_PATH)
+#define ENABLE_FILESYSTEM_POSIX_FAST_PATH 1
+#endif
+
 #if !defined(ENABLE_FILTERS_LEVEL_2)
 #define ENABLE_FILTERS_LEVEL_2 1

trunk/Source/WTF/wtf/posix/FileSystemPOSIX.cpp

@@ -236 +236 @@
 }
 
+#if ENABLE(FILESYSTEM_POSIX_FAST_PATH)
+
+bool fileExists(const String& path)
+{
+    return access(fileSystemRepresentation(path).data(), F_OK) != -1;
+}
+
+bool deleteFile(const String& path)
+{
+    // unlink(...) returns 0 on successful deletion of the path and non-zero in any other case (including invalid permissions or non-existent file)
+    bool unlinked = !unlink(fileSystemRepresentation(path).data());
+    if (!unlinked && errno != ENOENT)
+        LOG_ERROR("File failed to delete. Error message: %s", strerror(errno));
+
+    return unlinked;
+}
+
+bool makeAllDirectories(const String& path)
+{
+    auto fullPath = fileSystemRepresentation(path);
+    if (!access(fullPath.data(), F_OK))
+        return true;
+
+    char* p = fullPath.mutableData() + 1;
+    int length = fullPath.length();
+    if (p[length - 1] == '/')
+        p[length - 1] = '\0';
+    for (; *p; ++p) {
+        if (*p == '/') {
+            *p = '\0';
+            if (access(fullPath.data(), F_OK)) {
+                if (mkdir(fullPath.data(), S_IRWXU))
+                    return false;
+            }
+            *p = '/';
+        }
+    }
+    if (access(fullPath.data(), F_OK)) {
+        if (mkdir(fullPath.data(), S_IRWXU))
+            return false;
+    }
+
+    return true;
+}
+
+String pathByAppendingComponent(const String& path, const String& component)
+{
+    if (path.endsWith('/'))
+        return path + component;
+    return path + "/" + component;
+}
+
+String pathByAppendingComponents(StringView path, const Vector<StringView>& components)
+{
+    StringBuilder builder;
+    builder.append(path);
+    bool isFirstComponent = true;
+    for (auto& component : components) {
+        if (isFirstComponent) {
+            isFirstComponent = false;
+            if (path.endsWith('/')) {
+                builder.append(component);
+                continue;
+            }
+        }
+        builder.append('/', component);
+    }
+    return builder.toString();
+}
+
+#endif
+
 } // namespace FileSystemImpl
 } // namespace WTF