Disk usage estimations in Network Cache underestimate actual usage on platforms that aren't using 4KB file block size.
Created attachment 432518 [details] patch
Created attachment 432519 [details] patch
Created attachment 432545 [details] updated patch
Created attachment 432546 [details] updated patch
Comment on attachment 432545 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=432545&action=review > Source/WTF/wtf/FileSystem.cpp:714 > + return std::nullopt; We should probably add a `notImplemented();` in here too (after providing a Posix implementation). > Source/WTF/wtf/playstation/FileSystemPlayStation.cpp:172 > +std::optional getVolumeFileBlockSize(const String& path) Seems like we could use the exact same implementation in FileSystemPosix.cpp instead of returning std::nullopt? > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:315 > + static uint32_t cachedBlockSize; I don't think this should be static given that the block size depends on the storage path. We should probably store the block size as a data member on NetworkCacheStorage instead. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:317 > + if (auto blockSize = FileSystem::getVolumeFileBlockSize(path)) Would probably look nicer if using value_or()
Comment on attachment 432545 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=432545&action=review Just an idea for you cdumez on maybe simplifying this patch. >> Source/WTF/wtf/FileSystem.cpp:714 >> + return std::nullopt; > > We should probably add a `notImplemented();` in here too (after providing a Posix implementation). The previous behavior was that it just guesstimated a 4KB block size. Should that just go here? >> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:315 >> + static uint32_t cachedBlockSize; > > I don't think this should be static given that the block size depends on the storage path. We should probably store the block size as a data member on NetworkCacheStorage instead. If `getVolumeFileBlockSize` just defaults to 4KBs then this function isn't needed.
(In reply to Don Olmstead from comment #6) > Comment on attachment 432545 [details] > updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432545&action=review > > Just an idea for you cdumez on maybe simplifying this patch. > > >> Source/WTF/wtf/FileSystem.cpp:714 > >> + return std::nullopt; > > > > We should probably add a `notImplemented();` in here too (after providing a Posix implementation). > > The previous behavior was that it just guesstimated a 4KB block size. Should > that just go here? > > >> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:315 > >> + static uint32_t cachedBlockSize; > > > > I don't think this should be static given that the block size depends on the storage path. We should probably store the block size as a data member on NetworkCacheStorage instead. > > If `getVolumeFileBlockSize` just defaults to 4KBs then this function isn't > needed. Since you're adding a POSIX-compliant implementation, I'd rather we use it on all POSIX compliant systems, rather than just PlayStation.
Created attachment 432712 [details] Fixing review comments
Comment on attachment 432545 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=432545&action=review >>>> Source/WTF/wtf/FileSystem.cpp:714 >>>> + return std::nullopt; >>> >>> We should probably add a `notImplemented();` in here too (after providing a Posix implementation). >> >> The previous behavior was that it just guesstimated a 4KB block size. Should that just go here? > > Since you're adding a POSIX-compliant implementation, I'd rather we use it on all POSIX compliant systems, rather than just PlayStation. I wasn't able to use notImplemented here since it's currently in WebCore >> Source/WTF/wtf/playstation/FileSystemPlayStation.cpp:172 >> +std::optional getVolumeFileBlockSize(const String& path) > > Seems like we could use the exact same implementation in FileSystemPosix.cpp instead of returning std::nullopt? Because of internal reasons playstation isn't completely POSIX compliant here since we currently need to use f_bsize instead of f_frsize. I added a posix implementation using f_frsize.
Comment on attachment 432712 [details] Fixing review comments View in context: https://bugs.webkit.org/attachment.cgi?id=432712&action=review r=me with comments. > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:187 > +#if !PLATFORM(PLAYSTATION) Why is this needed? I would think that either: 1. The Playstation port doesn't build this file, in which case we don't need this #if !PLATFORM(PLAYSTATION) or 2. The Playstation port builds this file, in which case we don't this this #if or the duplicate implementation in FileSystemPlayStation.cpp. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:270 > + , m_volumeBlockSize(FileSystem::getVolumeFileBlockSize(baseDirectoryPath).value_or(4 * KB)) r- for this. This does a filesystem operation on the *main* thread, which is never OK. You need to call this on the background thread. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:165 > + size_t estimateRecordsSize(unsigned, unsigned); Please don't omit the parameter names as they are not obvious. Also, this function can probably be const.
(In reply to Chris Dumez from comment #10) > Comment on attachment 432712 [details] > Fixing review comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432712&action=review > > r=me with comments. Oops, I meant r-.
Comment on attachment 432712 [details] Fixing review comments View in context: https://bugs.webkit.org/attachment.cgi?id=432712&action=review >> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:187 >> +#if !PLATFORM(PLAYSTATION) > > Why is this needed? I would think that either: > 1. The Playstation port doesn't build this file, in which case we don't need this #if !PLATFORM(PLAYSTATION) > or > 2. The Playstation port builds this file, in which case we don't this this #if or the duplicate implementation in FileSystemPlayStation.cpp. I missed that the 2 implementations are slightly different. I am still curious if the Playstation port really builds the FileSystemPOSIX.cpp file.
Comment on attachment 432712 [details] Fixing review comments View in context: https://bugs.webkit.org/attachment.cgi?id=432712&action=review >> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:187 >> +#if !PLATFORM(PLAYSTATION) > > Why is this needed? I would think that either: > 1. The Playstation port doesn't build this file, in which case we don't need this #if !PLATFORM(PLAYSTATION) > or > 2. The Playstation port builds this file, in which case we don't this this #if or the duplicate implementation in FileSystemPlayStation.cpp. I think you got to my patch before my other comment. PlayStation does include FileSystemPOSIX but our platform isn't completely POSIX compliant here and we currently need to use f_bsize instead of f_frsize with statvfs. >> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:270 >> + , m_volumeBlockSize(FileSystem::getVolumeFileBlockSize(baseDirectoryPath).value_or(4 * KB)) > > r- for this. This does a filesystem operation on the *main* thread, which is never OK. You need to call this on the background thread. I'll take it off the main thread. >> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:165 >> + size_t estimateRecordsSize(unsigned, unsigned); > > Please don't omit the parameter names as they are not obvious. > > Also, this function can probably be const. Will fix.
> PlayStation does include FileSystemPOSIX but our platform isn't completely POSIX compliant here and we currently need to use f_bsize instead of f_frsize with statvfs. I think we can easily make PlayStation's f_frsize POSIX-compliant so we can use FileSystemPOSIX's impl.
<rdar://problem/80228379>
Created attachment 432988 [details] patch Fixing review comments. Switched playstation implementation to the POSIX one since we can make our version compliant. I also added a windows implementation to get the block size.
Comment on attachment 432988 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=432988&action=review > Source/WTF/wtf/FileSystem.h:125 > +WTF_EXPORT_PRIVATE std::optional<uint32_t> getVolumeFileBlockSize(const String&); Should not have "get" In the name, to match WebKit coding style, even though some existing functions do. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:166 > + uint32_t getVolumeBlockSize(); Should not have "get" In the name, to match WebKit coding style, even though some existing functions do.
Comment on attachment 432988 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=432988&action=review >> Source/WTF/wtf/FileSystem.h:125 >> +WTF_EXPORT_PRIVATE std::optional<uint32_t> getVolumeFileBlockSize(const String&); > > Should not have "get" In the name, to match WebKit coding style, even though some existing functions do. I think the guildline rules getters, but not free functions. It should add a new rule for free functions. https://webkit.org/code-style-guidelines/#names-setter-getter https://webkit.org/code-style-guidelines/#names-out-argument
Created attachment 433047 [details] Patch Removing "get"
Comment on attachment 433047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433047&action=review r=me with nits. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:166 > + uint32_t volumeBlockSize(); nit: I would mark this as const. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:176 > + uint32_t m_volumeBlockSize { 0 }; nit: I would mark this as mutable since it is computed lazily in the getter. Also, I'd using an std::optional<> instead of a magic value of 0 when unknown.
(In reply to Fujii Hironori from comment #18) > I think the guildline rules getters, but not free functions. Good point about how the guideline is written. The intention was to apply the rule to all functions, but the guidelines are not written that way. I agree that we should revise the guidelines at some point.
Comment on attachment 433047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433047&action=review > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:165 > + size_t estimateRecordsSize(unsigned recordCount, unsigned blobCount); I agree with Chris’s comment about marking volumeBlockSize const, and I suggest also marking this const for the same reason.
Created attachment 433056 [details] Patch for landing
Committed r279660 (239473@main): <https://commits.webkit.org/239473@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433056 [details].