537547 - Cycle collector uses virtual methods

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

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

Attachment: WIP

Based on Shark profiles GraphWalker::DoWalk is a hot method, yet it calls
virtual ShouldVisitNode and VisitNode methods. We should and could avoid virtual methods.

One option is to just implement ::DoWalk in each class (using a macro).
There are actually just 2 such classes which are compiled normally.
Implementing it twice allows (hopefully) compiler to inline ShouldVisitNode
and VisitNode.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It wouldn't surprise me if MSVC already inlines both implementations into separate instantiations of DoWalk (though perhaps it only does one, guarded with a vtable pointer check, and lets the other use virtual calls).

Comment 2

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

Comment on attachment 419793 [details] [diff] [review]
WIP

Well at least OSX gcc doesn't seem to inline anything here.

Is this too ugly? We really need to start optimizing CC.

After the patch things like nsQueue::Push shows up in the profiles. I think it could be made a bit faster by using faster mod, since as far as I see it is guaranteed that divider is n^2. Or something else than nsQueue could be used in CC. ...but that is a different bug.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: template-based approach

I guess I think that templates are probably less ugly than macros here; does this provide equivalent performance?

Comment 4

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

Comment on attachment 419825 [details] [diff] [review]
template-based approach


>+    // copy-constructing the visitor should be cleaner than useng a reference
useng

And I don't know why copy-ctor is cleaner than a reference, but if it is, then ok.

Seems to provide similar performance as the macro based patch.
And this is easier to read.

Comment 5

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

David, are you going to land your patch?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just pushed it to try with some other patches; I'll hopefully land it in the next few days (unlikely before Monday, though).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/16e05af6610d