Open
Bug 1128091
Opened 10 years ago
Updated 2 years ago
Add mfbt/Thread.h for easy "is on same thread as before" checks
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
ASSIGNED
People
(Reporter: mstange, Assigned: mstange)
Details
Attachments
(1 file, 1 obsolete file)
I wanted to add an assert of the style
MOZ_ASSERT(mOwningThread == NS_GetCurrentThread());
to PseudoStack.h and became very sad when it wouldn't compile in some parts of the tree that don't have MOZILLA_INTERNAL_API defined. (The specific file that blew up was media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp.)
This patch adds mfbt/Thread.h so you can do MOZ_ASSERT(mOwningThread == mozilla::ThisThread::Id()). It is modeled after std::this_thread::id().
Attachment #8557367 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•10 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #0)
> I wanted to add an assert of the style
> MOZ_ASSERT(mOwningThread == NS_GetCurrentThread());
> to PseudoStack.h and became very sad when it wouldn't compile in some parts
> of the tree that don't have MOZILLA_INTERNAL_API defined. (The specific file
> that blew up was
> media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp.)
I realize now that this is a bad example as far as justification for Thread.h goes, because I can just fix my problem by shuffling code between some .h and .cpp files. But I think the patch has value anyway.
Comment 2•10 years ago
|
||
Comment on attachment 8557367 [details] [diff] [review]
patch
Review of attachment 8557367 [details] [diff] [review]:
-----------------------------------------------------------------
So, sadly, there's already a bunch of work done on this in bug 991710, that floundered for lack of time on my part. And you haven't addressed all the complexity people wanted for this.
Specifically people wanted universally unique thread ids, which GetCurrentThreadId() does not give (nor, I believe, is pthread_self() guaranteed to give). The approach that seems best to me is just to use a ThreadLocal<uintptr_t> to store each thread's id, then initialize it from an Atomic<uintptr_t> that increments until it's rolled around. Unfortunately once you overflow you lose uniqueness guarantees. Maybe we should just assume that's impossible? And crash if it happens? I tried a more involved approach in the patch there, that I guess is kind of overkill, for a 32-bit-only problem.
Anyway. I'll copy my patch over to this bug, then you can finish it up. I guess we should probably do ThreadId for now and deal with having a Thread class and Thread::id for later. And the oddities of having an always-64-bit counter variable, we can just not deal with, because on 32-bit I think it probably just requires a lock (which just isn't worth it).
Attachment #8557367 -
Flags: review?(jwalden+bmo) → review-
Comment 3•10 years ago
|
||
Probably you can just remove a bunch of nonsense complexity in this, then resubmit it and I (or someone else, depending how much we care about self-review, tho older versions of that patch were reviewed by others) can quickly stamp it.
Attachment #8557367 -
Attachment is obsolete: true
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•