Closed Bug 1360241 Opened 7 years ago Closed 7 years ago

Devirtualize nsIFrame::GetType.

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file, 3 obsolete files)

It's the function that annoys me the most so far. I think we should consider doing this. We call it in all sort of hot loops and I think we could use a byte of the three left contiguously to mWritingMode to do this.

I have a few patches I did yesterday that de-virtualize it in a hacky way, that I want to push for feedback.

If this makes sense to you all, next steps would be:

 * Rewrite the calls that look like |GetType() == nsGkAtoms::fooFrame| to use |IsFooFrame()|.
 * Replace all the |nsGkAtoms::fooFrame| for |FrameType::Foo|.
 * Remove the atoms and the GetType_() function, which are only in order to prove correctnes.

Do you think this is worth doing? I thought of adding just a single bit to check for placeholder frames in the context of bug 1356133, but then saw that we have all sorts of checks in hot places, so I'd rather use 7 bits and make all the checks fast.

I'm going to post just a squashed patch, if you want to review more carefully the approach and the process, I have the un-squashed patches too.

I pushed to Talos here: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=e6a3e0639afdcb63289f344af5aaa343811b137c
And to try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a2c7cad449d6452ce5043115c2deca4a2e44e8
Seems pretty reasonable to me.  This has always annoyed me too.
Comment on attachment 8862483 [details]
Bug 1360241: Devirtualize nsIFrame::GetType.

Let me know if I've missed anyone else :)
Attachment #8862483 - Flags: feedback?(tnikkel)
Attachment #8862483 - Flags: feedback?(mats)
Attachment #8862483 - Flags: feedback?(dholbert)
Attachment #8862483 - Flags: feedback?(dbaron)
Attachment #8862483 - Flags: feedback?(cam)
Attachment #8862483 - Flags: feedback?(bzbarsky)
Attachment #8862483 - Flags: feedback?(xidorn+moz)
Comment on attachment 8862483 [details]
Bug 1360241: Devirtualize nsIFrame::GetType.

This sounds like a good idea.

I would probably prefer we remove the frame atoms, and make GetType() solely return FrameType, so that:
1. we don't add another list to touch when we add new frame,
2. we may not need to add frames to FrameTypeList.h if we don't check them at all, and
3. one can never misuse this function if the frame they want to check is not listed in FrameTypeList.h

That may need some more work, though...
Attachment #8862483 - Flags: feedback?(xidorn+moz) → feedback+
Comment on attachment 8862483 [details]
Bug 1360241: Devirtualize nsIFrame::GetType.

Happy to let others handle this.  ;)
Attachment #8862483 - Flags: feedback?(bzbarsky)
I hope we can setup some approach to ensure the list keeps sync to frame ctors... but that feels really hard...
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #4)
> Comment on attachment 8862483 [details]
> Bug 1360241: WIP: Devirtualize nsIFrame::GetType
> 
> This sounds like a good idea.
> 
> I would probably prefer we remove the frame atoms, and make GetType() solely
> return FrameType, so that:
> 1. we don't add another list to touch when we add new frame,
> 2. we may not need to add frames to FrameTypeList.h if we don't check them
> at all, and
> 3. one can never misuse this function if the frame they want to check is not
> listed in FrameTypeList.h
> 
> That may need some more work, though...

Right, fwiw I plan to do this before landing if everyone is on board. I only kept them in order to show something for feedback in case anyone is against it before spending more time on it.

I think this shouldn't be too hard using clang AST matchers[1], or a python script that reads FrameTypeList.h.

[1]: http://clang.llvm.org/docs/LibASTMatchers.html
Also, can we somehow just reuse nsQueryFrame::FrameIID?
Hmmm, probably no, that is a plain enum so int32_t. But can we merge the list with nsFrameIdList sonehow?
That sounds more complex, because there are a lot of frames that don't share ID, but share type, like nsMathMLmtableWrapperFrame and nsTableWrapperFrame (and a lot of other frames).

Perhaps we can pass the frame ID down through the constructor instead, and then switch to get the frame type, instead of passing the frame type directly... but we still need the two lists, it's just the mapping of frame id to frame type what changes (instead of being specified in the constructor, it's specified on the frame id list).

I guess I could do that, but doesn't sound like a strictly better solution to me.
Right, I just realized that they exist for different usage. We should probably keep them separate.
Comment on attachment 8862483 [details]
Bug 1360241: Devirtualize nsIFrame::GetType.

