Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Jul 1, 2021

Followup to #10632

@github-actions
Copy link

github-actions bot commented Jul 1, 2021

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I did a bit of research on this out of curiosity. TL;DR: this should be safe enough.

std::thread::id is often a number, sometimes it is a pointer to a struct, and sometimes it is a struct (https://github.com/nginx/unit/blob/master/src/nxt_thread_id.h attempts to catalogue all the possible methods). I couldn't find any example where it is larger than 8 bytes. This approach has some precedent. OpenSSL originally required thread_id be coerced into unsigned long and now it has an implementation where it is either unsigned long or a pointer. However, I think that had to be done because on some systems a pointer may be larger than unsigned long. So I think uint64_t is safe since a pointer should never be larger than 8 bytes.

One potential concern is that technically == could be overloaded so that two threads could in theory have the same uint64_t representation but not be equal. One case is when a thread id is a pointer and that pointer is reused. However, the spec for std::thread states Once a thread has finished, the value of std::thread::id may be reused by another thread so this is already a given.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't see any reason this wouldn't work but why not just use the uint64_t version of std::thread::id()? Is it to allow this to be constexpr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I hadn't thought of that. But it seems simpler to use a hardcoded number like this.

@pitrou pitrou force-pushed the ARROW-13244-current-tid branch from 5958ee3 to 4dce506 Compare July 5, 2021 14:23
@pitrou
Copy link
Member Author

pitrou commented Jul 5, 2021

Rebased, will merge if green.

@pitrou pitrou closed this in 905809c Jul 5, 2021
@pitrou pitrou deleted the ARROW-13244-current-tid branch July 5, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants