I've investigated this a bit and I think this is doable, at least for most common use case, which is: SomeConcreteFrameClass* f = do_QueryFrame(aFrame); // nsIFrame* aFrame Those can be done as a static_cast if aFrame->mClass is that of SomeConcreteFrameClass. Other cases can be optimized through a table-based approach...

I did some coarse measurements of the kind of do_QueryFrame calls we do on a few web pages. The number of calls that are eligible for the above optimization is in the 60-100% range depending on the page, e.g. https://slashdot.org is ~60% and https://wikipedia.org/wiki/The_Beatles is ~100%. Internal XUL pages that we load during startup are typically less than that, 20-40% or so. Out of the eligible calls, roughly 55% are successfully optimized. The total number of eligible calls are quite high; starting a browser with a single tab loading https://wikipedia.org/wiki/The_Beatles generates ~1 million calls. FTR, non-eligible calls are of the form: SomeQueryFrameInterface* f = do_QueryFrame(aFrame); // nsIFrame* aFrame SomeConcreteFrameClass* f = do_QueryFrame(aThing); // SomeQueryFrameInterface* aThing SomeQueryFrameInterface* f = do_QueryFrame(aThing); // SomeOtherQueryFrameInterface* aThing (I don't think we have many of the last kind though)

Comment on attachment 8906084 [details] [diff] [review] fix >Bug 1364815 - Optimize away many (virtual) calls to QueryFrame. r=dholbert > >do_QueryFrame from one frame type to another frame type >can compare mClass first, and if successful just downcast >the pointer to the target frame type. If unsuccesful, Typo: s/unsuccesful/unsuccessful/ (needs a double "s" at the end) >diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h >--- a/layout/generic/nsIFrame.h >+++ b/layout/generic/nsIFrame.h >+// Use QueryFrame fast-path for nsIFrame. >+inline do_QueryFrameHelper<nsIFrame> >+do_QueryFrame(AutoWeakFrame& s) >+{ >+ return do_QueryFrameHelper<nsIFrame>(s.GetFrame()); >+} The comment here is ambiguous, RE what "for nsIFrame" means. Initially, I read it as meaning "Use QueryFrame fast-path, if we have a nsIFrame", and I had a hard time mapping that onto the code. Please reword to avoid that ambiguity -- e.g. maybe: // Use nsIFrame's fast-path to avoid QueryFrame: (I think that's what the comment means to say, anyway.) >+// Use QueryFrame fast-path for nsIFrame. >+inline do_QueryFrameHelper<nsIFrame> >+do_QueryFrame(WeakFrame& s) Same here. >diff --git a/layout/generic/nsQueryFrame.h b/layout/generic/nsQueryFrame.h >--- a/layout/generic/nsQueryFrame.h >+++ b/layout/generic/nsQueryFrame.h >+ // Specialization for nsIFrame to nsIFrame subclasses -- if the source >+ // instance's mClass matches kFrameIID of the destination type then >+ // downcasting is safe. >+ template<class Src, class Dst> >+ struct FastQueryFrame<Src, Dst, >+ typename mozilla::EnableIf<mozilla::IsBaseOf<nsIFrame, Src>::value>::type, >+ typename mozilla::EnableIf<mozilla::IsBaseOf<nsIFrame, Dst>::value>::Type> >+ { The comment here, "for nsIFrame to nsIFrame subclasses", sounds like it's saying the source type has to be nsIFrame itself (and the destination can be a nsIFrame subclass). But the code looks like it allows both Src and Dst to be subclasses. So perhaps the comment should say "Specialization for nsIFrame subclasses to other nsIFrame subclasses", perhaps? >+ static Dst* QueryFrame(Src* aFrame) { >+ return nsQueryFrame::FrameIID(aFrame->mClass) == Dst::kFrameIID ? >+ reinterpret_cast<Dst*>(aFrame) : nullptr; It's not obvious to me why the conversions in the "==" check are valid. It looks like: * aFrame->mClass has type nsQueryFrame::ClassID * ...which we cast to type FrameIID... * ...and that seems like it might be a bogus cast, since those are two **completely different** enum types. They happen to be generated from the same header, but they use different (overlapping) pieces of that header -- one uses the ABSTRACT_ entries and the other does not. So superficially, it's not clear that we can depend on the numbers matching up. Ah, OK -- I guess this is fine because nsFrameIdList.h happens to list *all* of its ABSTRACT_FRAME_ID() entries at the end, after all of its FRAME_ID() entries. So we are 100% depending on that here, in order for this enum cast to be valid. Could you be sure that dependency is documented somewhere? (Either in nsFrameIdList.h itself (which doesn't have any documentation to suggest that the ordering is significant AFAICT), or in nsQueryFrame alongside these enum types, or here alongside your type-conversion. Or maybe in several of those places.)

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/826f9585e662 Optimize away many (virtual) calls to QueryFrame. r=dholbert

> So perhaps the comment should say "Specialization for nsIFrame subclasses to other nsIFrame subclasses" I chose "Specialization for any nsIFrame type to any nsIFrame type". To me, "nsIFrame subclasses" sounds like it implies the type nsIFrame is excluded, which it isn't. > Could you be sure that dependency is documented somewhere? I added in nsFrameIdList.h (before the first ABSTRACT_FRAME_ID): // The following ABSTRACT_FRAME_IDs needs to come after the above // FRAME_IDs, because we have two separate enums, one that includes // only FRAME_IDs and another which includes both and we depend on // FRAME_IDs to have the same number in both. // See ClassID (the former) and FrameIID in nsQueryFrame.h. Fwiw, we already have an assertion for this: http://searchfox.org/mozilla-central/rev/70cfd6ceecacbe779456654b596bbee4f2b8890b/layout/generic/nsFrame.cpp#610

I think this might be worth taking as a performance improvement for v57. The regression risk seems fairly low.

Comment on attachment 8906084 [details] [diff] [review] fix Review of attachment 8906084 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsQueryFrame.h @@ +124,5 @@ > + // instance's mClass matches kFrameIID of the destination type then > + // downcasting is safe. > + template<class Src, class Dst> > + struct FastQueryFrame<Src, Dst, > + typename mozilla::EnableIf<mozilla::IsBaseOf<nsIFrame, Src>::value>::type, Drive-by comment, because I happened to see this commit go past: This should be ::Type rather than ::type, or it will never match.

Good catch, thanks! I'll land a typo fix...

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0f7fc025ee part 2 - Follow-up to fix a typo. r=me

https://hg.mozilla.org/mozilla-central/rev/826f9585e662 https://hg.mozilla.org/mozilla-central/rev/1f0f7fc025ee