Stop using IOChannel in NetworkCache

https://bugs.webkit.org/show_bug.cgi?id=290211
rdar://142645876

Reviewed by Per Arne Vollan.

This is a speculative fix for a hard-to-repro issue where page loads sometimes hang for minutes
while the system is under heavy load.

What we see in logs is that the disk network cache lookup ends up on a pri 4 default overcommit
dispatch queue. That thread is parked in a pread and gets preempted for minutes while other higher
pri work is occurring. The thing that is enqueuing the pread on to that dispatch queue is
`dispatch_io_read`, which enqueues that block on to a queue that it controls. We think that probably
something is going wrong with the priority propagation to that queue sometimes, as the network cache
lookup hops from main thread => network cache I/O queue => dispatch_io private queue and back.

As a speculative fix:

- Stop using `dispatch_io` / `IOChannel` altogether. It buys us nothing other than adding extra
  thread hops, making things harder to debug, and perhaps exposing some latent bug in priority
  propagation. Instead, just do the I/O directly on the network cache's I/O queues.

- Change the priority of the read-only I/O queue from default to UserInteractive, since it's on the
  blocking path for resource fetching.

- Change the priority of the write I/O queues from Background to Utility, since Background is very
  punitive (capped at max pri 4), and if we get anything wrong, getting capped at max pri 4 can
  cause minutes of starvation.

WriteOperations also had an completion handler that received an integer errno as an argument. This
was unused, so I removed it.

* Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp:
(WebKit::NetworkCache::Data::Data):
* Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:
* Source/WebKit/NetworkProcess/cache/NetworkCacheDataCocoa.mm:
(WebKit::NetworkCache::Data::Data):
* Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:
(WebKit::NetworkCache::Storage::WriteOperation::WriteOperation):
(WebKit::NetworkCache::Storage::WriteOperation::~WriteOperation):
(WebKit::NetworkCache::Storage::Storage):
(WebKit::NetworkCache::Storage::dispatchReadOperation):
(WebKit::NetworkCache::Storage::readRecordFromData):
(WebKit::NetworkCache::Storage::dispatchWriteOperation):
(WebKit::NetworkCache::Storage::finishWriteOperationActivity):
(WebKit::NetworkCache::Storage::store):
(WebKit::NetworkCache::Storage::WriteOperation::invokeCompletionHandler): Deleted.
* Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:
(WebKit::NetworkCache::Storage::store): Deleted.

Canonical link: https://commits.webkit.org/292576@main

Author: bnham

Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp

