Bug 237870

Created attachment 454657 [details]
Patch

Comment on attachment 454657 [details]
Patch

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

> Source/WebGPU/WebGPU/Buffer.mm:352
> +    if (validateUnmap()) {

if (!validateUnmap())

Created attachment 454663 [details]
Patch

Comment on attachment 454663 [details]
Patch

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

some nits

> Source/WebGPU/WebGPU/Buffer.h:44
> +    using MappingRange = std::pair<size_t, size_t>;

Is there a benefit to using pair instead of real struct? You take a hit with the cumbersomeness of make_pair, requiring static casts

> Source/WebGPU/WebGPU/Buffer.h:85
> +    size_t m_size { 0 }; // "The length of the GPUBuffer allocation in bytes."

if some of these never change, you could mark them as const and not have the initialiser..

> Source/WebGPU/WebGPU/Buffer.mm:43
> +    // FIXME: "If any of the bits of descriptorâs usage arenât present in this deviceâs [[allowed buffer usages]] return false."

non-latin chars in comment?

> Source/WebGPU/WebGPU/Buffer.mm:77
> +    if (descriptor.mappedAtCreation && isNotMultipleOfPowerOfTwo(descriptor.size, 4))

typically I'd expect 
descriptor.size is a multiple of 4.
  descriptor.size % 4 == 0

E.g. in this case I'd expect
descriptor.mappedAtCreation && (descriptor.size % 4) != 0

I couldn't understand that function immediately isNotMultipleOfPowerOfTwo, read it as "is not multiple power of two x, y".

> Source/WebGPU/WebGPU/Buffer.mm:114
> +    buffer.label = [NSString stringWithCString:descriptor.label encoding:NSUTF8StringEncoding];

maybe descriptor.label == nullptr is fixed up in the latter commit?

> Source/WebGPU/WebGPU/Buffer.mm:132
> +        return Buffer::create(buffer, descriptor.size, descriptor.usage, Buffer::State::MappedAtCreation, std::make_pair(static_cast<size_t>(0), descriptor.size), *this);

std::make_pair could be just {0, descriptor.size} if your MappingRange would be normal struct.

> Source/WebGPU/WebGPU/Buffer.mm:186
> +    if (isNotMultipleOfPowerOfTwo(offset, 8))

here you check offset and 8, maybe intended is something else.
(I don't understand the isNotMultipleOfPowerOfTwo func)

> Source/WebGPU/WebGPU/Buffer.mm:195
> +    if (offset + rangeSize > m_mappingRange.second)

could move this to a variable, so then when the variable is declared checked arithmetic, you get the checked value down below in line 199

> Source/WebGPU/WebGPU/Buffer.mm:213
> +        rangeSize = std::max(static_cast<size_t>(0), m_size - offset);

I'm terrible at these, but:
 m_size - offset is still unsigned, so I don't think this is valid.

size_t value = 1;
value -= 77;
assert(std::max<size_t>(0, value) == value)


Writing these checks without checked arithmetic or if (offset > m_size) does not produce very understandable code

> Source/WebGPU/WebGPU/Buffer.mm:275
> +        rangeSize = std::max(static_cast<size_t>(0), m_size - offset);

max(0, u) is always u when u is unsigned.

> Source/WebGPU/WebGPU/Device.h:97
> +    bool m_hasUnifiedMemory { false };

could also be
hasUnfifiedMemory() ...

or could be const field

> Source/WebGPU/WebGPU/Utilities.h:32
> +inline bool isNotMultipleOfPowerOfTwo(uint64_t x, uint64_t y)

If it is used to check (x % y)  == 0 then I'd expect the check to be written inline instead of custom function with bit twiddling.

compiler knows all your ys, so it should be able to generate better code than you..

Similar functions exist in WTF StdLibExtras.h, so if this sort of stuff is added, maybe there..

also the "power of two" seems like a implementation detail. So we could call it
  isNotMultipleOf(x, y)
 and  we would then do 
   if constexpr(isPowerOfTwo(y)) ....
and then we would arrive the same conclusion that we should just let the compiler handle that, as it has isMultipleOf(a,b) implemented as  (a%b) == 0

Comment on attachment 454663 [details]
Patch

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

>> Source/WebGPU/WebGPU/Buffer.mm:186
>> +    if (isNotMultipleOfPowerOfTwo(offset, 8))
> 
> here you check offset and 8, maybe intended is something else.
> (I don't understand the isNotMultipleOfPowerOfTwo func)

Clarifying: 
 - offset is checked already
 - here you have a copy-paste error, you should check `(rangeSize % 4)`

Comment on attachment 454663 [details]
Patch

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

>> Source/WebGPU/WebGPU/Buffer.h:85
>> +    size_t m_size { 0 }; // "The length of the GPUBuffer allocation in bytes."
> 
> if some of these never change, you could mark them as const and not have the initialiser..

Good idea!!!

>> Source/WebGPU/WebGPU/Buffer.mm:43
>> +    // FIXME: "If any of the bits of descriptorâs usage arenât present in this deviceâs [[allowed buffer usages]] return false."
> 
> non-latin chars in comment?

It's a copy/paste. It's probably valuable to keep the original source text in the comment, so it can be copied and searched for within the spec.

>> Source/WebGPU/WebGPU/Buffer.mm:77
>> +    if (descriptor.mappedAtCreation && isNotMultipleOfPowerOfTwo(descriptor.size, 4))
> 
> typically I'd expect 
> descriptor.size is a multiple of 4.
>   descriptor.size % 4 == 0
> 
> E.g. in this case I'd expect
> descriptor.mappedAtCreation && (descriptor.size % 4) != 0
> 
> I couldn't understand that function immediately isNotMultipleOfPowerOfTwo, read it as "is not multiple power of two x, y".

Right, good idea.

>> Source/WebGPU/WebGPU/Buffer.mm:114
>> +    buffer.label = [NSString stringWithCString:descriptor.label encoding:NSUTF8StringEncoding];
> 
> maybe descriptor.label == nullptr is fixed up in the latter commit?

Yes.

>> Source/WebGPU/WebGPU/Buffer.mm:132
>> +        return Buffer::create(buffer, descriptor.size, descriptor.usage, Buffer::State::MappedAtCreation, std::make_pair(static_cast<size_t>(0), descriptor.size), *this);
> 
> std::make_pair could be just {0, descriptor.size} if your MappingRange would be normal struct.

Yep!

>>> Source/WebGPU/WebGPU/Buffer.mm:186
>>> +    if (isNotMultipleOfPowerOfTwo(offset, 8))
>> 
>> here you check offset and 8, maybe intended is something else.
>> (I don't understand the isNotMultipleOfPowerOfTwo func)
> 
> Clarifying: 
>  - offset is checked already
>  - here you have a copy-paste error, you should check `(rangeSize % 4)`

Yes, it's a copy-paste error. Good catch.

>> Source/WebGPU/WebGPU/Buffer.mm:213
>> +        rangeSize = std::max(static_cast<size_t>(0), m_size - offset);
> 
> I'm terrible at these, but:
>  m_size - offset is still unsigned, so I don't think this is valid.
> 
> size_t value = 1;
> value -= 77;
> assert(std::max<size_t>(0, value) == value)
> 
> 
> Writing these checks without checked arithmetic or if (offset > m_size) does not produce very understandable code

Right, this will be handled when I switch to using checked arithmetic. You're right that it's kind of silly now, but it's temporary.

>> Source/WebGPU/WebGPU/Buffer.mm:275
>> +        rangeSize = std::max(static_cast<size_t>(0), m_size - offset);
> 
> max(0, u) is always u when u is unsigned.

Ditto.

>> Source/WebGPU/WebGPU/Utilities.h:32
>> +inline bool isNotMultipleOfPowerOfTwo(uint64_t x, uint64_t y)
> 
> If it is used to check (x % y)  == 0 then I'd expect the check to be written inline instead of custom function with bit twiddling.
> 
> compiler knows all your ys, so it should be able to generate better code than you..
> 
> Similar functions exist in WTF StdLibExtras.h, so if this sort of stuff is added, maybe there..
> 
> also the "power of two" seems like a implementation detail. So we could call it
>   isNotMultipleOf(x, y)
>  and  we would then do 
>    if constexpr(isPowerOfTwo(y)) ....
> and then we would arrive the same conclusion that we should just let the compiler handle that, as it has isMultipleOf(a,b) implemented as  (a%b) == 0

I've deleted this function.

Committed r291369 (248501@trunk): <https://commits.webkit.org/248501@trunk>

<rdar://problem/90393491>