Closed
Bug 1191063
Opened 9 years ago
Closed 9 years ago
Followup SharedThreadPool nits from bug 1190496
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Assignee | ||
Comment 1•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #11) > SharedThreadPool::Get should have a main thread assertion to back up the > comment in SharedThreadPool.h. I actually removed that restriction in bug 1177282. I'll update the comment. > Now that we have nsThreadPool and SharedThreadPool, we should have some > documentation somewhere (headers, preferably) describing why you might > prefer one or the other. This mostly exists at the top of SharedThreadPool.h, but I'll expand a bit. > EnsureThreadLimitIsAtLeast should probably ditch the reference to "thread > that we use for media". That already happened in bug 1190496. > I guess SharedThreadPool::AddRef/Release can't be done via the standard > XPCOM macros, even with threadsafe ones, since they require locking? Correct. That's > a bummer. I suggest swapping the stabilization of the refcount with the > dispatch of the event to the main thread in Release, though, and using a > NonOwning runnable method, to make things a little less confusing... Can you explain what you mean here? The runnable target is |mPool| (not |this|), so I don't see how we could use a NonOwning Runnable.
Flags: needinfo?(nfroyd)
Comment 2•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #1) > (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #11) > > SharedThreadPool::Get should have a main thread assertion to back up the > > comment in SharedThreadPool.h. > > I actually removed that restriction in bug 1177282. I'll update the comment. Ah, the problem of keeping the code in sync with the comments. =/ > > Now that we have nsThreadPool and SharedThreadPool, we should have some > > documentation somewhere (headers, preferably) describing why you might > > prefer one or the other. > > This mostly exists at the top of SharedThreadPool.h, but I'll expand a bit. Thank you! I guess "Users aren't required to manually shutdown the pool, and can release references on any thread" might be justification enough, but I think it's nice to spell things out in a little more detail. > > EnsureThreadLimitIsAtLeast should probably ditch the reference to "thread > > that we use for media". > > That already happened in bug 1190496. Doh. I tried looking at my inbound copy, and saw the comment there, and I skimmed through the patches in that bug and didn't see any changes. I didn't skim hard enough, my mistake! > > I suggest swapping the stabilization of the refcount with the > > dispatch of the event to the main thread in Release, though, and using a > > NonOwning runnable method, to make things a little less confusing... > > Can you explain what you mean here? The runnable target is |mPool| (not > |this|), so I don't see how we could use a NonOwning Runnable. Argh, I read carefully enough to generate nits, but not carefully enough to figure out what was going on. You're right, ignore me.
Flags: needinfo?(nfroyd)
Comment 4•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5c158420a7fa
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•