Closed Bug 1047696 Opened 6 years ago Closed 6 years ago

mark a bunch of stuff final to get compilers to devirtualize more

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file)

No description provided.
Attached patch final.patchSplinter Review
Attachment #8466525 - Flags: review?(nfroyd)
froydnj I can spread this review around if you like, but I'm not sure there's much point.
Comment on attachment 8466525 [details] [diff] [review]
final.patch

Review of attachment 8466525 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: content/base/public/nsNameSpaceManager.h
@@ +65,5 @@
>   * the NameSpace IDs are needed.
>   *
>   */
>  
> +class nsNameSpaceManager MOZ_FINAL

File a followup to remove virtual methods from this, if it can be legitimately marked MOZ_FINAL?

::: docshell/base/nsDocShell.h
@@ +127,5 @@
> +                             public nsIWebNavigation,
> +                             public nsIBaseWindow, 
> +                             public nsIScrollable, 
> +                             public nsITextScroll, 
> +                             public nsIDocCharset, 

Might as well trim the trailing whitespace if you're going to modify these lines.

::: dom/base/nsGlobalWindow.h
@@ +409,3 @@
>  
>    virtual mozilla::EventListenerManager*
> +    GetOrCreateListenerManager() MOZ_OVERRIDE MOZ_FINAL;

Why don't you just move these to a separate patch that MOZ_FINAL's a bunch of nsGlobalWindow's virtual methods?

::: layout/forms/nsListControlFrame.h
@@ +46,5 @@
>   * Frame-based listbox.
>   */
>  
> +class nsListControlFrame MOZ_FINAL : public nsHTMLScrollFrame,
> +                                     public nsIFormControlFrame, 

Likewise here with the trailing whitespace.

::: layout/xul/grid/nsGridCell.h
@@ +24,5 @@
>   * When asked for preferred/min/max sizes it works like a stack and takes the 
>   * biggest sizes.
>   */
>  
> +class nsGridCell MOZ_FINAL

File a followup to remove virtual methods from this, if it can be legitimately marked MOZ_FINAL?

::: netwerk/protocol/http/nsHttpChannel.h
@@ +38,5 @@
>  //-----------------------------------------------------------------------------
>  
> +class nsHttpChannel MOZ_FINAL : public HttpBaseChannel
> +                              , public HttpAsyncAborter<nsHttpChannel>
> +                                , public nsIStreamListener

Slightly too much indentation here and for all the lines below.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +40,5 @@
> +                                 , public nsIInputStreamCallback
> +                                 , public nsIOutputStreamCallback
> +                                 , public nsITransportEventSink
> +                                 , public nsIInterfaceRequestor
> +                                 ,  public NudgeTunnelCallback

Wrong indentation of |public| here.
Attachment #8466525 - Flags: review?(nfroyd) → review+
Oh, also, did you see how much more devirtualization gets done (with or without LTO)?
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Oh, also, did you see how much more devirtualization gets done (with or
> without LTO)?

no, since I can't see a way for it to hurt, though I suppose it would be interesting to do a primitive grep for calls and see how many more direct ones there are.  I generated the list of things to mark final from http://kam.mff.cuni.cz/~hubicka/final-warnings.txt
 so I guess that's kind of sort of similar to compilers we're currently using.
Is possible devirtualization the whole point of the bug? Many of added MOZ_FINAL classes can be easily not final in the future.
(In reply to alexander :surkov from comment #6)
> Is possible devirtualization the whole point of the bug? Many of added
> MOZ_FINAL classes can be easily not final in the future.

then they can easily be made not final.
I know, just curious about reason to make them final.
(In reply to alexander :surkov from comment #8)
> I know, just curious about reason to make them final.

then yes better devirtualization is the only reason I know of though it may help understanding the current state to know nothing inherits from a class.
so it's not for design proposes, just a hint for compiler. compilers aren't smart enough to figure out themselves?
(In reply to alexander :surkov from comment #10)
> so it's not for design proposes, just a hint for compiler. compilers aren't
> smart enough to figure out themselves?

basically no, its really hard for them to do at best.
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Oh, also, did you see how much more devirtualization gets done (with or
> without LTO)?

greping for ' call .* <.*>' returns ~3600 more hits, and then another couple hundred with a second patch. with gcc trunk and lto.  I'm not sure if some of that is speculation, or if its all full devirtualization.
landed without the xpcprivate.h bits because they seem to trip an an issue in the hazard analysis builds, and skipped the places stuff because its easier to push that later when I don't need to rebase using mercurial.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc727b5027c
Does the devirtualisation apply to the destructor or do you have to fix that manually? For example:
interface nsIFoo {
  attribute nsIFoo parent;
};
class Foo MOZ_FINAL: public nsIFoo {
  NS_DECL_ISUPPORTS
  NS_DECL_NSIFOO
private:
  ~Foo();
};
https://hg.mozilla.org/mozilla-central/rev/3bc727b5027c
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.