Bug 227502 - [PlayStation] Don't assume 4KB block size when estimating Network Cache disk usage
Summary: [PlayStation] Don't assume 4KB block size when estimating Network Cache disk ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Christopher Reid
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-29 13:32 PDT by Christopher Reid
Modified: 2021-07-07 12:54 PDT (History)
11 users (show)

See Also:


Attachments
patch (5.46 KB, patch)
2021-06-29 13:42 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
patch (5.45 KB, patch)
2021-06-29 13:47 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
updated patch (5.45 KB, patch)
2021-06-29 15:46 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
updated patch (5.46 KB, patch)
2021-06-29 15:47 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Fixing review comments (7.15 KB, patch)
2021-07-01 11:49 PDT, Christopher Reid
cdumez: review-
Details | Formatted Diff | Diff
patch (6.38 KB, patch)
2021-07-06 16:44 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2021-07-07 10:56 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (6.85 KB, patch)
2021-07-07 11:57 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2021-06-29 13:32:34 PDT
Disk usage estimations in Network Cache underestimate actual usage on platforms that aren't using 4KB file block size.
Comment 1 Christopher Reid 2021-06-29 13:42:55 PDT
Created attachment 432518 [details]
patch
Comment 2 Christopher Reid 2021-06-29 13:47:27 PDT
Created attachment 432519 [details]
patch
Comment 3 Christopher Reid 2021-06-29 15:46:04 PDT
Created attachment 432545 [details]
updated patch
Comment 4 Christopher Reid 2021-06-29 15:47:13 PDT
Created attachment 432546 [details]
updated patch
Comment 5 Chris Dumez 2021-06-29 15:53:26 PDT
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 6 Don Olmstead 2021-06-29 16:05:15 PDT
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.
Comment 7 Chris Dumez 2021-06-29 16:10:00 PDT
(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.
Comment 8 Christopher Reid 2021-07-01 11:49:28 PDT
Created attachment 432712 [details]
Fixing review comments
Comment 9 Christopher Reid 2021-07-01 11:52:27 PDT
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 10 Chris Dumez 2021-07-01 11:55:05 PDT
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.
Comment 11 Chris Dumez 2021-07-01 11:55:31 PDT
(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 12 Chris Dumez 2021-07-01 12:03:20 PDT
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 13 Christopher Reid 2021-07-01 12:04:29 PDT
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.
Comment 14 Yoshiaki Jitsukawa 2021-07-03 19:47:13 PDT
> 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.
Comment 15 Radar WebKit Bug Importer 2021-07-06 13:33:15 PDT
<rdar://problem/80228379>
Comment 16 Christopher Reid 2021-07-06 16:44:37 PDT
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 17 Darin Adler 2021-07-06 16:53:41 PDT
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 18 Fujii Hironori 2021-07-06 17:49:50 PDT
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
Comment 19 Christopher Reid 2021-07-07 10:56:46 PDT
Created attachment 433047 [details]
Patch

Removing "get"
Comment 20 Chris Dumez 2021-07-07 10:59:53 PDT
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.
Comment 21 Darin Adler 2021-07-07 11:08:52 PDT
(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 22 Darin Adler 2021-07-07 11:09:44 PDT
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.
Comment 23 Christopher Reid 2021-07-07 11:57:47 PDT
Created attachment 433056 [details]
Patch for landing
Comment 24 EWS 2021-07-07 12:54:13 PDT
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].