1281626 - micro-optimize runnable refcounting in some cases

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

We can avoid a little bit of addref/release traffic for some runnables.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - make RunnableMethod* internals final classes

This step is mostly tidiness.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 12

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 14

[Nathan Froyd [:froydnj]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Nathan Froyd [:froydnj]]

I landed the first five patches for cleanup purposes; I left off the sixth.

Comment 17

[Carsten Book [:Tomcat]]

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