Closed
Bug 1443252
Opened 7 years ago
Closed 7 years ago
Mark nsGlobalWindowInner/Outer final
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
I made a mistake: build times for Windows only go down 10-15%.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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.)
Updated•7 years ago
|
Attachment #8956178 -
Flags: review?(continuation) → review?(nika)
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•