Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upUse `thread-id` in thief_id call #347
Conversation
cuviper
reviewed
May 25, 2017
|
The change here looks good. My only hesitation is that we were talking on gitter about possibilities for an extended Well, plus we need to verify that there's no performance regression in this. |
| @@ -147,8 +148,7 @@ impl Splitter { | |||
| fn thief_id() -> usize { | |||
| // The actual `ID` value is irrelevant. We're just using its TLS | |||
| // address as a unique thread key, faster than a real thread-id call. | |||
| thread_local!{ static ID: bool = false } | |||
| ID.with(|id| id as *const bool as usize) | |||
| thread_id::get() | |||
This comment has been minimized.
This comment has been minimized.
cuviper
May 25, 2017
Member
The comment should be updated too, like:
// The actual `ID` value is irrelevant, as long as it's unique.
hodgesds
force-pushed the
hodgesds:thread-id
branch
from
0821df2
to
40a936f
May 25, 2017
This comment has been minimized.
This comment has been minimized.
|
Thanks for the comments, I'll try to take a look at doing some performance testing tonight. |
This comment has been minimized.
This comment has been minimized.
|
As it happens, I just made a wiki page about benchmarking that you may find useful. I can try to do some measurements too. |
This comment has been minimized.
This comment has been minimized.
|
This is superceded by #354, right? I think I prefer that approach. |
This comment has been minimized.
This comment has been minimized.
|
(To be clear, I appreciate the PR @hodgesds -- just seems better if we can avoid the dependency and make things a bit cleaner in the bargain. =) Sorry for the long delay btw. |
hodgesds commentedMay 24, 2017
Hi, I'm new and wanted to help out a little if I can. I might not have followed all the conversation in the related issue but happy to help out if I can.
Resolves: #335