Mark nsGlobalWindowInner/Outer final

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Depends on 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

In Bug 1332680 we got a list of classes and methods we could mark 'final', as suggested by an LTO build of gcc. One of the items on that list was nsGlobalWindowInner and nsGlobalWindowOuter, with quite a lot of virtual calls:

> dom/base/nsGlobalWindowInner.h:206:7: warning: Declaring type 'struct nsGlobalWindowInner' final would enable devirtualization of 483 calls
> dom/base/nsGlobalWindowOuter.h:164:7: warning: Declaring type 'struct nsGlobalWindowOuter' final would enable devirtualization of 143 calls

After trying it out, we saw a modest improvement to a single Talos test (displaylist mutate got 4-8.5% better). That's not the interesting thing though.

Build times were reduced by half across the board. They're a bit variable of course, but 30-70% improvements are shown by Talos.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=48615cf2083ae24a28f7981cb2ec2de42abc3ae8&framework=2&showOnlyImportant=1&selectedTimeRange=172800
I made a mistake: build times for Windows only go down 10-15%.
I'm still not sure if this is real, but Bug 1332680 has a long list of other targets we could slap final on and potentially see build (or runtime) improvements as well. (Some of them are in third party code, but some are not.)
Attachment #8956178 - Flags: review?(continuation) → review?(nika)
Cool! Nika would be a better reviewer for this. I did notice that your change messes up the alignment of the super classes here. I don't know if it is better to fix it, or to change it to a style that is less easily perturbed...
Comment on attachment 8956178 [details]
Bug 1443252 Make nsGlobalWindowInner/Outer final to reduce build times

https://reviewboard.mozilla.org/r/225098/#review231068

\o/ - I forgot we could do this now that I got rid of nsGlobalChromeWindow~!

Thanks! (The build time improvements look great)

::: commit-message-1b2e6:15
(Diff revision 2)
> +For Linux and OSX (and some flavors of Android) build times were reduced
> +by half across the board. They're a bit variable of course, but 30-70%
> +improvements are shown by Talos. Windows and other flavors of Android
> +show 10-15% improvements.

I'm now curious if we could get build time improvements by devirtualizing all of the virtual methods on nsPIDOMWindow{Inner,Outer} (as the underlying type is always nsGlobalWindow{Inner,Outer})... Perhaps in a follow up.

::: dom/base/nsGlobalWindowInner.h:207
(Diff revision 2)
>  // also contain all inner window objects that are still in memory (and in
>  // reality all inner window object's lists also contain its outer and all other
>  // inner windows belonging to the same outer window, but that's an unimportant
>  // side effect of inheriting PRCList).
>  
> -class nsGlobalWindowInner : public mozilla::dom::EventTarget,
> +class nsGlobalWindowInner final : public mozilla::dom::EventTarget,

We're gonna be disturbing the columns here anyway, so let's move the base classes onto a different line & clean up the style:

class nsGlobalWindowInner final
  : public mozilla::dom::EventTarget
  , public nsPIDOMWindowInner
  , private nsIDOMChromeWindow
  , ...

::: dom/base/nsGlobalWindowOuter.h:165
(Diff revision 2)
>  // reality all inner window object's lists also contain its outer and all other
>  // inner windows belonging to the same outer window, but that's an unimportant
>  // side effect of inheriting PRCList).
>  
> -class nsGlobalWindowOuter : public mozilla::dom::EventTarget,
> +class nsGlobalWindowOuter final : public mozilla::dom::EventTarget,
>                              public nsPIDOMWindowOuter,

see nsGlobalWindowInner & do the same thing here :-)
Attachment #8956178 - Flags: review?(nika) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6e5bda8eb73
Make nsGlobalWindowInner/Outer final to reduce build times. r=mystor
Keywords: checkin-needed
Compile time speedup surprises me as well. Is that compile time of whole firefox or some specific file?
Devirtualization may enable inlining and subsequent dead code elimination, but it is odd it makes such large difference.

As mentioned in the other PR, if you want to look for perofrmance improvements, perhaps it is better to collect devirtualization warnings with profile feedback. You will then get info on how many given call was executed. It is however good to add final as a coding style and also for security.
https://hg.mozilla.org/mozilla-central/rev/c6e5bda8eb73
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.