1422529 - Some trivial element methods are hidden behind virtual calls and could be devirtualized.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Noticed while looking at bug 1422528.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1422529: Make some trivial element methods final and inline.

Give the compiler the chance to devirtualize / inline a bit more.

Review commit: https://reviewboard.mozilla.org/r/204876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204876/

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 8933942 [details]
Bug 1422529: Make some trivial element methods final and inline.

https://reviewboard.mozilla.org/r/204876/#review210404

::: dom/base/Element.h:786
(Diff revision 1)
>                                    AttrValuesArray* aValues,
>                                    nsCaseTreatment aCaseSensitive) const override;
>    virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsAtom* aAttribute,
>                               bool aNotify) override;
> -  virtual const nsAttrName* GetAttrNameAt(uint32_t aIndex) const override;
> -  virtual BorrowedAttrInfo GetAttrInfoAt(uint32_t aIndex) const override;
> +
> +  virtual const nsAttrName* GetAttrNameAt(uint32_t aIndex) const final

`override final`?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8933942 [details]
Bug 1422529: Make some trivial element methods final and inline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204876/diff/1-2/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8933942 [details]
Bug 1422529: Make some trivial element methods final and inline.

https://reviewboard.mozilla.org/r/204876/#review210404

> `override final`?

Huh, sure thing, thanks! :)

Actually one of those didn't even need to be virtual...

I went with `final override` because there was another use of it in the file, but we're inconsistent in the codebase...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933942 [details]
Bug 1422529: Make some trivial element methods final and inline.

https://reviewboard.mozilla.org/r/204876/#review210682

::: dom/base/Element.h:787
(Diff revision 2)
>                                    AttrValuesArray* aValues,
>                                    nsCaseTreatment aCaseSensitive) const override;
>    virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsAtom* aAttribute,
>                               bool aNotify) override;
> -  virtual const nsAttrName* GetAttrNameAt(uint32_t aIndex) const override;
> -  virtual BorrowedAttrInfo GetAttrInfoAt(uint32_t aIndex) const override;
> +
> +  virtual const nsAttrName* GetAttrNameAt(uint32_t aIndex) const final override

This is OK, but we could do a followup to move GetAttrNameAt to Element and devirtualize it altogether.

::: dom/base/Element.h:792
(Diff revision 2)
> -  virtual BorrowedAttrInfo GetAttrInfoAt(uint32_t aIndex) const override;
> -  virtual uint32_t GetAttrCount() const override;
> +  virtual const nsAttrName* GetAttrNameAt(uint32_t aIndex) const final override
> +  {
> +    return mAttrsAndChildren.GetSafeAttrNameAt(aIndex);
> +  }
> +
> +  virtual BorrowedAttrInfo GetAttrInfoAt(uint32_t aIndex) const final override

Again, could be moved to Element.  Doing that cleanly involves changing some function signatures in the serializers and XBL, so followup.

::: dom/base/Element.h:801
(Diff revision 2)
> +    }
> +
> +    return mAttrsAndChildren.AttrInfoAt(aIndex);
> +  }
> +
> +  virtual uint32_t GetAttrCount() const final override

Again might be worth moving to Element, but has more callers than the other two...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> ::: dom/base/Element.h:801
> (Diff revision 2)
> > +    }
> > +
> > +    return mAttrsAndChildren.AttrInfoAt(aIndex);
> > +  }
> > +
> > +  virtual uint32_t GetAttrCount() const final override
> 
> Again might be worth moving to Element, but has more callers than the other
> two...

It may even be worth to convert the nsIContent version in IsElement() ? AsElement()->GetAttrCount() : 0 or such.

Anyway, will file followups for those.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22da077ec8f0
Make some trivial element methods final and inline. r=bz

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/22da077ec8f0