Bug 227502 – [PlayStation] Don't assume 4KB block size when estimating Network Cache disk usage

• Component: Platform
• Status: RESOLVED FIXED

[Christopher Reid]

Disk usage estimations in Network Cache underestimate actual usage on platforms that aren't using 4KB file block size.

[Christopher Reid]

Created attachment 432518 [details]
patch

[Christopher Reid]

Created attachment 432519 [details]
patch

[Christopher Reid]

Created attachment 432545 [details]
updated patch

[Christopher Reid]

Created attachment 432546 [details]
updated patch

[Chris Dumez]

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()

[Don Olmstead]

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.

[Chris Dumez]

(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.

[Christopher Reid]

Created attachment 432712 [details]
Fixing review comments

[Christopher Reid]

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.

[Chris Dumez]

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.

[Chris Dumez]

(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-.

[Chris Dumez]

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.

[Christopher Reid]

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.

[Yoshiaki Jitsukawa]

> 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.

[Radar WebKit Bug Importer]

<rdar://problem/80228379>

[Christopher Reid]

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.

[Darin Adler]

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.

[Fujii Hironori]

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

[Christopher Reid]

Created attachment 433047 [details]
Patch

Removing "get"

[Chris Dumez]

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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Christopher Reid]

Created attachment 433056 [details]
Patch for landing

[EWS]

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].