Followup SharedThreadPool nits from bug 1190496

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

(Assignee)

Comment 1

4 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)
(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)
https://hg.mozilla.org/mozilla-central/rev/5c158420a7fa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.