Bug 207618 - Shrink CachedResource
Summary: Shrink CachedResource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-12 01:55 PST by Yusuke Suzuki
Modified: 2020-02-12 14:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (32.44 KB, patch)
2020-02-12 01:57 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.12 KB, patch)
2020-02-12 02:04 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-02-12 01:55:10 PST
Shrink CachedResource
Comment 1 Yusuke Suzuki 2020-02-12 01:57:54 PST
Created attachment 390499 [details]
Patch
Comment 2 Yusuke Suzuki 2020-02-12 02:04:51 PST
Created attachment 390500 [details]
Patch
Comment 3 Mark Lam 2020-02-12 11:44:04 PST
Comment on attachment 390500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390500&action=review

> Source/WebCore/ChangeLog:9
> +        For each enum class, we define `bitsOfXXX` value, which indicates # of bits to represent it. And using

Let's replace bitsOfXXX with XXXBitWidth to be consistent with https://bugs.webkit.org/show_bug.cgi?id=207616.

> Source/WTF/wtf/Markable.h:150
> +    Optional<T> asOptional() const
> +    {
> +        if (bool(*this))
> +            return m_value;
> +        return WTF::nullopt;
> +    }

This is identical to the cast operator above.  Can you implement this in terms of the cast operator or better yet, implement the cast operator in term of asOptional()?

> Source/WebCore/loader/ResourceLoaderOptions.h:49
> +static constexpr unsigned bitsOfSendCallbackPolicy = 1;

Let's call this sendCallbackPolicyBitWidth.

> Source/WebCore/loader/ResourceLoaderOptions.h:57
> +static constexpr unsigned bitsOfContentSniffingPolicy = 1;

Ditto: contentSniffingPolicyBitWidth.

> Source/WebCore/loader/ResourceLoaderOptions.h:63
> +static constexpr unsigned bitsOfDataBufferingPolicy = 1;

Ditto here and the others below.

> Source/WebCore/loader/cache/CachedResource.h:94
> +    static constexpr unsigned bitsOfType = 5;
> +    static_assert(static_cast<unsigned>(Type::LastType) <= ((1U << bitsOfType) - 1));

nit: Let's call this typeBitWidth instead.

> Source/WebCore/loader/cache/CachedResource.h:104
> +    static constexpr unsigned bitsOfStatus = 3;
> +    static_assert(static_cast<unsigned>(DecodeError) <= ((1ULL << bitsOfStatus) - 1));

Ditto: statusBitWidth.

> Source/WebCore/loader/cache/CachedResource.h:150
> +    static constexpr unsigned bitsOfPreloadResult = 2;

Ditto: preloadResultBitWidth.

> Source/WebCore/platform/network/ResourceLoadPriority.h:40
> +static constexpr unsigned bitsOfResourceLoadPriority = 3;

Ditto: resourceLoadPriorityBitWidth.

> Source/WebCore/platform/network/ResourceResponseBase.h:51
> +    static constexpr unsigned bitsOfType = 3;

Ditto: typeBitWidth.

> Source/WebCore/platform/network/ResourceResponseBase.h:53
> +    static constexpr unsigned bitsOfTainting = 2;

Ditto: taintingBitWidth.

> Source/WebCore/platform/network/StoredCredentialsPolicy.h:35
> +static constexpr unsigned bitsOfStoredCredentialsPolicy = 2;

Ditto: storedCredentialsPolicyBitWidth.
Comment 4 Yusuke Suzuki 2020-02-12 13:55:50 PST
Comment on attachment 390500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390500&action=review

Thanks!

>> Source/WebCore/ChangeLog:9
>> +        For each enum class, we define `bitsOfXXX` value, which indicates # of bits to represent it. And using
> 
> Let's replace bitsOfXXX with XXXBitWidth to be consistent with https://bugs.webkit.org/show_bug.cgi?id=207616.

Talked with Mark, replacing it with bitWidthOfXXX.

>> Source/WTF/wtf/Markable.h:150
>> +    }
> 
> This is identical to the cast operator above.  Can you implement this in terms of the cast operator or better yet, implement the cast operator in term of asOptional()?

OK, implementing it via cast operator.

>> Source/WebCore/loader/ResourceLoaderOptions.h:49
>> +static constexpr unsigned bitsOfSendCallbackPolicy = 1;
> 
> Let's call this sendCallbackPolicyBitWidth.

Fixed.

>> Source/WebCore/loader/ResourceLoaderOptions.h:57
>> +static constexpr unsigned bitsOfContentSniffingPolicy = 1;
> 
> Ditto: contentSniffingPolicyBitWidth.

Fixed.

>> Source/WebCore/loader/ResourceLoaderOptions.h:63
>> +static constexpr unsigned bitsOfDataBufferingPolicy = 1;
> 
> Ditto here and the others below.

Fixed.

>> Source/WebCore/loader/cache/CachedResource.h:94
>> +    static_assert(static_cast<unsigned>(Type::LastType) <= ((1U << bitsOfType) - 1));
> 
> nit: Let's call this typeBitWidth instead.

Fixed.

>> Source/WebCore/loader/cache/CachedResource.h:104
>> +    static_assert(static_cast<unsigned>(DecodeError) <= ((1ULL << bitsOfStatus) - 1));
> 
> Ditto: statusBitWidth.

Fixed.

>> Source/WebCore/loader/cache/CachedResource.h:150
>> +    static constexpr unsigned bitsOfPreloadResult = 2;
> 
> Ditto: preloadResultBitWidth.

Fixed.

>> Source/WebCore/platform/network/ResourceLoadPriority.h:40
>> +static constexpr unsigned bitsOfResourceLoadPriority = 3;
> 
> Ditto: resourceLoadPriorityBitWidth.

Fixed.

>> Source/WebCore/platform/network/ResourceResponseBase.h:51
>> +    static constexpr unsigned bitsOfType = 3;
> 
> Ditto: typeBitWidth.

Fixed.

>> Source/WebCore/platform/network/ResourceResponseBase.h:53
>> +    static constexpr unsigned bitsOfTainting = 2;
> 
> Ditto: taintingBitWidth.

Fixed.

>> Source/WebCore/platform/network/StoredCredentialsPolicy.h:35
>> +static constexpr unsigned bitsOfStoredCredentialsPolicy = 2;
> 
> Ditto: storedCredentialsPolicyBitWidth.

Fixed.
Comment 5 Yusuke Suzuki 2020-02-12 13:58:23 PST
Comment on attachment 390500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390500&action=review

> Source/WebCore/loader/cache/CachedImage.h:188
> +    unsigned m_updateImageDataCount : 3;

Initializer of this is added.
Comment 6 Yusuke Suzuki 2020-02-12 14:51:26 PST
Committed r256482: <https://trac.webkit.org/changeset/256482>
Comment 7 Radar WebKit Bug Importer 2020-02-12 14:52:16 PST
<rdar://problem/59402466>