Skip to content

Commit

Permalink
Use mapped files in CacheStorageDiskStore
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=259371
rdar://problem/112623177

Reviewed by Chris Dumez.

Currently CacheStorageDiskStore reads entire files into dirty memory. When many DOMCache operations
in flight, this can lead to a temporary explosion in NetworkProcess memory usage (in the gigabytes
of footprint).

To fix this issue, we use read-only mapped files in CacheStorageDiskStore instead, as we do with the
regular network cache. This trades dirty footprint for higher vnode usage. But that should be okay
as the number of vnodes consumed by DOMCache should be much lower than the number of vnodes used by
NetworkCache, which is already mmap'ing everything.

I also removed some unnecessary cross-thread copies, as the data we're passing across threads isn't
affined to a particular thread (e.g. no non-atomic ref counts).

* Source/WTF/wtf/FileSystem.h:
(WTF::FileSystemImpl::MappedFileData::toSpan):
(WTF::FileSystemImpl::MappedFileData::create):
* Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.cpp:
(WebKit::readRecordInfoFromFileData):
(WebKit::CacheStorageDiskStore::readRecordFromFileData):
(WebKit::CacheStorageDiskStore::readAllRecordInfosInternal):
(WebKit::CacheStorageDiskStore::readAllRecordInfos):
(WebKit::CacheStorageDiskStore::readRecordsInternal):
(WebKit::CacheStorageDiskStore::readRecords):
* Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.h:

Canonical link: https://commits.webkit.org/266199@main
  • Loading branch information
@bnham
bnham committed Jul 21, 2023
1 parent c107fc3 commit 470bd54
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 15 deletions.
35 changes: 35 additions & 0 deletions Source/WTF/wtf/FileSystem.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#pragma once

