Bug 234571 - Add WTF::UUID class which is natively a 128-bit integer
Summary: Add WTF::UUID class which is natively a 128-bit integer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-21 11:29 PST by Brady Eidson
Modified: 2021-12-26 13:17 PST (History)
9 users (show)

See Also:


Attachments
EWS v1 (58.39 KB, patch)
2021-12-22 21:01 PST, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS v2 (58.37 KB, patch)
2021-12-23 08:33 PST, Brady Eidson
achristensen: review-
Details | Formatted Diff | Diff
v3 (57.92 KB, patch)
2021-12-23 10:56 PST, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff
PFL v1 (57.85 KB, patch)
2021-12-23 11:27 PST, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
PFL v2 (57.86 KB, patch)
2021-12-23 12:24 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2021-12-21 11:29:41 PST
WTF::UUID should natively be a 128-bit integer, with an optional conversion to string as needed

This will be useful - amongst other places - where we use UUIDs as object identifiers.
Comment 1 Brady Eidson 2021-12-22 21:01:47 PST
Created attachment 447857 [details]
EWS v1
Comment 2 Brady Eidson 2021-12-23 08:33:44 PST
Created attachment 447879 [details]
EWS v2
Comment 3 Alex Christensen 2021-12-23 09:47:40 PST
Comment on attachment 447879 [details]
EWS v2

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

> Source/WTF/wtf/UUID.cpp:47
> +    cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);

static_assert(sizeof(m_data) == 16)

> Source/WTF/wtf/UUID.cpp:50
> +UUID::UUID(const Span<const uint8_t>& span)

I think this should take a Span<const uint8_t, 16> then the caller must ensure the size is correct.
Also, Span is like StringView and doesn't need to be passed by reference.

> Source/WTF/wtf/UUID.cpp:52
> +    uint64_t high, low;

This seems unnecessary.  Can't we just memcpy into m_data directly?

> Source/WTF/wtf/UUID.cpp:59
> +Vector<uint8_t> UUID::toVector() const

I think this is always an unnecessary copy and allocation.  It could return a Span<const uint8_t, 16>

> Source/WTF/wtf/UUID.cpp:74
> +    numbers[0] = UInt128High64(m_data);

This copy also seems unnecessary.

> Source/WTF/wtf/UUID.h:50
> +        if (span.size() != 16)

That would remove this check and this constructor would always succeed.  Also, I think create functions are a bit overkill here.
Comment 4 Brady Eidson 2021-12-23 10:56:59 PST
Created attachment 447883 [details]
v3
Comment 5 Alex Christensen 2021-12-23 11:00:33 PST
Comment on attachment 447883 [details]
v3

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

> Source/WTF/wtf/UUID.cpp:57
> +    return StringHasher::hashMemory(numbers, 16);

Can't we just call this on &m_data?
Comment 6 Brady Eidson 2021-12-23 11:27:21 PST
Created attachment 447889 [details]
PFL v1
Comment 7 Brady Eidson 2021-12-23 12:24:52 PST
Created attachment 447900 [details]
PFL v2
Comment 8 EWS 2021-12-23 13:51:43 PST
Committed r287412 (245547@main): <https://commits.webkit.org/245547@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447900 [details].
Comment 9 Radar WebKit Bug Importer 2021-12-23 13:52:16 PST
<rdar://problem/86864395>
Comment 10 mitz 2021-12-23 14:05:40 PST
Comment on attachment 447900 [details]
PFL v2

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

> Source/WTF/wtf/UUID.cpp:48
> +    cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);

Seems misleading to call these UUIDs if they don’t have a conforming version number (which would be 4 for random UUIDs). Should this use something like the uuid_generate_random (<https://github.com/apple-oss-distributions/Libc/blob/Libc-1439.141.1/uuid/uuidsrc/gen_uuid.c#L238>) instead?
Comment 11 Darin Adler 2021-12-24 08:55:39 PST
Comment on attachment 447900 [details]
PFL v2

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

>> Source/WTF/wtf/UUID.cpp:48
>> +    cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);
> 
> Seems misleading to call these UUIDs if they don’t have a conforming version number (which would be 4 for random UUIDs). Should this use something like the uuid_generate_random (<https://github.com/apple-oss-distributions/Libc/blob/Libc-1439.141.1/uuid/uuidsrc/gen_uuid.c#L238>) instead?

I came here to write something similar; I don’t think is accurate or safe to generate 128 random bits and call that a UUID.

> Source/WTF/wtf/UUID.cpp:53
> +    return StringHasher::hashMemory(reinterpret_cast<const unsigned char*>(&m_data), 16);

I suspect we can has the 128 bits more efficiently. I suggest we change this to call intHash() and write an integer hash for 128 bits analogous to the ones we have for 8, 16, 32, and 64 bit integers. Might be tricky because of UInt128.

> Source/WTF/wtf/UUID.h:41
> +WTF_MAKE_FAST_ALLOCATED;

Why are we including this? Are we allocating UUID objects on the heap? I suggest omitting this.

> Source/WTF/wtf/UUID.h:53
> +    explicit UUID(UInt128Impl&& data)

Since UInt128Impl is a structure containing only scalars, we should just take a UInt128Impl, not a UInt128Impl&&. There’s no ownership transfer here.

This should use UInt128, not UInt128Impl.

> Source/WTF/wtf/UUID.h:58
> +    UUID(const UUID&) = default;

If we omit this, it should be correctly generated.

> Source/WTF/wtf/UUID.h:60
> +    Span<const uint8_t, 16> toSpan() const

This can just be a conversion operator to Span. We can also have the toSpan function if you like, for cases where conversion would otherwise be ambiguous. But if we had a conversion operator I am pretty sure we could just write:

    Span span = identifier;

Or if that doesn’t compile, define a UUIDBytesSpan type to get the length right:

    UUIDBytesSpan span = identifier;

> Source/WTF/wtf/UUID.h:65
> +    UUID& operator=(const UUID&) = default;

If we omit this, it should be correctly generated.

> Source/WTF/wtf/UUID.h:87
> +    UInt128Impl m_data;

This should use UInt128, not UInt128Impl.
Comment 12 Yusuke Suzuki 2021-12-25 04:00:16 PST
Comment on attachment 447900 [details]
PFL v2

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

Nice

>>> Source/WTF/wtf/UUID.cpp:48
>>> +    cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);
>> 
>> Seems misleading to call these UUIDs if they don’t have a conforming version number (which would be 4 for random UUIDs). Should this use something like the uuid_generate_random (<https://github.com/apple-oss-distributions/Libc/blob/Libc-1439.141.1/uuid/uuidsrc/gen_uuid.c#L238>) instead?
> 
> I came here to write something similar; I don’t think is accurate or safe to generate 128 random bits and call that a UUID.

Can we ensure that m_data is not 0 or 1? According to newly added HashTraits<UUID>, 0 is used for empty and 1 is used for deleted value.
And I think this random value can be 0 or 1 currently.

> Source/WTF/wtf/UUID.h:106
> +    encoder << UInt128High64(m_data) << UInt128Low64(m_data);

Use the following, which is more efficient in architectures which support __uint128_t.

encoder << static_cast<uint64_t>(m_data >> 64) << static_cast<uint64_t>(m_data);

> Source/WTF/wtf/UUID.h:124
> +    return { UUID {
> +        MakeUInt128(*high, *low),
> +    } };

Let's just do this instead of using MakeUInt128.

return UUID { (static_cast<UInt128>(*high) << 64) | *low };