1901610 - Investigate the performance when implementing nsIFrame::IsScrollContainerOrSubclass() with do_QueryFrame

Status: RESOLVED WONTFIX

(Core :: Layout: Scrolling and Overflow, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

This is a follow-up to Bug 1824877 Part 6. In that patch, I kept nsIFrame::IsScrollContainerOrSubclass() to use layout type checking to avoid changing the performance behavior.

In theory, using do_QueryFrame to downcast nsIFrame* to ScrollContainerFrame* is fast, because it goes to the fast path in QueryFrame, which just checks against nullptr and compares two FrameIID integers. If the performance is on par with the layout type checking, we can just use do_QueryFrame to implement IsScrollContainerOrSubclass() just like other Is*Subclass() methods nearby.

Also, callers who need the ScrollContainerFrame* pointers can also do do_QueryFrame directly without needing to use IsScrollContainerOrSubclass() followed by a static_cast<ScrollContainerFrame*>().

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1901610 - Change IsScrollContainerOrSubclass() to use do_QueryFrame.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Even though my try runs do not show significant performance regression, it is still better to investigate whether the patch impacts the performance in a granular way.

I grabbed a profile while running speedometer 3 with my patch https://share.firefox.dev/3KDlvDo, and filtered for MaybeCreateDisplayPortInFirstScrollFrameEncountered(). I noticed nsSplittableFrame::QueryFrame(), appearing in the invert call stack. That means, my understanding of do_QueryFrame in comment 0 was wrong—we are still hitting the slow path sometimes in this statement:

ScrollContainerFrame* sf = do_QueryFrame(aFrame);

where aFrame is declared as nsIFrame*, but the actual value is a ViewportFrame*.

The lesson learned is that in performance-critical code, checking layout frame types such as IsScrollContainerFrame() and and usingstatic_cast<ScrollContainerFrame*>() are recommended because they are faster than do_QueryFrame. However, regarding type safety and ergonomic, I think do_QueryFrame is safer and perhaps easier to use.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Close this bug per comment 2.