micro-optimize runnable refcounting in some cases

RESOLVED FIXED in Firefox 50

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(6 attachments)

We can avoid a little bit of addref/release traffic for some runnables.
Created attachment 8764394 [details] [diff] [review]
part 1 - move nsRunnableMethod* into mozilla::detail and rename them

Less ns-prefixing is more better.  Also, this renaming makes clearer
that these classes are private implementation details, which is good,
because we're going to take advantage of that fact in a bit.
Attachment #8764394 - Flags: review?(khuey)
Created attachment 8764395 [details] [diff] [review]
part 2 - make RunnableMethod* internals final classes

This step is mostly tidiness.
Attachment #8764395 - Flags: review?(khuey)
Created attachment 8764396 [details] [diff] [review]
part 3 - make various things accept already_AddRefed<nsIRunnable>

All of these things are called with the result of
NS_NewRunnableFunction, so we need to transition them over to a world
where NS_NewRunnableFunction returns something different.
Attachment #8764396 - Flags: review?(khuey)
Created attachment 8764397 [details] [diff] [review]
part 4 - change NS_NewRunnableFunction to return already_AddRefed

This change makes it more consistent with NS_NewRunnableMethod and also
opens up optimization opportunities for later.
Attachment #8764397 - Flags: review?(khuey)
Created attachment 8764398 [details] [diff] [review]
part 5 - move nsRunnableFunction into mozilla::detail and rename it

We do this for similar reasons as nsRunnableMethod*: less prefixing and
a more obvious signal that this is a private implementation class.
Attachment #8764398 - Flags: review?(khuey)
Created attachment 8764399 [details] [diff] [review]
part 6 - introduce more efficient initialization of runnable refcounts

When we have:

  class Foo : public Runnable { ... };

  RefPtr<Runnable> f = new Foo(...);

we initialize Foo's refcount to zero, and then immediately call AddRef,
which increments the refcount (atomically!) and performs a virtual
method call to do so.  A good compiler would devirtualize the call, but
that call might not get inlined.

In any event, we are wasting work; in a number of cases, we can simply
initialize Runnable's refcount to 1 when we create it, which saves a
virtual call and an atomic increment.  Since this is sort of a delicate
operation, this patch restricts doing so to classes that are
behind-the-scenes classes of higher-level utilities.

There's a small codesize win, ~5K on x86-64 Linux.
Attachment #8764399 - Flags: review?(khuey)
Comment on attachment 8764394 [details] [diff] [review]
part 1 - move nsRunnableMethod* into mozilla::detail and rename them

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

::: xpcom/glue/nsThreadUtils.h
@@ +705,5 @@
>  } /* namespace detail */
>  
> +namespace mozilla {
> +
> +namespace detail {

Er, there's another namespace detail above that just ended.  Is that namespace just ::detail, and this is ::mozilla::detail?  Or is this one actually ::mozilla::mozilla::detail?  Regardless, it'd be nice if this file only had one detail namespace if feasible.
Attachment #8764394 - Flags: review?(khuey) → review+
Attachment #8764395 - Flags: review?(khuey) → review+
Comment on attachment 8764396 [details] [diff] [review]
part 3 - make various things accept already_AddRefed<nsIRunnable>

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

::: xpcom/threads/SyncRunnable.h
@@ +42,5 @@
>  
> +  explicit SyncRunnable(already_AddRefed<nsIRunnable> aRunnable)
> +    : mRunnable(Move(aRunnable))
> +    , mMonitor("SyncRunnable")
> +    , mDone(false)

Delegating constructors can't come soon enough.
Attachment #8764396 - Flags: review?(khuey) → review+
Attachment #8764397 - Flags: review?(khuey) → review+
Attachment #8764398 - Flags: review?(khuey) → review+
Comment on attachment 8764399 [details] [diff] [review]
part 6 - introduce more efficient initialization of runnable refcounts

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

I'm not going to say no, but this feels kinda icky :/  5K codesize win sounds like it's being optimized out in the vast majority of cases.

If you want to do it I won't stop you though.
Attachment #8764399 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> Comment on attachment 8764394 [details] [diff] [review]
> part 1 - move nsRunnableMethod* into mozilla::detail and rename them
> 
> Review of attachment 8764394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/glue/nsThreadUtils.h
> @@ +705,5 @@
> >  } /* namespace detail */
> >  
> > +namespace mozilla {
> > +
> > +namespace detail {
> 
> Er, there's another namespace detail above that just ended.  Is that
> namespace just ::detail, and this is ::mozilla::detail?  Or is this one
> actually ::mozilla::mozilla::detail?

The namespace just above is ::detail.  I agree that it would be nicer to get rid of that, but I'll have to think about how that works, since some of the parameter storage stuff is nominally meant to be user-visible.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Delegating constructors can't come soon enough.

I thought we already had them?
https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
Comment on attachment 8764399 [details] [diff] [review]
part 6 - introduce more efficient initialization of runnable refcounts

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

::: xpcom/threads/TimerThread.cpp
@@ +717,5 @@
>    // the caller. We need to copy the generation number from this timer into the
>    // event, so we can avoid firing a timer that was re-initialized after being
>    // canceled.
>  
> +  RefPtr<nsTimerEvent> event = dont_AddRef(new nsTimerEvent);

It is a bit tricky to know whether it should be:
RefPtr<Foo> foo = new Foo;
or RefPtr<Foo> foo = dont_AddRef(new Foo);
without looking into the implementation of Foo.
Yup.  I think that's why Nathan wanted to limit it to a few "infrastructure" classes.  I'd feel better if there were some sort of MakeAddRefed<Foo>(runnableArgs) function though.
Yeah, this technique is a bit dangerous if people don't know what's going on.  This is one of those times when mandating no direct |new| usage and requiring static |Create| functions (ala Webkit?) would be useful.

The motivation behind this is bug 1277709, where we see a decent pageload win on ARMv8 devices by relaxing the memory ordering of refcounting.  I was trying to see if we could get some benefits of that win by simply doing less work elsewhere and retaining our memory ordering semantics.

Comment 15

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14345389a69a
part 1 - move nsRunnableMethod* into mozilla::detail and rename them; r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e317ff236ec
part 2 - make RunnableMethod* internals final classes; r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba3ae25c8ff
part 3 - make various things accept already_AddRefed<nsIRunnable>; r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b499433ebd
part 4 - change NS_NewRunnableFunction to return already_AddRefed; r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5cba49844ee
part 5 - move nsRunnableFunction into mozilla::detail and rename it; r=khuey
I landed the first five patches for cleanup purposes; I left off the sixth.

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14345389a69a
https://hg.mozilla.org/mozilla-central/rev/5e317ff236ec
https://hg.mozilla.org/mozilla-central/rev/aba3ae25c8ff
https://hg.mozilla.org/mozilla-central/rev/62b499433ebd
https://hg.mozilla.org/mozilla-central/rev/c5cba49844ee
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.