#include <span>
#include <sys/types.h>
#include <time.h>
#include <utility>
Expand Down Expand Up @@ -242,6 +243,9 @@ class MappedFileData {
public:
MappedFileData() { }
MappedFileData(MappedFileData&&);
static std::optional<MappedFileData> create(const String& filePath, MappedFileMode);
static std::optional<MappedFileData> create(PlatformFileHandle, MappedFileMode);
static std::optional<MappedFileData> create(PlatformFileHandle, FileOpenMode, MappedFileMode);
WTF_EXPORT_PRIVATE MappedFileData(const String& filePath, MappedFileMode, bool& success);
WTF_EXPORT_PRIVATE MappedFileData(PlatformFileHandle, MappedFileMode, bool& success);
WTF_EXPORT_PRIVATE MappedFileData(PlatformFileHandle, FileOpenMode, MappedFileMode, bool& success);
Expand All @@ -251,6 +255,7 @@ class MappedFileData {
explicit operator bool() const { return !!m_fileData; }
const void* data() const { return m_fileData; }
unsigned size() const { return m_fileSize; }
std::span<const uint8_t> toSpan() { return { static_cast<const uint8_t *>(data()), size() }; }

#if PLATFORM(COCOA)
void* leakHandle() { return std::exchange(m_fileData, nullptr); }
Expand All @@ -269,6 +274,36 @@ class MappedFileData {
#endif
};

inline std::optional<MappedFileData> MappedFileData::create(const String& filePath, MappedFileMode mode)
{
std::optional<MappedFileData> result;
bool success = false;
auto data = MappedFileData { filePath, mode, success };
if (success)
result = WTFMove(data);
return result;
}

inline std::optional<MappedFileData> MappedFileData::create(PlatformFileHandle handle, MappedFileMode mode)
{
std::optional<MappedFileData> result;
bool success = false;
auto data = MappedFileData { handle, mode, success };
if (success)
result = WTFMove(data);
return result;
}

inline std::optional<MappedFileData> MappedFileData::create(PlatformFileHandle handle, FileOpenMode openMode, MappedFileMode mappedFileMode)
{
std::optional<MappedFileData> result;
bool success = false;
auto data = MappedFileData { handle, openMode, mappedFileMode, success };
if (success)
result = WTFMove(data);
return result;
}

inline MappedFileData::MappedFileData(PlatformFileHandle handle, MappedFileMode mapMode, bool& success)
{
success = mapFileHandle(handle, FileOpenMode::Read, mapMode);
Expand Down
27 changes: 14 additions & 13 deletions Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,11 @@ static std::optional<RecordHeader> decodeRecordHeader(std::span<const uint8_t> h
};
}

static std::optional<StoredRecordInformation> readRecordInfoFromFileData(const FileSystem::Salt& salt, const Vector<uint8_t>& buffer)
static std::optional<StoredRecordInformation> readRecordInfoFromFileData(const FileSystem::Salt& salt, std::span<const uint8_t> fileData)
{
if (buffer.isEmpty())
if (!fileData.size())
return std::nullopt;

auto fileData = std::span(buffer.data(), buffer.size());
auto metaData = decodeRecordMetaData(fileData);
if (!metaData)
return std::nullopt;
Expand All @@ -301,7 +300,7 @@ static std::optional<StoredRecordInformation> readRecordInfoFromFileData(const F
return StoredRecordInformation { info, WTFMove(*metaData), WTFMove(*header) };
}

std::optional<CacheStorageRecord> CacheStorageDiskStore::readRecordFromFileData(const Vector<uint8_t>& buffer, const Vector<uint8_t>& blobBuffer)
std::optional<CacheStorageRecord> CacheStorageDiskStore::readRecordFromFileData(std::span<const uint8_t> buffer, FileSystem::MappedFileData&& blobBuffer)
{
auto storedInfo = readRecordInfoFromFileData(m_salt, buffer);
if (!storedInfo)
Expand All @@ -318,12 +317,14 @@ std::optional<CacheStorageRecord> CacheStorageDiskStore::readRecordFromFileData(
if (storedInfo->metaData.bodyHash != computeSHA1(bodyData, m_salt))
return std::nullopt;

// FIXME: avoid copying inline body data here, perhaps by adding offset support to
// MappedFileData, or by taking a read-only virtual copy of bodyData.
responseBody = WebCore::SharedBuffer::create(bodyData.data(), bodyData.size());
} else {
if (blobBuffer.isEmpty())
if (!blobBuffer)
return std::nullopt;

auto sharedBuffer = WebCore::SharedBuffer::create(blobBuffer.data(), blobBuffer.size());
auto sharedBuffer = WebCore::SharedBuffer::create(WTFMove(blobBuffer));
auto bodyData = std::span(sharedBuffer->data(), sharedBuffer->size());
if (storedInfo->metaData.bodyHash != computeSHA1(bodyData, m_salt))
return std::nullopt;
Expand Down Expand Up @@ -351,13 +352,13 @@ void CacheStorageDiskStore::readAllRecordInfosInternal(CompletionHandler<void(Fi
continue;

auto recordFile = FileSystem::pathByAppendingComponent(cacheDirectory, recordName);
auto fileData = FileSystem::readEntireFile(recordFile);
auto fileData = FileSystem::MappedFileData::create(recordFile, FileSystem::MappedFileMode::Private);
if (fileData)
fileDatas.append(WTFMove(*fileData));
}
}

m_callbackQueue->dispatch([protectedThis = WTFMove(protectedThis), fileDatas = crossThreadCopy(WTFMove(fileDatas)), callback = WTFMove(callback)]() mutable {
m_callbackQueue->dispatch([protectedThis = WTFMove(protectedThis), fileDatas = WTFMove(fileDatas), callback = WTFMove(callback)]() mutable {
callback(WTFMove(fileDatas));
});
});
Expand All @@ -369,7 +370,7 @@ void CacheStorageDiskStore::readAllRecordInfos(ReadAllRecordInfosCallback&& call
Vector<CacheStorageRecordInformation> result;
result.reserveInitialCapacity(fileDatas.size());
for (auto& fileData : fileDatas) {
if (auto storedInfo = readRecordInfoFromFileData(m_salt, fileData))
if (auto storedInfo = readRecordInfoFromFileData(m_salt, fileData.toSpan()))
result.uncheckedAppend(WTFMove(storedInfo->info));
else
RELEASE_LOG(CacheStorage, "%p - CacheStorageDiskStore::readAllRecordInfos fails to decode record from file", this);
Expand All @@ -384,13 +385,13 @@ void CacheStorageDiskStore::readRecordsInternal(Vector<String>&& recordFiles, Co
FileDatas fileDatas;
FileDatas blobDatas;
for (auto& recordFile : recordFiles) {
auto fileData = valueOrDefault(FileSystem::readEntireFile(recordFile));
auto blobData = fileData.isEmpty()? Vector<uint8_t> { } : valueOrDefault(FileSystem::readEntireFile(recordBlobFilePath(recordFile)));
auto fileData = valueOrDefault(FileSystem::MappedFileData::create(recordFile, FileSystem::MappedFileMode::Private));
auto blobData = !fileData.size() ? FileSystem::MappedFileData { } : valueOrDefault(FileSystem::MappedFileData::create(recordBlobFilePath(recordFile), FileSystem::MappedFileMode::Private));
fileDatas.append(WTFMove(fileData));
blobDatas.append(WTFMove(blobData));
}

m_callbackQueue->dispatch([protectedThis = WTFMove(protectedThis), fileDatas = crossThreadCopy(WTFMove(fileDatas)), blobDatas = crossThreadCopy(WTFMove(blobDatas)), callback = WTFMove(callback)]() mutable {
m_callbackQueue->dispatch([protectedThis = WTFMove(protectedThis), fileDatas = WTFMove(fileDatas), blobDatas = WTFMove(blobDatas), callback = WTFMove(callback)]() mutable {
callback(WTFMove(fileDatas), WTFMove(blobDatas));
});
});
Expand All @@ -408,7 +409,7 @@ void CacheStorageDiskStore::readRecords(const Vector<CacheStorageRecordInformati

Vector<std::optional<CacheStorageRecord>> result;
for (size_t index = 0; index < recordInfos.size(); ++index) {
auto record = readRecordFromFileData(fileDatas[index], blobDatas[index]);
auto record = readRecordFromFileData(fileDatas[index].toSpan(), WTFMove(blobDatas[index]));
if (record) {
auto recordInfo = recordInfos[index];
if (recordInfo.insertionTime != record->info.insertionTime || recordInfo.size != record->info.size || recordInfo.url != record->info.url)
Expand Down
5 changes: 3 additions & 2 deletions Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include "CacheStorageStore.h"
#include "NetworkCacheKey.h"
#include <wtf/FileSystem.h>
#include <wtf/WorkQueue.h>
#include <wtf/text/WTFString.h>

Expand All @@ -52,8 +53,8 @@ class CacheStorageDiskStore final : public CacheStorageStore {
String recordBlobFilePath(const String&) const;
String blobsDirectoryPath() const;
String blobFilePath(const String&) const;
std::optional<CacheStorageRecord> readRecordFromFileData(const Vector<uint8_t>&, const Vector<uint8_t>&);
using FileDatas = Vector<Vector<uint8_t>>;
std::optional<CacheStorageRecord> readRecordFromFileData(std::span<const uint8_t>, FileSystem::MappedFileData&&);
using FileDatas = Vector<FileSystem::MappedFileData>;
void readAllRecordInfosInternal(CompletionHandler<void(FileDatas&&)>&&);
void readRecordsInternal(Vector<String>&&, CompletionHandler<void(FileDatas&&, FileDatas&&)>&&);

Expand Down

0 comments on commit 470bd54

Please sign in to comment.