Consider a version of NS_DECL_ISUPPORTS_INHERITED that has 0 overhead when built without refcount logging

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(5 attachments)

If class A inherits from B, but doesn't implement any additional interfaces, we often still NS_DECL_ISUPPORTS_INHERITED on it to get better refcount logging.

How about if we add a macro which decls and impls nsISupports with no extra interfaces but only if NS_BUILD_REFCNT_LOGGING.  Otherwise it expands to nothing.  That way we get 0 overhead in release builds but we can still get nicer refcount logs.

We can use this for some event classes after bug 1436508.
Depends on: 1436508
That seems like a good idea. I always assumed that PGO would be smart enough to eliminate the overhead, but there's no need to rely on that.
Do you have any suggested names?  I'd also point out that it's likely to be a bit of a hazard for causing opt-only compilation errors that developers won't see until they push to try, so it would be good if the name helps avoid that.

Should it just be NS_DECL_ISUPPORTS_INHERITED0, to match the NS_IMPL_* we already have?  And should we just make NS_IMPL_ISUPPORTS_INHERITED0 require use of NS_DECL_ISUPPORTS_INHERITED0?
I was going to name it NS_DECL_REFCOUNTING_FOR_LOGGING_ONLY, with an "inline" impl to reduce the amount of boilerplate we need to have and to make it clear why it's there.  But I could be convinced on other names and maybe on the two-macro thing.

> I'd also point out that it's likely to be a bit of a hazard for causing opt-only
> compilation errors that developers won't see until they push to try

I did think about that.  That's another benefit of having it be a header-only macro: you can't accidentally NS_DECL_WHATEVER and then not provide an implementation and have it compile in a debug build.
> you can't accidentally NS_DECL_WHATEVER and then not provide an implementation
> and have it compile in a debug build.

Or the other way around, write an impl that's there all the time but have the decl disappear in opt builds.
Fwiw, I have local patches for this; I figure we should sort out the naming before I post them.
Assignee: nobody → bzbarsky
Per discussion with mccr8 on irc, we're leaning toward NS_INLINE_DECL_REFCOUNTING_INHERITED by analogy with NS_INLINE_DECL_REFCOUNTING.
This did show a situation where we can get opt-only bustage: if we have multiple inheritance and some of the things we inherit from have pure virtual refcounting...  I'm not sure what to do about mitigating that.  :(
Attachment #8949950 - Flags: review?(continuation)
I looked at the diff in the generated code, and the only change is that
SpeechRecognitionEvent's unlink impl goes from:

 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(SpeechRecognitionEvent, Event)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mResults)
   tmp->mInterpretation.setUndefined();
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mEmma)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END

to:

 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(SpeechRecognitionEvent, Event)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mResults)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mEmma)
+  tmp->mInterpretation.setUndefined();
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END

MozReview-Commit-ID: FISIphljlT6
Attachment #8949951 - Flags: review?(continuation)
This makes 23 generated event clases use NS_INLINE_DECL_REFCOUNTING_INHERITED.

MozReview-Commit-ID: 7dyWTc1AUfH
Attachment #8949952 - Flags: review?(continuation)
Attachment #8949948 - Flags: review?(continuation) → review+
Comment on attachment 8949949 [details] [diff] [review]
part 2.  Use NS_INLINE_DECL_REFCOUNTING_INHERITED for some classes that have otherwise-empty QI impls

Review of attachment 8949949 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8949949 - Flags: review?(continuation) → review+
Comment on attachment 8949950 [details] [diff] [review]
part 3.  Replace usage of NS_IMPL_ISUPPORTS_INHERITED0 with NS_INLINE_DECL_REFCOUNTING_INHERITED when possible

Review of attachment 8949950 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/windows/msaa/ARIAGridAccessibleWrap.h
@@ +31,5 @@
>    // IUnknown
>    DECL_IUNKNOWN_INHERITED
>  
>    // nsISupports
> +  // Need to declare addref/release here unconditionall, because

nit: "unconditionally"

@@ +55,5 @@
>    // IUnknown
>    DECL_IUNKNOWN_INHERITED
>  
>    // nsISupports
> +  // Need to declare addref/release here unconditionall, because

nit: "unconditionally"
Attachment #8949950 - Flags: review?(continuation) → review+
Comment on attachment 8949951 [details] [diff] [review]
part 4.  Refactor event codegen a bit to make it easier to tell whether the event class needs cycle collection

Review of attachment 8949951 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +17885,5 @@
> +                retVal += "  tmp->" + name + ".setUndefined();\n"
> +            elif m.type.isObject() or m.type.isSpiderMonkeyInterface():
> +                retVal += "  tmp->" + name + " = nullptr;\n"
> +            else:
> +                raise TypeError("Uknown traceable member type %s" % m.type)

nit: "Unknown"

@@ +17904,5 @@
> +                dropJS += member + " = JS::UndefinedValue();\n"
> +            elif m.type.isObject() or m.type.isSpiderMonkeyInterface():
> +                dropJS += member + " = nullptr;\n"
> +            else:
> +                raise TypeError("Uknown traceable member type %s" % m.type)

nit: "Unknown"
Attachment #8949951 - Flags: review?(continuation) → review+
Comment on attachment 8949952 [details] [diff] [review]
part 5.  Change generated event classes to use NS_INLINE_DECL_REFCOUNTING_INHERITED when possible

Review of attachment 8949952 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +17851,5 @@
> +                NS_INLINE_DECL_REFCOUNTING_INHERITED(${nativeType}, ${parentType})
> +                """,
> +                nativeType=self.nativeType,
> +                parentType=self.parentType)
> +    

nit: trailing whitespace.
Attachment #8949952 - Flags: review?(continuation) → review+
> nit: "unconditionally"

Fixed, both places.

> nit: "Unknown"

Fixed, both places.

> nit: trailing whitespace.

Fixed.

Comment 17

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68bab9b6f40b
part 1.  Add a new NS_INLINE_DECL_REFCOUNTING_INHERITED macro that declares addref/release only for logging purposes.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/606c47199b30
part 2.  Use NS_INLINE_DECL_REFCOUNTING_INHERITED for some classes that have otherwise-empty QI impls.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d647c847e5
part 3.  Replace usage of NS_IMPL_ISUPPORTS_INHERITED0 with NS_INLINE_DECL_REFCOUNTING_INHERITED when possible.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e3ae4647ac
part 4.  Refactor event codegen a bit to make it easier to tell whether the event class needs cycle collection.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1757b17e21b5
part 5.  Change generated event classes to use NS_INLINE_DECL_REFCOUNTING_INHERITED when possible.  r=mccr8
You need to log in before you can comment on or make changes to this bug.