Start deinlining non-empty virtual methods. (This will be automatically checked

Start deinlining non-empty virtual methods. (This will be automatically checked
for in the future.)

BUG=none
TEST=compiles

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68746

[Elliot Glaysher, 2010-12-08 22:55:09]

(Looking for rubberstamp)
 
Now that I've made it an error to declare a virtual override w/o the virtual
keyword, I'm going to deinline all virtual methods. They'll never get inlined
because of dynamic dispatch.

Thing I've noticed, which is my next queued action: People have no idea when to
UseCamelCase() or use_accessor_style(). virtual methods should *never*
use_accessor_style(), and yet people are doing that all the time. This should be
easy to check for from my clang plugin, but I want to get all the code out of
the headers first.

[willchan no longer on Chromium, 2010-12-08 23:01:41]

Minor concern that I want to raise that doesn't mean this change isn't good,
but is something to be aware of.

There _are_ cases where inlined virtuals will be inlined instead of routing
through dynamic dispatch.  Specifically, if we have code that instantiates
an object on the stack, then the compiler knows the exact type at that
point, and it can inline even if the member function is virtual.

Of course, this is not the common case, and the runtime gain of inlining in
these cases is probably negligible (I don't know any case where we purposely
do this for performance reasons).  So, I think this change is most likely a
net win.  It wasn't mentioned in the changelist description, so I just
wanted to mention that.

Cheers.

On Wed, Dec 8, 2010 at 2:55 PM, <erg@chromium.org> wrote:

> Reviewers: Lei Zhang, Ben Goodger, James Hawkins,
>
> Message:
> (Looking for rubberstamp)
>
> Now that I've made it an error to declare a virtual override w/o the
> virtual
> keyword, I'm going to deinline all virtual methods. They'll never get
> inlined
> because of dynamic dispatch.
>
> Thing I've noticed, which is my next queued action: People have no idea
> when to
> UseCamelCase() or use_accessor_style(). virtual methods should *never*
> use_accessor_style(), and yet people are doing that all the time. This
> should be
> easy to check for from my clang plugin, but I want to get all the code out
> of
> the headers first.
>
> Description:
> Start deinlining non-empty virtual methods. (This will be automatically
> checked
> for in the future.)
>
> BUG=none
> TEST=compiles
>
> Please review this at http://codereview.chromium.org/5574006/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M app/animation.h
>  M app/animation.cc
>  M app/menus/menu_model.h
>  M app/menus/menu_model.cc
>  M app/slide_animation.h
>  M app/slide_animation.cc
>  M app/throb_animation.h
>  M app/throb_animation.cc
>  M base/message_pump_glib.h
>  M base/message_pump_glib.cc
>  M base/metrics/histogram.h
>  M base/metrics/histogram.cc
>  M base/values_util.h
>  M base/values_util.cc
>  M chrome/browser/importer/importer.h
>  M chrome/browser/importer/importer.cc
>  M chrome/common/json_pref_store.h
>  M chrome/common/json_pref_store.cc
>  M chrome/common/net/gaia/gaia_authenticator.h
>  M chrome/common/net/gaia/gaia_authenticator.cc
>  M chrome/common/pref_store.h
>  M chrome/common/pref_store.cc
>  M chrome/common/pref_store_base.h
>  M chrome/common/pref_store_base.cc
>  M chrome/service/net/service_url_request_context.h
>  M chrome/service/net/service_url_request_context.cc
>  M chrome/worker/websharedworker_stub.h
>  M chrome/worker/websharedworker_stub.cc
>  M chrome/worker/webworker_stub.h
>  M chrome/worker/webworker_stub.cc
>  M chrome/worker/webworkerclient_proxy.h
>  M chrome/worker/webworkerclient_proxy.cc
>  M chrome/worker/worker_webapplicationcachehost_impl.h
>  M chrome/worker/worker_webapplicationcachehost_impl.cc
>  M media/base/buffers.h
>  M media/base/buffers.cc
>  M media/base/filters.h
>  M media/base/filters.cc
>  M media/base/video_frame.h
>  M media/base/video_frame.cc
>  M net/base/capturing_net_log.h
>  M net/base/capturing_net_log.cc
>  M net/base/cookie_monster.h
>  M net/base/cookie_monster.cc
>  M net/base/host_resolver.h
>  M net/base/host_resolver.cc
>  M net/ftp/ftp_directory_listing_parser_ls.h
>  M net/ftp/ftp_directory_listing_parser_ls.cc
>  M net/ftp/ftp_directory_listing_parser_mlsd.h
>  M net/ftp/ftp_directory_listing_parser_mlsd.cc
>  M net/ftp/ftp_directory_listing_parser_netware.h
>  M net/ftp/ftp_directory_listing_parser_netware.cc
>  M net/ftp/ftp_directory_listing_parser_vms.h
>  M net/ftp/ftp_directory_listing_parser_vms.cc
>  M net/ftp/ftp_directory_listing_parser_windows.h
>  M net/ftp/ftp_directory_listing_parser_windows.cc
>  M net/http/http_auth_handler.h
>  M net/http/http_auth_handler.cc
>  M net/http/http_stream_request.h
>  M net/http/http_stream_request.cc
>  M net/net.gyp
>  M net/server/http_listen_socket.h
>  M net/server/http_listen_socket.cc
>  M net/socket/socks_client_socket_pool.h
>  M net/socket/socks_client_socket_pool.cc
>  M net/socket/ssl_client_socket.h
>  A net/socket/ssl_client_socket.cc
>  M net/socket/tcp_client_socket_libevent.h
>  M net/socket/tcp_client_socket_libevent.cc
>  M net/socket/tcp_client_socket_pool.h
>  M net/socket/tcp_client_socket_pool.cc
>  M net/spdy/spdy_http_stream.h
>  M net/spdy/spdy_http_stream.cc
>  M ppapi/cpp/module.h
>  M ppapi/cpp/module.cc
>  M ppapi/proxy/host_dispatcher.h
>  M ppapi/proxy/host_dispatcher.cc
>  M ppapi/proxy/plugin_dispatcher.h
>  M ppapi/proxy/plugin_dispatcher.cc
>  M ppapi/proxy/plugin_resource.h
>  M ppapi/proxy/plugin_resource.cc
>  M remoting/jingle_glue/xmpp_socket_adapter.h
>  M remoting/jingle_glue/xmpp_socket_adapter.cc
>  M skia/ext/bitmap_platform_device_linux.h
>  M skia/ext/bitmap_platform_device_linux.cc
>  M webkit/glue/plugins/webplugin.h
>  M webkit/glue/plugins/webplugin.cc
>
>
>

[Elliot Glaysher, 2010-12-08 23:19:25]

On 2010/12/08 23:01:41, willchan wrote:
> There _are_ cases where inlined virtuals will be inlined instead of routing
> through dynamic dispatch.  Specifically, if we have code that instantiates
> an object on the stack, then the compiler knows the exact type at that
> point, and it can inline even if the member function is virtual.
> ...
> It wasn't mentioned in the changelist description, so I just
> wanted to mention that.

I'm aware that the compiler can do this, and the case with explicit
ClassName::Method() calls, it's just that I don't think that's going to happen
much, if ever. Sorry, I was speaking hyperbolically.

[scherkus (not reviewing), 2010-12-09 02:24:14]

one nit on media code (found a method that should'nt have been virtual in the
first place!)

http://codereview.chromium.org/5574006/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/5574006/diff/1/media/base/video_frame.h#newcode97
media/base/video_frame.h:97: virtual SurfaceType type() const;
just checked... this method shouldn't be virtual

[Elliot Glaysher, 2010-12-09 17:37:10]

Ping reviewers; looking for a rubberstamp.

On 2010/12/09 02:24:14, scherkus wrote:
http://codereview.chromium.org/5574006/diff/1/media/base/video_frame.h#newcode97
> media/base/video_frame.h:97: virtual SurfaceType type() const;
> just checked... this method shouldn't be virtual

Done.

[Elliot Glaysher, 2010-12-09 17:37:11]

Ping reviewers; looking for a rubberstamp.

On 2010/12/09 02:24:14, scherkus wrote:
http://codereview.chromium.org/5574006/diff/1/media/base/video_frame.h#newcode97
> media/base/video_frame.h:97: virtual SurfaceType type() const;
> just checked... this method shouldn't be virtual

Done.

[James Hawkins, 2010-12-09 18:03:59]

LGTM