Closed Bug 1281626 Opened 4 years ago Closed 4 years ago

micro-optimize runnable refcounting in some cases

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(6 files)

We can avoid a little bit of addref/release traffic for some runnables.
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)
This step is mostly tidiness.
Attachment #8764395 - Flags: review?(khuey)
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)
This change makes it more consistent with NS_NewRunnableMethod and also
opens up optimization opportunities for later.
Attachment #8764397 - Flags: review?(khuey)
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)
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+
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+
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.
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.
You need to log in before you can comment on or make changes to this bug.