LGTM.  I suspect some uses of these values could be converted to IsFrameOfType /
do_QueryFrame / dedicated virtual methods.  But, using a byte for this seems
justified for now.
Attachment #8862483 - Flags: feedback?(mats) → feedback+
Attachment #8862483 - Flags: feedback?(dholbert) → feedback+
For example, the only use of nsGkAtoms::svgMaskFrame/svgClipPathFrame/svgMarkerFrame
seems to be in calls to nsSVGRenderingObserver::GetReferencedFrame:
http://searchfox.org/mozilla-central/search?q=nsGkAtoms%3A%3AsvgMaskFrame&path=
http://searchfox.org/mozilla-central/search?q=nsGkAtoms%3A%3AsvgClipPathFrame&path=
http://searchfox.org/mozilla-central/search?q=nsGkAtoms%3A%3AsvgMarkerFrame&path=

which nsSVGRenderingObserver::GetReferencedFrame checks against the referenced frame:
http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/layout/svg/nsSVGEffects.cpp#89

Maybe we could pass a nsQueryFrame::FrameIID instead and check with QueryFrame?
It's a virtual call, but none of these seems perf sensitive to me.
(In reply to Mats Palmgren (:mats) from comment #12)
> Comment on attachment 8862483 [details]
> Bug 1360241: WIP: Devirtualize nsIFrame::GetType
> 
> LGTM.  I suspect some uses of these values could be converted to
> IsFrameOfType /
> do_QueryFrame / dedicated virtual methods.  But, using a byte for this seems
> justified for now.

I agree. We should remove a lot / most of them. Removing frame types is slightly tricky though, because don't want to check only the uses, but also that the uses of the frame type of the superclass are fine.

We have 92 frame types, in order to save 1 more bit we need to reduce that number to 64. Figuring out which ones can we use a virtual call / FrameIID for vs which ones do not is quite an amount of analysis and work, so I think I want to get this done first, and then considering if it's worth the effort.
Attachment #8862483 - Flags: feedback?(tnikkel) → feedback+
Comment on attachment 8862483 [details]
Bug 1360241: Devirtualize nsIFrame::GetType.

This looks fine to me.

As a followup, I wonder if it's worth looking at devirtualizing IsFrameOfType, too.  Most of those functions just return constant values, so we could have some static data somewhere with the flags that each frame type sets, although some also check some state of the frame, like http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/layout/generic/nsFirstLetterFrame.h#38-44.
Attachment #8862483 - Flags: feedback?(cam) → feedback+
Ok, since seems that everyone so far is fine with this change, I'm going to start posting patches for review. I plan to squash all the patches before landing.

The first patch is the already-posted one, but rebased, so probably just needs to be stamped. The second one is automated with a python script, so should be easy to audit. The third one is a lot of manual fixups on top of that (my script was really naive, so that's what I get :P), but should be really easy to verify it's right (also, here's a try run[1]).

After that, the last patch is removing unused stuff, which should also be trivially reviewable.

I'm going to ask Cam for review because he was going to be on-and-off during the weekend, but if somebody else wants to poke at it instead, please feel free to.

One doubt is how to name the frame type that is used where null was returned before. I've used FrameType::None, but perhaps it should be FrameType::Generic or FrameType::Unknown. That being said, renaming it is trivial, so I don't think it's worth blocking landing on that.

Also, after this a lot of code, specially in nsCSSFrameConstructor and ReflowInput can be simplified. Also we can get rid of stuff like GetFrameName quite easily.

For the curious, the total outcome of this patch is:

    243 files changed, 1599 insertions(+), 2428 deletions(-)

Which is 829 lines of code less :)

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2d6a30a7313d677c06c86cf7daa777b28fd3662
Comment on attachment 8862483 [details]
Bug 1360241: Devirtualize nsIFrame::GetType.

https://reviewboard.mozilla.org/r/134400/#review137930

I didn't check the new FrameTypeList.h file carefully, but given the assertions you have in there I assume you did a try run to check that the types and atoms matched up.

::: layout/forms/nsHTMLButtonControlFrame.h:21
(Diff revision 2)
>  
>  class nsHTMLButtonControlFrame : public nsContainerFrame,
> -                                 public nsIFormControlFrame 
> +                                 public nsIFormControlFrame
>  {
>  public:
> -  explicit nsHTMLButtonControlFrame(nsStyleContext* aContext);
> +  nsHTMLButtonControlFrame(nsStyleContext* aContext, FrameType aType);

Make this constructor protected?

::: layout/generic/ViewportFrame.h:36
(Diff revision 2)
> +  ViewportFrame(nsStyleContext* aContext, FrameType aType)
> +    : nsContainerFrame(aContext, aType)
>      , mView(nullptr)
>    {}

Make this protected too?

::: layout/generic/nsIFrame.h:168
(Diff revision 2)
>  //  and leads to assertions)
>  #define INFINITE_ISIZE_COORD nscoord(NS_MAXSIZE - (1000000*60))
>  
>  //----------------------------------------------------------------------
>  
> +enum class FrameType : uint8_t {

As a top-level type, this probably should be inside a |namespace mozilla|.

::: layout/xul/nsBoxFrame.h:146
(Diff revision 2)
> +  nsBoxFrame(nsStyleContext* aContext,
> +             FrameType aType,
> +             bool aIsRoot = false,
> +             nsBoxLayout* aLayoutManager = nullptr);

And this one?
Attachment #8862483 - Flags: review?(cam) → review+
Comment on attachment 8863149 [details]
Bug 1360241: Automated replacement to use Type() + FrameType.

https://reviewboard.mozilla.org/r/134974/#review137936

rs=me me based on the script and a quick scan through the patch.

::: commit-message-1caa0:21
(Diff revision 1)
> +    if atom != "nullptr":
> +      assert atom.endswith("Frame")
> +      atom_to_frame_type[atom] = frame_type
> +
> +ASSIGN_REGEX = re.compile(r"nsIAtom\* ([a-zA-Z_]+) = ([a-zA-Z_]*-?>?GetType\(\));")
> +EQ_REGEX = re.compile(r"GetType\(\) == nsGkAtoms::([a-zA-Z]+)Frame")

It might worth doing this for != as well, since there are a number of places we currently do |frame->GetType() != nsGkAtoms::someFrame| that could become |!frame->IsSomeFrame()|.

::: commit-message-1caa0:25
(Diff revision 1)
> +ASSIGN_REGEX = re.compile(r"nsIAtom\* ([a-zA-Z_]+) = ([a-zA-Z_]*-?>?GetType\(\));")
> +EQ_REGEX = re.compile(r"GetType\(\) == nsGkAtoms::([a-zA-Z]+)Frame")
> +FTYPE_REGEX = re.compile(r"nsGkAtoms::([a-zA-Z]+)Frame")
> +
> +def replace_assignment(matches, in_layout, in_moz_ns):
> +  ty = "FrameType" if in_moz_ns else "mozilla::FrameType"

I'm a bit confused why it needs to be referred to as mozilla::FrameType and not just FrameType, since the type is declared at the top level in the previous patch...
Attachment #8863149 - Flags: review?(cam) → review+
Comment on attachment 8863150 [details]
Bug 1360241: Lots of manual fixups.

https://reviewboard.mozilla.org/r/134976/#review137940

::: layout/base/nsCSSFrameConstructor.h:617
(Diff revision 1)
>    /* Get the parent type that aParentFrame has. */
>    static ParentType GetParentType(nsIFrame* aParentFrame) {
> -    return GetParentType(aParentFrame->GetType());
> +    return GetParentType(aParentFrame->Type());
>    }
>  
>    /* Get the parent type for the given nsIFrame type atom */

Adjust the comment here.

::: layout/base/nsCSSFrameConstructor.cpp:351
(Diff revision 1)
>  // Returns true IFF the given nsIFrame is a nsFlexContainerFrame and
>  // represents a -webkit-{inline-}box container. The frame's GetType() result is
>  // passed as a separate argument, since in most cases, the caller will have
>  // looked it up already, and we'd rather not make redundant calls to the
>  // virtual GetType() method.

This too.

::: layout/base/nsLayoutUtils.h:38
(Diff revision 1)
>  #include "nsStyleConsts.h"
>  #include "SVGImageContext.h"
>  #include <limits>
>  #include <algorithm>
>  
> +enum class FrameType : uint8_t;

(If you move it to the mozilla namespace, this forward declaration should change too.)
Attachment #8863150 - Flags: review?(cam) → review+
Comment on attachment 8863149 [details]
Bug 1360241: Automated replacement to use Type() + FrameType.

https://reviewboard.mozilla.org/r/134974/#review137936

> It might worth doing this for != as well, since there are a number of places we currently do |frame->GetType() != nsGkAtoms::someFrame| that could become |!frame->IsSomeFrame()|.

Ah, you did this manually in the next patch.
Comment on attachment 8863151 [details]
Bug 1360241: Remove unused atoms, and simplify FrameTypeList.h.

https://reviewboard.mozilla.org/r/134978/#review137944
Attachment #8863151 - Flags: review?(cam) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> One doubt is how to name the frame type that is used where null was returned
> before. I've used FrameType::None, but perhaps it should be
> FrameType::Generic or FrameType::Unknown. That being said, renaming it is
> trivial, so I don't think it's worth blocking landing on that.

FrameType::None's probably fine.


Thanks for doing this refactoring!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89d2d1865c40989b86cfc505f4e06c23c8d36194

Needed to change two calls to nsIFrame::GetType in widget code that I had initially missed, but with those try looks green.

I ended up moving it to the mozilla namespace and addressing all those review comments, thanks for the review Cameron!
Attachment #8863149 - Attachment is obsolete: true
Attachment #8863150 - Attachment is obsolete: true
Attachment #8863151 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/662df8ef329f
Devirtualize nsIFrame::GetType. r=heycam
https://hg.mozilla.org/mozilla-central/rev/662df8ef329f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.