Use uintXX_t instead of size_t for items/bytes/iterations #507
Conversation
|
✅ Build benchmark 927 completed (commit da546b801d by @Maratyszcza) |
include/benchmark/benchmark.h
Outdated
|
|
||
| BENCHMARK_ALWAYS_INLINE | ||
| size_t iterations() const { return (max_iterations - total_iterations_) + 1; } | ||
| uint32_t iterations() const { return (max_iterations - total_iterations_) + 1; } |
There was a problem hiding this comment.
This change seems unnecessary. On a 32-bit system it has no effect and on a 64-bit system there is no harm in using size_t. Same comment for all the places that touch iterations counters.
There was a problem hiding this comment.
With this change, if a benchmark has an overflow bug on 32-bit systems, it will also manifest on a 64-bit system. E.g. state.SetItemsProcessed(1000000 * state.iterations()) would have identical overflow behavior on both 32-bit and 64-bit systems.
There was a problem hiding this comment.
And I claim that it's a bad property.
I happen to only ever run benchmarks on 64-bit systems. Why would you force me to have to deal with 32-bit overflows just for a misconceived notion of uniformity?
|
✅ Build benchmark 928 completed (commit 7afcca4fc8 by @Maratyszcza) |
|
I changed interface for |
|
✅ Build benchmark 929 completed (commit 8b4f11e1f9 by @Maratyszcza) |
|
for one thing, these should not be explicitly unsigned unless we're representing bitfields or closed-group arithmetic. We're not, so we should be using signed integers if we're going to move away from the standard size_t type (which is unsigned by the standard, but that was an unfortunate decision). while signed integer overflow is undefined behaviour, this is detectable with ubsan and is safer (in a way) than the wrapping that unsigned integers would give us. ie, i'm comfortable with the bytes/items processed being intxx_t instead of size_t, but not uintxx_t. |
|
@Maratyszcza Thanks for the patch. Don't worry about ABI breaks. The project is free to break the ABI. First, I agree with @dominichamon about using signed integer types. And I agree about making this change for things we explicitly want to be 64 bits. However, I don't know if using explicit 32 bit types makes sense. In these cases, it seems to make the most sense to simply use |
9082adf to
be535f8
Compare
|
✅ Build benchmark 930 completed (commit 08a0561a14 by @Maratyszcza) |
|
@dominichamon I changed types in the interface to @EricWF Using |
include/benchmark/benchmark.h
Outdated
| bool started_; | ||
| bool finished_; | ||
| size_t total_iterations_; | ||
| uint32_t total_iterations_; |
There was a problem hiding this comment.
i buy your 'int' argument and if we need a guarantee, which we do, then this should be ?int32_t. The Google C++ style guide, interestingly, states that we can assume an int is at least 32 bits, which is a bit surprising.
However, in the same section it also specifically recommends avoiding unsigned unless we need overflow modulo 2^N or representing bit patterns. Given we're doing neither, this should be signed.
So: int32_t please. And then promoting once to int64_t in 'iterations' seems reasonable, though i'd promote the individual parts of the calculation. ie, max_iterations - total_iterations_ could be INT_MAX already if total is 0. the +1 will cause an overflow before you get the chance to promote to int64.
There was a problem hiding this comment.
I'm sorry, I don't buy the int vs. int32_t argument.
According to cppreference, int can only be a 16-bit type on LP32 systems, e.g., Win16 or, maybe, some microcontrollers. We have no evidence that the library was ever ported to such a system. If software hasn't been ported, it's not portable, period. The code is choke-full of usages of int so there is no point in an illusory attempt at improving it by using int32_t in a handful of places.
If someone wanted to port the library to Win16 that might be an interesting exercise, but that's not what this PR does.
There was a problem hiding this comment.
++pleroy. @Maratyszcza I maintain an STL, and I honestly I don't think we even worry about 16 bit integers. Let's go with the Google style guide and assume that int is at least 32 bits.
|
✅ Build benchmark 931 completed (commit cdd596fd8d by @Maratyszcza) |
0e93a18 to
33a3e03
Compare
|
❌ Build benchmark 936 failed (commit 62340aac82 by @Maratyszcza) |
7a76701 seemingly unintentionally changed the data types for bytes_processed, items_processed, and iterations to size_t. On 32-bit systems, the bytes_processed and items_processed variables easily overflow, and often report fewer items/second for faster code. The fact that overflow chances depends on architecture and even ABI (e.g. x64 vsi x32 ABI on x86-64) makes it particularly annoying. iterations counter is made to use int32_t internally because it is incremented in the hot loop, and incrementing int64_t is slow on some 32-bit platforms (e.g. Asm.js). However, the public interface State.iterations() is changed to return int64_t.
|
✅ Build benchmark 937 completed (commit fefdcd5b80 by @Maratyszcza) |
|
I think this has been obsoleted by other changes. |
7a76701 seemingly unintentionally changed the data types for bytes_processed, items_processed, and iterations to size_t. On 32-bit systems, the bytes_processed and items_processed variables easily overflow, and often report fewer items/second for faster code. The fact that overflow chances depends on architecture and even ABI (e.g. x64 vs x32 ABI on x86-64) makes it particularly annoying. iterations counter is made to use uint32_t because it is incremented in the hot loop, and incrementing uint64_t is slow on some 32-bit platforms (e.g. Asm.js).