Bug 211328 – Allow Bitmap to use up to a UCPURegister word size for internal bit storage.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Previously, it was fixed at uint32_t.  This patch also allows it to use smaller ints (uint8_t, and uint16_t) if they will suffice.

[Radar WebKit Bug Importer]

<rdar://problem/62755865>

[Mark Lam]

Created attachment 398270 [details]
proposed patch.

[Mark Lam]

Created attachment 398271 [details]
proposed patch.

[Mark Lam]

Created attachment 398272 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 398272 [details]
proposed patch.

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

r=me

> Source/WTF/wtf/Bitmap.h:51
> +template<size_t size, typename = std::true_type>
> +struct BitmapWordType {
> +    using type = UCPURegister;
> +};
> +
> +template<size_t size>
> +struct BitmapWordType<size, std::enable_if_t<(size <= 8), std::true_type>> {
> +    using type = uint8_t;
> +};
> +
> +template<size_t size>
> +struct BitmapWordType<size, std::enable_if_t<(size > 8 && size <= 16), std::true_type>> {
> +    using type = uint16_t;
> +};
> +
> +template<size_t size>
> +struct BitmapWordType<size, std::enable_if_t<(size > 16 && size <= 32 && sizeof(UCPURegister) > sizeof(uint32_t)), std::true_type>> {
> +    using type = uint32_t;
> +};
> +
> +template<size_t bitmapSize, typename WordType = typename BitmapWordType<bitmapSize>::type>

You can use std::conditional<>.

template<size_t size>
using BitmapWordType = std::conditional_t<size <= 8,
    uint8_t,
    std::conditional_t<size <= 16,
        uint16_t,
        std::conditional_t<size <= 32 && sizeof(UCPURegister) > sizeof(uint32_t),
            uint32_t,
            UCPURegister>>>;

And,

template<size_t bitmapSize, typename WordType = BitmapWordType<bitmapSize>>

[Yusuke Suzuki]

Also, can you ensure that using `uint8_t`, `uint16_t` is better than using `uint32_t` in ARM etc. in terms of performance? And is it OK to use `uint8_t` and `uint16_t` while Bitmap needs to support concurrentTestAndSet etc.?

[Yusuke Suzuki]

Comment on attachment 398272 [details]
proposed patch.

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

>> Source/WTF/wtf/Bitmap.h:51
>> +template<size_t bitmapSize, typename WordType = typename BitmapWordType<bitmapSize>::type>
> 
> You can use std::conditional<>.
> 
> template<size_t size>
> using BitmapWordType = std::conditional_t<size <= 8,
>     uint8_t,
>     std::conditional_t<size <= 16,
>         uint16_t,
>         std::conditional_t<size <= 32 && sizeof(UCPURegister) > sizeof(uint32_t),
>             uint32_t,
>             UCPURegister>>>;
> 
> And,
> 
> template<size_t bitmapSize, typename WordType = BitmapWordType<bitmapSize>>

Because of concurrentTestAndSet etc. I think Bitmap is assuming WordSize for a default type. I think it is better that we just specialize Bitmap with UCPURegister only when we want and keep Bitmap's default parameter `uint32_t`. What do you think of?

[Mark Lam]