@@ -39,6 +39,18 @@
 namespace WebKit {
 namespace NetworkCache {
 
+#if !USE(GLIB) && USE(CURL)
+Data::Data(Vector<uint8_t>&& data)
+    : Data(std::variant<Vector<uint8_t>, FileSystem::MappedFileData> { WTFMove(data) })
+{
+}
+#elif !PLATFORM(COCOA)
+Data::Data(Vector<uint8_t>&& data)
+    : Data(data.span())
+{
+}
+#endif
+
 Data Data::mapToFile(const String& path) const
 {
     FileSystem::FileHandle handle;

Source/WebKit/NetworkProcess/cache/NetworkCacheData.h

@@ -32,6 +32,7 @@
 #include <wtf/MappedFileData.h>
 #include <wtf/SHA1.h>
 #include <wtf/ThreadSafeRefCounted.h>
+#include <wtf/Vector.h>
 #include <wtf/text/WTFString.h>
 
 #if PLATFORM(COCOA)
@@ -59,6 +60,7 @@ class Data {
 public:
     Data() { }
     Data(std::span<const uint8_t>);
+    Data(Vector<uint8_t>&& data);
 
     ~Data() { }
 
@@ -73,7 +75,6 @@ class Data {
     Data(GRefPtr<GBytes>&&, FileSystem::FileHandle&& = { });
 #elif USE(CURL)
     Data(std::variant<Vector<uint8_t>, FileSystem::MappedFileData>&&);
-    Data(Vector<uint8_t>&& data) : Data(std::variant<Vector<uint8_t>, FileSystem::MappedFileData> { WTFMove(data) }) { }
 #endif
     bool isNull() const;
     bool isEmpty() const { return !size(); }

Source/WebKit/NetworkProcess/cache/NetworkCacheDataCocoa.mm

@@ -32,6 +32,7 @@
 #import <sys/stat.h>
 #import <wtf/FileHandle.h>
 #import <wtf/cocoa/SpanCocoa.h>
+#import <wtf/cocoa/VectorCocoa.h>
 
 namespace WebKit {
 namespace NetworkCache {
@@ -47,6 +48,11 @@
 {
 }
 
+Data::Data(Vector<uint8_t>&& data)
+    : Data(makeDispatchData(WTFMove(data)).get(), Backing::Buffer)
+{
+}
+
 Data Data::empty()
 {
     return { OSObjectPtr<dispatch_data_t> { dispatch_data_empty } };

Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp

@@ -206,32 +206,27 @@ void Storage::ReadOperation::finishReadBlob(BlobStorage::Blob&& blob, MonotonicT
 class Storage::WriteOperation {
     WTF_MAKE_TZONE_ALLOCATED(Storage::WriteOperation);
 public:
-    WriteOperation(const Record& record, MappedBodyHandler&& mappedBodyHandler, CompletionHandler<void(int)>&& completionHandler)
+    WriteOperation(const Record& record, MappedBodyHandler&& mappedBodyHandler)
         : m_identifier(Storage::WriteOperationIdentifier::generate())
         , m_record(record)
         , m_mappedBodyHandler(WTFMove(mappedBodyHandler))
-        , m_completionHandler(WTFMove(completionHandler))
     {
         ASSERT(isMainRunLoop());
     }
 
     ~WriteOperation()
     {
         ASSERT(isMainRunLoop());
-        if (m_completionHandler)
-            m_completionHandler(0);
     }
 
     Storage::WriteOperationIdentifier identifier() const { return m_identifier; }
     const Record& record() const WTF_REQUIRES_CAPABILITY(mainThread) { return m_record; }
     void invokeMappedBodyHandler(const Data&);
-    void invokeCompletionHandler(int);
 
 private:
     Storage::WriteOperationIdentifier m_identifier;
     const Record m_record;
     const MappedBodyHandler m_mappedBodyHandler;
-    CompletionHandler<void(int)> m_completionHandler;
 };
 
 WTF_MAKE_TZONE_ALLOCATED_IMPL(Storage::WriteOperation);
@@ -242,12 +237,6 @@ void Storage::WriteOperation::invokeMappedBodyHandler(const Data& data)
         m_mappedBodyHandler(data);
 }
 
-void Storage::WriteOperation::invokeCompletionHandler(int result)
-{
-    if (m_completionHandler)
-        m_completionHandler(result);
-}
-
 class TraverseOperation final : public ThreadSafeRefCounted<TraverseOperation, WTF::DestructionThread::MainRunLoop> {
 public:
     static Ref<TraverseOperation> create(Storage::TraverseHandler&& handler)
@@ -414,9 +403,9 @@ Storage::Storage(const String& baseDirectoryPath, Mode mode, Salt salt, size_t c
     , m_capacity(capacity)
     , m_readOperationTimeoutTimer(*this, &Storage::cancelAllReadOperations)
     , m_writeOperationDispatchTimer(*this, &Storage::dispatchPendingWriteOperations)
-    , m_ioQueue(ConcurrentWorkQueue::create("com.apple.WebKit.Cache.Storage"_s))
-    , m_backgroundIOQueue(ConcurrentWorkQueue::create("com.apple.WebKit.Cache.Storage.background"_s, WorkQueue::QOS::Background))
-    , m_serialBackgroundIOQueue(WorkQueue::create("com.apple.WebKit.Cache.Storage.serialBackground"_s, WorkQueue::QOS::Background))
+    , m_ioQueue(ConcurrentWorkQueue::create("com.apple.WebKit.Cache.Storage"_s, WorkQueue::QOS::UserInteractive))
+    , m_backgroundIOQueue(ConcurrentWorkQueue::create("com.apple.WebKit.Cache.Storage.background"_s, WorkQueue::QOS::Utility))
+    , m_serialBackgroundIOQueue(WorkQueue::create("com.apple.WebKit.Cache.Storage.serialBackground"_s, WorkQueue::QOS::Utility))
     , m_blobStorage(makeBlobDirectoryPath(baseDirectoryPath), m_salt)
 {
     ASSERT(RunLoop::isMain());
@@ -884,21 +873,16 @@ void Storage::dispatchReadOperation(std::unique_ptr<ReadOperation> readOperation
     }
 
     ioQueue().dispatch([this, protectedThis = Ref { *this }, identifier, recordPath = crossThreadCopy(WTFMove(recordPath)), blobPath = crossThreadCopy(WTFMove(blobPath))]() mutable {
-        auto recordIOStartTime = MonotonicTime::now();
-        auto channel = IOChannel::open(recordPath, IOChannel::Type::Read);
-        channel->read(0, std::numeric_limits<size_t>::max(), ioQueue(), [this, protectedThis = Ref { *this }, identifier, recordIOStartTime](auto fileData, int error) mutable {
-            readRecordFromData(identifier, recordIOStartTime, WTFMove(fileData), error);
-        });
-
+        readRecordFromData(identifier, MonotonicTime::now(), FileSystem::readEntireFile(recordPath));
         readBlobIfNecessary(identifier, blobPath);
     });
 }
 
-void Storage::readRecordFromData(Storage::ReadOperationIdentifier identifier, MonotonicTime recordIOStartTime, Data&& data, int error)
+void Storage::readRecordFromData(Storage::ReadOperationIdentifier identifier, MonotonicTime recordIOStartTime, std::optional<Vector<uint8_t>>&& data)
 {
     Record record;
-    if (!error)
-        record = readRecord(data);
+    if (data)
+        record = readRecord(WTFMove(*data));
 
     auto recordIOEndTime = MonotonicTime::now();
     RunLoop::protectedMain()->dispatch([this, protectedThis = Ref { *this }, identifier, recordIOStartTime, recordIOEndTime, record = crossThreadCopy(WTFMove(record))]() mutable {
@@ -1039,15 +1023,14 @@ void Storage::dispatchWriteOperation(std::unique_ptr<WriteOperation> writeOperat
         bool shouldStoreAsBlob = shouldStoreBodyAsBlob(record.body);
         auto blob = shouldStoreAsBlob ? storeBodyAsBlob(identifier, record) : std::nullopt;
         auto recordData = encodeRecord(record, blob);
+        auto recordSize = recordData.size();
 
-        auto channel = IOChannel::open(WTFMove(recordPath), IOChannel::Type::Create);
-        size_t recordSize = recordData.size();
-        channel->write(0, recordData, WorkQueue::main(), [this, protectedThis = Ref { *this }, identifier, recordSize](int error) {
-            // On error the entry still stays in the contents filter until next synchronization.
-            m_approximateRecordsSize += recordSize;
-            finishWriteOperationActivity(identifier, error);
+        if (!FileSystem::overwriteEntireFile(recordPath, recordData.span()))
+            RELEASE_LOG_ERROR(NetworkCacheStorage, "Failed to write %zu bytes of network cache record data to %" PUBLIC_LOG_STRING, recordSize, recordPath.utf8().data());
 
-            LOG(NetworkCacheStorage, "(NetworkProcess) write complete error=%d", error);
+        RunLoop::protectedMain()->dispatch([this, protectedThis = Ref { *this }, identifier, recordSize]() mutable {
+            m_approximateRecordsSize += recordSize;
+            finishWriteOperationActivity(identifier);
         });
     });
 }
@@ -1065,15 +1048,14 @@ bool Storage::removeWriteOperationActivity(WriteOperationIdentifier identifier)
     return m_writeOperationActivities.remove(identifier);
 }
 
-void Storage::finishWriteOperationActivity(WriteOperationIdentifier identifier, int error)
+void Storage::finishWriteOperationActivity(WriteOperationIdentifier identifier)
 {
     ASSERT(RunLoop::isMain());
     if (!removeWriteOperationActivity(identifier))
         return;
 
     auto writeOperation = m_activeWriteOperations.take(identifier);
     RELEASE_ASSERT(writeOperation);
-    writeOperation->invokeCompletionHandler(error);
 
     dispatchPendingWriteOperations();
 
@@ -1108,15 +1090,15 @@ void Storage::retrieve(const Key& key, unsigned priority, RetrieveCompletionHand
     dispatchPendingReadOperations();
 }
 
-void Storage::store(const Record& record, MappedBodyHandler&& mappedBodyHandler, CompletionHandler<void(int)>&& completionHandler)
+void Storage::store(const Record& record, MappedBodyHandler&& mappedBodyHandler)
 {
     ASSERT(RunLoop::isMain());
     ASSERT(!record.key.isNull());
 
     if (!m_capacity)
         return;
 
-    auto writeOperation = makeUnique<WriteOperation>(record, WTFMove(mappedBodyHandler), WTFMove(completionHandler));
+    auto writeOperation = makeUnique<WriteOperation>(record, WTFMove(mappedBodyHandler));
     m_pendingWriteOperations.prepend(WTFMove(writeOperation));
 
     // Add key to the filter already here as we do lookups from the pending operations too.

Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h

@@ -103,7 +103,7 @@ class Storage : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<Storage,
     void retrieve(const Key&, unsigned priority, RetrieveCompletionHandler&&);
 
     using MappedBodyHandler = Function<void (const Data& mappedBody)>;
-    void store(const Record&, MappedBodyHandler&&, CompletionHandler<void(int)>&& = { });
+    void store(const Record&, MappedBodyHandler&&);
 
     void remove(const Key&);
     void remove(const Vector<Key>&, CompletionHandler<void()>&&);
@@ -166,13 +166,13 @@ class Storage : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<Storage,
     void dispatchPendingWriteOperations();
     void addWriteOperationActivity(WriteOperationIdentifier);
     bool removeWriteOperationActivity(WriteOperationIdentifier);
-    void finishWriteOperationActivity(WriteOperationIdentifier, int error = 0);
+    void finishWriteOperationActivity(WriteOperationIdentifier);
 
     bool shouldStoreBodyAsBlob(const Data& bodyData);
     std::optional<BlobStorage::Blob> storeBodyAsBlob(WriteOperationIdentifier, const Storage::Record&);
     Data encodeRecord(const Record&, std::optional<BlobStorage::Blob>);
     Record readRecord(const Data&);
-    void readRecordFromData(Storage::ReadOperationIdentifier, MonotonicTime, Data&&, int error);
+    void readRecordFromData(Storage::ReadOperationIdentifier, MonotonicTime, std::optional<Vector<uint8_t>>&&);
     void readBlobIfNecessary(Storage::ReadOperationIdentifier, const String& blobPath);
 
     void updateFileModificationTime(String&& path);