Open Bug 705541 Opened 13 years ago Updated 2 years ago

Make EventTarget cycle collectable and implement non-virtual versions of addref/release

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
All
Type:
defect

Tracking

()

People

(Reporter: smaug, Unassigned)

References

Details

If all the event targets would inherit nsIDOMEventTarget first, we could
push nsCycleCollectingAutoRefCnt mRefCnt; to there and add non-virtual
versions of AddRef/Release and perhaps implement FastRef<class>.
I _think_ using non-virtual addref/release could improve for example event handling speed.
Depends on: 677026
Is the virtual call overhead noticeable compared to the cost of doing cycle-collected AddRef/Release?
In general virtual calls are slow, but need to profile.
and even if addref/release itself wouldn't be faster, having mRefCnt available in nsINode
methods would allow some optimizations to CC handling, I hope.
Ehsan, this is more or less what I was thinking of.

I think it would be safe to at least make NS_DECL_ISUPPORTS and NS_DECL_CYCLE_COLLECTING_ISUPPORTS declare AddRef and Release "final". We probably don't override those implementations anywhere. That alone should remove some virtual addrefs/releases.

Then in this bug, we could put NS_DECL_CYCLE_COLLECTING_ISUPPORTS in nsIDOMEventTarget, and use NS_IMPL_CYCLE_COLLECTING_ADDREF and NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY for nsIDOMEventTarget. We need to do something to make sure nsNodeUtils::LastRelease gets called for nodes --- maybe just declare a virtual LastRelease method in nsIDOMEventTarget.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Is the virtual call overhead noticeable compared to the cost of doing
> cycle-collected AddRef/Release?

It's hard to measure. For example, frequent virtual AddRef/Release calls could pollute the branch target buffer and slow down other virtual calls.

Not only could we reduce the cost of virtual calls, nonvirtual calls also require less generated code, and we'd have fewer implementations of the AddRef/Release methods generated. Anyway, doing this would reduce source code complexity (by reducing the number of classes needing NS_DECL_CYCLE_COLLECTING_ISUPPORTS) so even if the performance win turns out to be small, it's still a good thing to do.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Ehsan, this is more or less what I was thinking of.
> 
> I think it would be safe to at least make NS_DECL_ISUPPORTS and
> NS_DECL_CYCLE_COLLECTING_ISUPPORTS declare AddRef and Release "final". We
> probably don't override those implementations anywhere. That alone should
> remove some virtual addrefs/releases.

I spent a few minutes to get something like this working.  A WIP patch is here: <http://pastebin.mozilla.org/1668112>  Basically the remaining work for that would be to work through the compiler errors and fix them all up to use the _OVERRIDABLE macros.  Do you think it's valuable for me to pursue this?

> Then in this bug, we could put NS_DECL_CYCLE_COLLECTING_ISUPPORTS in
> nsIDOMEventTarget, and use NS_IMPL_CYCLE_COLLECTING_ADDREF and
> NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY for nsIDOMEventTarget. We need
> to do something to make sure nsNodeUtils::LastRelease gets called for nodes
> --- maybe just declare a virtual LastRelease method in nsIDOMEventTarget.

That would be a problem if we have classes inheriting from multiple interfaces including nsIDOMEventTarget.  This will compile fine with virtual inheritance (see http://pastebin.mozilla.org/1668127) but I'm not sure if we can use that with XPCOM.
(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > I think it would be safe to at least make NS_DECL_ISUPPORTS and
> > NS_DECL_CYCLE_COLLECTING_ISUPPORTS declare AddRef and Release "final". We
> > probably don't override those implementations anywhere. That alone should
> > remove some virtual addrefs/releases.
> 
> I spent a few minutes to get something like this working.  A WIP patch is
> here: <http://pastebin.mozilla.org/1668112>  Basically the remaining work
> for that would be to work through the compiler errors and fix them all up to
> use the _OVERRIDABLE macros.  Do you think it's valuable for me to pursue
> this?

I don't know. It's probably worthwhile for *someone* to pursue this.

> > Then in this bug, we could put NS_DECL_CYCLE_COLLECTING_ISUPPORTS in
> > nsIDOMEventTarget, and use NS_IMPL_CYCLE_COLLECTING_ADDREF and
> > NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY for nsIDOMEventTarget. We need
> > to do something to make sure nsNodeUtils::LastRelease gets called for nodes
> > --- maybe just declare a virtual LastRelease method in nsIDOMEventTarget.
> 
> That would be a problem if we have classes inheriting from multiple
> interfaces including nsIDOMEventTarget.  This will compile fine with virtual
> inheritance (see http://pastebin.mozilla.org/1668127) but I'm not sure if we
> can use that with XPCOM.

virtual inheritance is often really bad for performance, we shouldn't use it even if we could.

I sort of hope nothing is implementing nsIDOMEventTarget twice, but who knows...
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> > > Then in this bug, we could put NS_DECL_CYCLE_COLLECTING_ISUPPORTS in
> > > nsIDOMEventTarget, and use NS_IMPL_CYCLE_COLLECTING_ADDREF and
> > > NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY for nsIDOMEventTarget. We need
> > > to do something to make sure nsNodeUtils::LastRelease gets called for nodes
> > > --- maybe just declare a virtual LastRelease method in nsIDOMEventTarget.
> > 
> > That would be a problem if we have classes inheriting from multiple
> > interfaces including nsIDOMEventTarget.  This will compile fine with virtual
> > inheritance (see http://pastebin.mozilla.org/1668127) but I'm not sure if we
> > can use that with XPCOM.
> 
> virtual inheritance is often really bad for performance, we shouldn't use it
> even if we could.
> 
> I sort of hope nothing is implementing nsIDOMEventTarget twice, but who
> knows...

It's not a question of someone implementing nsIDOMEventTarget twice, I was talking about implementing nsISupports twice (i.e., inheriting from nsIDOMEventTarget and any other interface.)
That should be OK if we use "using" in derived classes to ensure that the implementation of all virtual AddRef/Release methods is the one from nsIDOMEventTarget.
FWIW, there's definitely classes implementing nsIDOMEventTarget twice. IIRC GlobalWindow and/or XMLHttpRequest does this. The reason is that we inherit it once internally in a EventTargetHelper and then the leaf interface inherits it as well.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> That should be OK if we use "using" in derived classes to ensure that the
> implementation of all virtual AddRef/Release methods is the one from
> nsIDOMEventTarget.

AFAIK "using" doesn't eliminate the double implementation of virtual functions.  It just tells the compiler to bring the base class member names into the static scope of the derived class.  Really, I don't think there is any way to do what we want in C++.  :(
Still likely worth doing, albeit with EventTarget.  At this point all DOM classes _do_ in fact inherit EventTarget first.

Might be simpler to wait for bug 1429903 to be fixed to simplify things a bit.
Depends on: 1429903
nsIDOMEventTarget is gone, but we may still want to do this with EventTarget.
Summary: Make nsIDOMEventTarget cycle collectable and implement non-virtual versions of addref/release → Make EventTarget cycle collectable and implement non-virtual versions of addref/release
Priority: -- → P3
Component: DOM → DOM: Core & HTML
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.