1612721 - Build warning spam from MediaUtils.h

Status: RESOLVED INVALID

(Core :: Audio/Video, task, P3)

Description

[Masatoshi Kimura [:emk]]

 9:36.75 e:/m/mozilla-unified/obj-x86_64-pc-mingw32/dist/include\mozilla/media/MediaUtils.h(175,43): warning: performance issue: temporary 'RefPtr<nsIAsyncShutdownClient>' is only dereferenced here once which involves short-lived AddRef/Release calls
 9:36.84   ~ShutdownTicket() { GetShutdownBarrier()->RemoveBlocker(mBlocker); }
 9:36.93                                           ^
 9:37.01 e:/m/mozilla-unified/obj-x86_64-pc-mingw32/dist/include\mozilla/media/MediaUtils.h(175,43): note: consider changing function mozilla::media::GetShutdownBarrier to return a raw reference instead (be sure that the pointee is held alive by someone else though!)

Please either fix this or suppress the warning if this is a false positive.

Comment 1

[Masatoshi Kimura [:emk]]

Bug 1407415 changed the return type of GetShutdownBarrier() from already_AddRefed to RefPtr.

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

:jib, any preference for how we fix this?

Comment 3

[Back Aug 24 [:jib]]

This is a false positive in the sense that there's no performance win to be had here. Look at MediaUtils.h's GetShutdownBarrier:

RefPtr<nsIAsyncShutdownClient> GetShutdownBarrier() {
  nsCOMPtr<nsIAsyncShutdownService> svc = services::GetAsyncShutdown();

  RefPtr<nsIAsyncShutdownClient> barrier;
  nsresult rv = svc->GetProfileBeforeChange(getter_AddRefs(barrier));
  ...
  return barrier;
}

Here,

1. GetProfileBeforeChange returns an already-addRef'ed object. Its refcount is 1.
2. The already-addRef'ed object is put in barrier. Its refcount is still 1.
3. Then, barrier is returned using move-semantics e.g. to here, so its refcount is still 1:

~ShutdownTicket() { GetShutdownBarrier()->RemoveBlocker(mBlocker); }

4. Then RemoveBlocker is called on it. Its refcount is still 1.
5. Once the call is done, the refcount goes to 0.

Ref-countable objects in our platform inherently need to be AddRef'ed and Release'd at least once to exist.

There seems to be an assumption behind this warning that returning RefPtrs always or even often cause needless ref-counting. This may have been true before move semantics were prevalent, but I don't think that assumption holds in modern c++ code.

Proposal: remove warning

I'd like to suggest we remove this warning, or at least reduce it to a an optional linting step.

I see this warning did catch at least one nonoptimal case in bug 1609638 comment 5, but looking at it, I think the bug in that case was having a member (mThread) whose getter always incremented it. I'd suggest a warning that more directly detects that pattern.

Comment 4

[Simon Giesecke [:sg] [he/him]]

I implemented the warning after discussion with bzbarsky resulting in that hiding the acquisition of a strong reference at the call site is an anti-pattern. Acqusition of a strong reference may be required or at least hard to change. Removing the build warning and only doing this during linting/reviewbot is an option. However, as suggested by https://bugzilla.mozilla.org/show_bug.cgi?id=1609638#c11 all occurrences of this anti-pattern should be removed.

At least the latter (hard to change) may be true here, but I am not sure what GetProfileBeforeChange really does. However, in that case, the calling code should be changed to:

~ShutdownTicket() {
  const RefPtr<nsIAsyncShutdownClient> shutdownBarrier = GetShutdownBarrier();
  shutdownBarrier->RemoveBlocker(mBlocker);
}

This way it will be visible when reading the code that a strong reference is acquired. This will also remove the warning. Do you think this is an acceptable change?

Comment 5

[Back Aug 24 [:jib]]

No, I do not think that is acceptable, because I haven't heard a benefit from imposing this new rule (the one positive bug found was not at the call-site being warned about, but in the getter being used. The call-site was not at fault, and what the warning said to fix, would not have mitigated it).

I'm also concerned we're moving goalposts. The original reason given was "unperformant uses of temporary RefPtr" in bug 1609638—which I show in comment 3 isn't so, at least in what I think is a common sub-set of cases. I'd like to hear a response to my challenge to this premise, before moving on to whether there is an "anti-pattern" here.

I suspect opinions differ about which of the following is superior:

~ShutdownTicket() {
  const RefPtr<nsIAsyncShutdownClient> shutdownBarrier = GetShutdownBarrier();
  shutdownBarrier->RemoveBlocker(mBlocker);
}

~ShutdownTicket() {
  GetShutdownBarrier()->RemoveBlocker(mBlocker);
}

I suspect the breakdown correlates strongly to those who disagree over:

~ShutdownTicket() {
  auto shutdownBarrier = GetShutdownBarrier();
  shutdownBarrier->RemoveBlocker(mBlocker);
}

But if someone has a reason that goes beyond readability, I'd like to hear it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I think what we really want is to catch two patterns (maybe others? Feedback welcome):

1. Getters that addref "for no good reason".
2. Code that calls addreffing getters (ones that do it for good reason) multiple times in a row instead of caching the return value and avoiding extra refcounts. Think foo->GetBar()->X(); foo->GetBar()->Y(); where GetBar addrefs.

Looking at my build, it seems like the current analysis is catching a much higher proportion of false positives than I am happy with. Is there a way to tweak it to more narrowly focus on those two issues?

Comment 7

[Simon Giesecke [:sg] [he/him]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)

So I think what we really want is to catch two patterns (maybe others? Feedback welcome):

1. Getters that addref "for no good reason".
2. Code that calls addreffing getters (ones that do it for good reason) multiple times in a row instead of caching the return value and avoiding extra refcounts. Think foo->GetBar()->X(); foo->GetBar()->Y(); where GetBar addrefs.

Looking at my build, it seems like the current analysis is catching a much higher proportion of false positives than I am happy with. Is there a way to tweak it to more narrowly focus on those two issues?

I fear that determining 1) is not (easily) possible automatically. However, an annotation mihht be added to declare that a function intentionally addrefs, which could be applied to GetShutdownBarrier in particular.

I also fear that 2) is hard to determine automatically. While foo->GetBar() is kind of a common subexpression here, it can't be automatically established that this always returns a RefPtr to the same object.

I will give the existing occurrences a closer look considering this.

I am also open for any other suggestions on improving this check.

Personally I think making RefPtfs visible at the callsite is a good thing, but I don't want to enforce this view on everyone.

Comment 8

[Masatoshi Kimura [:emk]]

Personally I think making RefPtfs visible at the callsite is a good thing, but I don't want to enforce this view on everyone.

The current warning spams virtually enforce your view. If you have no good idea how to reduce false positives at hand, warnings should be disabled (at least by default) until the check is improved.

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1612721 - Annotate GetShutdownBarrier with MOZ_MUST_ADDREF. r=andi

Depends on D62292

Comment 10

[Back Aug 24 [:jib]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)

2. ... foo->GetBar()->X(); foo->GetBar()->Y(); where GetBar addrefs.

Have the current warnings found this problem to be prevalent?

Comment 11

[Simon Giesecke [:sg] [he/him]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #10)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)

2. ... foo->GetBar()->X(); foo->GetBar()->Y(); where GetBar addrefs.

Have the current warnings found this problem to be prevalent?

I have seen instances of this, but cannot tell right now what share these make. I will provide an answer as part of Bug 1613418.

Comment 12

[Simon Giesecke [:sg] [he/him]]

I think this is no longer happening. Please reopen if it's still an issue.