Open Bug 1449290 Opened 6 years ago Updated 1 year ago

Consider having runnable not inherit from nsISupports

Categories

(Core :: XPCOM, enhancement)

enhancement

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:>100k])

Right now every runnable class has 4 pointers in its vtable (QI, AddRef, Release, Run).  And we have a lot of runnable classes.

If we changed runnables to not be nsISupports at all and use WebIDL bindings, we could switch to inline AddRef/Release on nsRunnable.  QI would go away, though we'd need to figure out what to do about nsIRunnablePriority and nsILabelableRunnable and so forth.  We'd pick up a virtual WrapObject from the wrapper cache... though if we wanted to we could mark this a non-wrapper-cached interface, as long as we're OK with a brand-new reflector being created any time a runnable is reflected into script.

We'd presumably at least need bug 1444991 fixed first.
> as long as we're OK with a brand-new reflector being created any time a runnable is reflected into script

And an open question about how XPConnect would manage to reflect such a thing....

Also, right now JS functions can just implement nsIRunnable, via XPConnect.  Changing that might be annoying.  :(
I think we QI nsIRunnable to nsICancelableRunnable in some places...
I feel like many runnables are never exposed to JS, so you could get some benefit from a non-nsISupports runnable class, but it might be a lot of effort to figure out which is which.
jrmuizel has asked for something like this before, that runnables essentially require Run() and nothing else.  I don't know if that plan would work very well, though.

Another possibility along the same lines is to have a C++ side NGRunnable that's refcounted, but not QI'able, so we'd only have (non-virtual) AddRef/Release, virtual Run and probably a virtual destructor (which mozilla::Runnable has today).  Everything on the C++ side would deal with NGRunnable.  Anything that dispatched runnables from script would use nsIRunnables, and would get wrapped into NGRunnable.  This scheme would have some time/space overhead for such runnables, but presumably we're not dispatching *tons* of runnables directly from script?

Either way, I think the wrinkle is working out what to do about our pervasive QI'ing to nsICancelableRunnable, nsIRunnablePriority, etc.
(In reply to Andrew McCreight [:mccr8] from comment #3)
> I feel like many runnables are never exposed to JS, so you could get some
> benefit from a non-nsISupports runnable class, but it might be a lot of
> effort to figure out which is which.

Great minds think alike! :)

My off-the-cuff opinion is that C++ code that hands out nsIRunnables to script is probably doing something wrong, and we should fix such instances.  I don't suppose we have some sort of XPConnect blacklist for forbidding certain interfaces from being handed out?  I guess that might be difficult, because presumably incoming nsIRunnables are wrapped in the same place as outgoing nsIRunnables...
> I don't suppose we have some sort of XPConnect blacklist for forbidding certain interfaces from being handed out?

We don't, but we could certainly add one easily at slight performance cost.

> because presumably incoming nsIRunnables are wrapped in the same place as outgoing nsIRunnables

No, JS-to-C++ and C++-to-JS are in different places.  We could, for example, add a check in nsScriptSecurityManager::CanCreateWrapper, or directly in XPCConvert::NativeInterface2JSObject.
It may also be desirable to move away from the vtable model for runnables, and hold the function pointer for the callback directly inline, avoiding stashing it in static storage alltogether. Perhaps use the CRTP to have this be an implicit thing from C++ code (e.g:

class MyRunnable : public Runnable<MyRunnable> { void Run() { ... } };

where we have something like:

class IRunnable
{
  void DoRun() { mRun(); }

  AddRef() { ... }
  Release() { ... }

private:
  RefCnt mRefCnt;
  void (*mRun)();
};

template<typename T>
class Runnable
{
private:
  static void RunWrap(void* self) {
    static_cast<T*>(self)->Run();
  } 
public:
  Runnable() { mRun = &RunWrap; }
};

That way we have no vtables at all, and just hold a single virtual function pointer which Does The Right Thing 
That's probably a bit more work to implement, though it also means that we (theoretically?) shouldn't need any static relocations for our Runnable vtables. We could add more pointers to dangle additional info (like cancelable, name, etc.) off of if we want to.

I imagine this would need some work from bindings code to make this work from JS, but perhaps it's doable?
To make the JS bits work, all we need is some type in xpidl that takes a JS callable and wraps it up in a runnable.  It shouldn't be too hard to creat that.
Nathan, do you have a guess of how much overhead this might save?
Flags: needinfo?(nfroyd)
In the very best case (something like eliminating QI and (virtual) AddRef/Release from all runnables), hundreds of kb of vtables.  It's a lot of work to get to that point, though.
Flags: needinfo?(nfroyd)
Whiteboard: [overhead:>100k]
Depends on: 1639632
Depends on: 1639658
Depends on: 1639837
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.