1274786 - Could NativeGlobal do less QIing?

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

It would be nice if NativeGlobal could avoid do QIing/Addref/Release, at least in 
case of nsIGlobalObject. Could we do a static cast from native to nsIGlobalObject*, or what would it take to be able to do so?
The QI/Addref/Release show up a bit in certain performance profiles.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Some part of the QIing is because of bug 1137739.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This shows up a tiny bit in Speedometer too.
gabor, by any chance, do you have any spare cycles to look at this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Could we do a static cast from native to nsIGlobalObject*, or what would it take to be able to do so?

So what we have right now is that the JSObject stores a pointer to the underlying C++ object's NativeType*, where NativeType is the native type for the binding.  For Window, for example, it's an nsGlobalWindow*.

Due to the way bindings work, this needs to be the same pointer as the EventTarget*, which is why nsGlobalWindow inherits from EventTarget first.

So once we've done the UnwrapDOMObjectToISupports call we have effectively 

  static_cast<nsISupports*>(static_cast<EventTarget*>(static_cast<nsGlobalWindow*>(ptr)))

where ptr is the void* stored in the JSObject.

If we want to cast to nsIGlobalObject*, we need to know its offset from the pointer above.  That means either casting through nsGlobalWindow* (and letting the compiler compute offset) or computing the offset in some other way.

We _could_ probably compute the offset at compile time something like:

    reinterpret_cast<char*>(static_cast<nsIGlobalObject*>(reinterpret_cast<nsGlobalWindow*>(0x1000))) -
    reinterpret_cast<char*>(0x1000)

or so and store it in the DOMJSClass that we codegen.  Then we could replace the QI with some nasty casting and pointer arithmetic.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Olli Pettay [:smaug] from comment #2)
> This shows up a tiny bit in Speedometer too.
> gabor, by any chance, do you have any spare cycles to look at this?

Not sure... it really depends on how urgent it is. If it's not too urgent I'm sure I can find some time for it sooner or later. If it's urgent, then I'm less sure.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3)
> or so and store it in the DOMJSClass that we codegen.  Then we could replace
> the QI with some nasty casting and pointer arithmetic.

Or we could just do the QI once during runtime, and then calculate and store the offset in a static variable and use that later? (all this either in a macro or in a static inline function) Then we don't have to worry that someone changes the class hierarchy, although in this case that's not very likely. But maybe it would be easier to reuse the same trick at other places if we really need to.

Comment 5

[Boris Zbarsky [:bzbarsky]]

My proposal in comment 3 does not depend on the class hierarchy changing, apart from nsGlobalWindow inheriting from nsIGlobalObject.  And if it stops doing that, it will fail to compile, alerting us to the problem.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

If we still want to do this, it should be possible by adding a method generated by WebIDL for all interfaces with a Global extended attribute on them. This would cover all of the ...GlobalScope interfaces for actors and Window. The code could look something like:

nsIGlobalObject* GetWebIDLNativeGlobal(JSObject* aObject) {
  const DOMJSClass* clasp = GetDOMClass(aObject);
  if (!clasp || clasp->ToJSClass()->isProxy()) {
    return nullptr;
  }

  nsIGlobalObject* global = nullptr;
  // This is for each of the `[Global]`-annotated interfaces, and would probably be auto-generated?
  if (NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(Window, aObject, global)) &&
      NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(AudioWorkletGlobalScope, aObject, global)) &&
      NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(DedicatedWorkerGlobalScope, aObject, global)) &&
      NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(PaintWorkletGlobalScope, aObject, global)) &&
      NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(ServiceWorkerGlobalScope, aObject, global)) &&
      NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(SharedWorkerGlobalScope, aObject, global)) &&
      NS_FAILED(UNWRAP_NON_WRAPPER_OBJECT(WorkerDebuggerGlobalScope, aObject, global))) {
    return nullptr;
  }
  return global; 
}

We could then have the caller perform this "optimized" check for DOM objects before falling back to the existing nsISupports/wrappercache-based private setup. I think this would remove one QI call for all non-xpcom global lookups.

In the specific case of the remaining xpcom globals, I don't know what can easily be done there :shrug:

Comment 7

[Chris Peterson [:cpeterson]]

Olli hasn't seen this function in recent perf profiles, so this bug is not a high priority.

P3/S3