[webkit-reviews] review granted: [Bug 237870] [WebGPU] Implement first draft of buffer mapping according to the spec : [Attachment 454663] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 237870: [WebGPU] Implement first draft of buffer mapping according to the
spec
https://bugs.webkit.org/show_bug.cgi?id=237870

Attachment 454663: Patch

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




--- Comment #4 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 454663
  --> https://bugs.webkit.org/attachment.cgi?id=454663
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