Devirtualize do_QueryFrame()

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57- fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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...
(Assignee)

Comment 1

2 years ago
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.)
Attachment #8906084 - Flags: review?(dholbert) → review+

Comment 5

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

Comment 6

2 years ago
> 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
(Assignee)

Comment 7

2 years ago
I think this might be worth taking as a performance improvement for v57.
The regression risk seems fairly low.
tracking-firefox57: --- → ?
Whiteboard: [qf]
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.
(Assignee)

Comment 9

2 years ago
Good catch, thanks!  I'll land a typo fix...
https://hg.mozilla.org/mozilla-central/rev/826f9585e662
https://hg.mozilla.org/mozilla-central/rev/1f0f7fc025ee
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
tracking-firefox57: ? → -

Updated

2 years ago
Whiteboard: [qf] → [qf:p1]
You need to log in before you can comment on or make changes to this bug.