search

boostorg / uuid

Boost.org uuid module
http://boost.org/libs/uuid
Boost Software License 1.0
83 stars 67 forks source link

Align `boost::uuid` to 16 bytes #148

Closed Lastique closed 2 months ago

Lastique commented 4 months ago

This improves performance of operations on UUIDs as the UUID object now does not(*) cross page boundary. It may also improve code generation by compilers, which can now assume that loads and stores of UUIDs are not as expensive and therefore it may be more preferable to use vector instructions for that.

(*) It is technically possible that the boost::uuid object is still not properly aligned under some conditions. For example, on some 32-bit targets the stack is not aligned to 16 bytes, so placing the UUID object on the unaligned stack may result in unaligned UUID. For this reason, this commit does not change the SIMD intrinsics used in boost::uuid operations to their aligned equivalents. Using unaligned load/store instructions on actually aligned memory location on modern CPUs has the same performance as using aligned instructions, so the performance benefit is still there.

Closes https://github.com/boostorg/uuid/issues/139.

Lastique commented 4 months ago

I've updated the PR to only require alignment of 4 on 32-bit platforms. At least, on Windows, 32-bit processes only get 4-byte aligned stack.

Lastique commented 4 months ago

@pdimov What's the purpose of test_alignment? Is it ok to just test that offsetof % alignof(uuid) == 0?

pdimov commented 4 months ago

It's something that works today.

Lastique commented 4 months ago

Do you mean that alignof(uuid) is guaranteed (i.e. documented) to be 1? I don't remember seeing this in the docs.

In any case, my question was what was the intended tested feature in this test. IOW, are you testing that alignof(uuid) is exactly 1 or are you testing that the compiler is able to align uuid in a struct by adding padding? Depending on what is it you're testing, the test could be updated differently, or the PR could be rejected altogether.

pdimov commented 2 months ago

I don't think libraries should overalign their public types, sorry.

I tried to align uuid like __uint128_t, but this type is also overaligned on some platforms, so I left it at std::uint64_t.

Maybe one day we'll get std::uint128_t which will be a standard integer type and hence not overaligned, but until that happens, uint64_t it is.