(In reply to Yusuke Suzuki from comment #7)
> Because of concurrentTestAndSet etc. I think Bitmap is assuming WordSize for
> a default type. I think it is better that we just specialize Bitmap with
> UCPURegister only when we want and keep Bitmap's default parameter
> `uint32_t`. What do you think of?

I talked to Yusuke offline.  We'll stick with uint32_t for Bitmap of size 32 or under.  We'll use UCPURegister for larger bitmaps.

[Mark Lam]

Created attachment 398280 [details]
patch for landing.

[Mark Lam]

I can reproduce the editing/undo-manager/undo-manager-delete-stale-undo-items.html failure locally is without my patch.  Filed https://bugs.webkit.org/show_bug.cgi?id=211340.

[Mark Lam]

Thanks for the review.  Landed in r261050: <http://trac.webkit.org/r261050>.

[Mark Lam]

Landed missing file in r261051: <http://trac.webkit.org/r261051>.

[Mark Lam]

Rolled out in r261055: <http://trac.webkit.org/r261055> while I investigate what's going on with editing/undo-manager/undo-manager-delete-stale-undo-items.html.

[Saam Barati]

Comment on attachment 398280 [details]
patch for landing.

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

> Source/WTF/wtf/StdIntExtras.h:34
> +#if USE(JSVALUE64)
> +using CPURegister = int64_t;
> +using UCPURegister = uint64_t;

nit: I know this is how things used to be, but it really doesn't feel like this should be keyed of JSVALUE64

[Saam Barati]

Comment on attachment 398280 [details]
patch for landing.

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

> Source/WTF/wtf/Bitmap.h:33
> +template<size_t size>
> +using BitmapWordType = std::conditional_t<(size <= 32 && sizeof(UCPURegister) > sizeof(uint32_t)), uint32_t, UCPURegister>;

Why not have this inside the class?

[Mark Lam]

Comment on attachment 398280 [details]
patch for landing.

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

>> Source/WTF/wtf/Bitmap.h:33
>> +using BitmapWordType = std::conditional_t<(size <= 32 && sizeof(UCPURegister) > sizeof(uint32_t)), uint32_t, UCPURegister>;
> 
> Why not have this inside the class?

Because it is used to provide the default type of the BitMap class.  Or do you mean why not just use the std::conditional as the default value?  Because it is a very long expression, and therefore makes the code less readable.

>> Source/WTF/wtf/StdIntExtras.h:34
>> +using UCPURegister = uint64_t;
> 
> nit: I know this is how things used to be, but it really doesn't feel like this should be keyed of JSVALUE64

Got any better suggestions?

[Saam Barati]

Comment on attachment 398280 [details]
patch for landing.

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

>>> Source/WTF/wtf/Bitmap.h:33
>>> +using BitmapWordType = std::conditional_t<(size <= 32 && sizeof(UCPURegister) > sizeof(uint32_t)), uint32_t, UCPURegister>;
>> 
>> Why not have this inside the class?
> 
> Because it is used to provide the default type of the BitMap class.  Or do you mean why not just use the std::conditional as the default value?  Because it is a very long expression, and therefore makes the code less readable.

I see, sorry. I misread the pre-existing code.

>>> Source/WTF/wtf/StdIntExtras.h:34
>>> +using UCPURegister = uint64_t;
>> 
>> nit: I know this is how things used to be, but it really doesn't feel like this should be keyed of JSVALUE64
> 
> Got any better suggestions?

arm64 || x86_64?

[Mark Lam]

Created attachment 398474 [details]
proposed patch.

[Mark Lam]

Created attachment 398478 [details]
proposed patch w/ Win build fixes.

[Yusuke Suzuki]

Comment on attachment 398474 [details]
proposed patch.

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

r=me with comment

> Source/WTF/wtf/PlatformCPU.h:320
> +/* CPU general purpose register width. */
> +#if CPU(ARM64) || CPU(X86_64) || CPU(MIPS64) || CPU(PPC64)
> +#define WTF_CPU_REGISTER64 1
> +#elif CPU(ARM) || CPU(X86) || CPU(MIPS) || CPU(PPC)
> +#define WTF_CPU_REGISTER32 1
> +#else
> +#error "Unknown register width"

This does not work since we support CPU(UNKNOWN) too.
I think that REGISTER64 and JSVALUE64 must be the same. If you define REGISTER64 here, I think we should define JSVALUE64 as,

#if CPU(REGISTER64)
#define WTF_USE_JSVALUE64
...

Can we just define this as,

#if !defined(WTF_CPU_REGISTER64) && !defined(WTF_CPU_REGISTER32)
#if CPU(ADDRESS64) || CPU(ARM64)
#define WTF_CPU_REGISTER64 1
#else
#define WTF_CPU_REGISTER32 1
#endif
#endif

And define USE_JSVALUE64 as,

#if CPU(REGISTER64)
#define USE_JSVALUE64
#else
#define USE_JSVALUE32
#endif

[Yusuke Suzuki]

Comment on attachment 398474 [details]
proposed patch.

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

>> Source/WTF/wtf/PlatformCPU.h:320
>> +#error "Unknown register width"
> 
> This does not work since we support CPU(UNKNOWN) too.
> I think that REGISTER64 and JSVALUE64 must be the same. If you define REGISTER64 here, I think we should define JSVALUE64 as,
> 
> #if CPU(REGISTER64)
> #define WTF_USE_JSVALUE64
> ...
> 
> Can we just define this as,
> 
> #if !defined(WTF_CPU_REGISTER64) && !defined(WTF_CPU_REGISTER32)
> #if CPU(ADDRESS64) || CPU(ARM64)
> #define WTF_CPU_REGISTER64 1
> #else
> #define WTF_CPU_REGISTER32 1
> #endif
> #endif
> 
> And define USE_JSVALUE64 as,
> 
> #if CPU(REGISTER64)
> #define USE_JSVALUE64
> #else
> #define USE_JSVALUE32
> #endif

#if CPU(REGISTER64)
#define USE_JSVALUE64 1
#else
#define USE_JSVALUE32 1
#endif

[Mark Lam]

Created attachment 398484 [details]
patch for landing.

Thanks for the review.  I've applied the suggested change to the definition of CPU(REGISTER64) and CPU(REGISTER32).

[Mark Lam]

Created attachment 398512 [details]
patch for landing.

[Mark Lam]

Landed in r261179: <http://trac.webkit.org/r261179>.