Allow inline virtual methods

[Jeffrey Yasskin]

One place the Chromium style guide diverges from both the Google and
Blink style guides is in our prohibition of all inline functions with
bodies. We say:

"error: [chromium-style] virtual methods with non-empty bodies
shouldn't be declared inline."

Google says:

"The main reason for making a virtual function inline is to place its
definition in the class, either for convenience or to document its
behavior, e.g., for accessors and mutators."
--https://google.github.io/styleguide/cppguide.html#Inline_Functions

Blink says nothing on the subject, but uses short inline virtual functions.


Moving a "return false;" default from a header to a .cc file hurts an
interface's readability, the opposite of what a style guide is
supposed to do.

I propose we remove or relax the clang plugin check
(FindBadConstructsConsumer::CheckVirtualBodies) that forbids these
functions. I'd default to removing it, to simplify the rules that can
break builds by surprise, but if lots of people want to limit the
number or complexity of statements, that'd be possible too.

Jeffrey

[Brett Wilson]

If I recall, forcing these to be non-inline sped up the build. I think otherwise the compiler must generate function definitions in every compilation unit, and then picks one at link time. My recollection could be wrong or out-of-date though.

But I do remember that this rule was made after some discussion and quite a bit of effort was put into converting the code to use the new style. Has something changed from that? I suggest that you, as the person advocating this change, dig up the previous discussion/bug and reply with a link (it wasn't obvious to me).

Brett

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev

[Jeffrey Yasskin]

It looks like https://groups.google.com/a/chromium.org/d/topic/chromium-dev/QEpKyLOQvQY/discussion
has the conclusion of the "Faster Builds Task Force", but not a record
of their discussions. Enforcement dates to the initial checkin of the
clang plugin: http://codereview.chromium.org/6368055.

If the problem is the time spent to emit definitions into every
translation unit, we can solve that by requiring that every class have
a "key function". A class's key function is "the first non-pure
virtual function that is not inline at the point of class definition"
(https://mentorembedded.github.io/cxx-abi/abi.html#vague-vtable). When
classes don't have a key function, their vtable and all inline virtual
functions (including empty ones) are emitted in every compilation unit
that uses the class. Requiring that non-empty virtual functions be
non-inline probably increased the number of key functions in the
codebase, which sped up builds, but we could do better by matching the
rule the compiler uses.

There's also some cost to parsing long inline definitions, but that
wouldn't be constrained to virtual functions, and doesn't apply much
to the sort of short inline definitions that improve readability.

Jeffrey

[Brett Wilson]

On Mon, Nov 16, 2015 at 7:55 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

It looks like https://groups.google.com/a/chromium.org/d/topic/chromium-dev/QEpKyLOQvQY/discussion
has the conclusion of the "Faster Builds Task Force", but not a record
of their discussions. Enforcement dates to the initial checkin of the
clang plugin: http://codereview.chromium.org/6368055.

If the problem is the time spent to emit definitions into every
translation unit, we can solve that by requiring that every class have
a "key function". A class's key function is "the first non-pure
virtual function that is not inline at the point of class definition"
(https://mentorembedded.github.io/cxx-abi/abi.html#vague-vtable). When
classes don't have a key function, their vtable and all inline virtual
functions (including empty ones) are emitted in every compilation unit
that uses the class. Requiring that non-empty virtual functions be
non-inline probably increased the number of key functions in the
codebase, which sped up builds, but we could do better by matching the
rule the compiler uses.

There's also some cost to parsing long inline definitions, but that
wouldn't be constrained to virtual functions, and doesn't apply much
to the sort of short inline definitions that improve readability.

 

The rule "don't write inline virtual methods" is easy to understand and follow for all developers. In my opinion "make sure your class has a non-inline key function" is neither.

Brett

[Jeffrey Yasskin]

The rule would be, "if you have virtual methods, make sure at least 1
is defined in the .cc file."

[Zachary Turner]

This is exactly what clang / llvm does, and for the same reason.

[Peter Kasting]

On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

The rule would be, "if you have virtual methods, make sure at least 1

is defined in the .cc file."

I think that's still too complicated, and way too easy to do the wrong thing with when refactoring code.  It's also far easier to review changes and simply say "don't do that" when seeing an inline virtual rather than going and looking to see if there's some other virtual that's non-inline.

Also, the recommendations here were not just for faster builds; IIRC we got both binary size and performance wins from reducing the amount of inlining we were doing.  Neither build speed nor binary size are significant factors in google3, which is part of why it's plausible that this rule should actually differ for Chromium.

I'm in favor of leaving the rule as-is.

PK

[Ryan Hamilton]

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

The rule would be, "if you have virtual methods, make sure at least 1

is defined in the .cc file."

I think that's still too complicated, and way too easy to do the wrong thing with when refactoring code.  It's also far easier to review changes and simply say "don't do that" when seeing an inline virtual rather than going and looking to see if there's some other virtual that's non-inline.

​I completely agree. On the QUIC team, we share code with an internal repository. Having easy to remember rules is really important to helping folks understand what Chrome code needs to look like.

​

 

Also, the recommendations here were not just for faster builds; IIRC we got both binary size and performance wins from reducing the amount of inlining we were doing.  Neither build speed nor binary size are significant factors in google3, which is part of why it's plausible that this rule should actually differ for Chromium.

I'm in favor of leaving the rule as-is.

I've not actually seen any numbers about the impact of this restriction. Do we have any data?​

[Jeffrey Yasskin]

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:

> On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> The rule would be, "if you have virtual methods, make sure at least 1
>> is defined in the .cc file."
>
>
> I think that's still too complicated, and way too easy to do the wrong thing
> with when refactoring code. It's also far easier to review changes and
> simply say "don't do that" when seeing an inline virtual rather than going
> and looking to see if there's some other virtual that's non-inline.

Review effort is irrelevant when there's a clang plugin enforcing the
rule. The violation won't survive to the code review.

> Also, the recommendations here were not just for faster builds; IIRC we got
> both binary size and performance wins from reducing the amount of inlining
> we were doing.

The "no inlined virtuals" rule is justified in
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-inlining-virtual-methods
as "You can't inline virtual methods under most circumstances". So why
do you think adding a second level of inlining-prevention for virtuals
helps binary size?

> Neither build speed nor binary size are significant factors
> in google3, which is part of why it's plausible that this rule should
> actually differ for Chromium.

Icache use is a significant factor for google3, which is how reducing
inlining helped performance in Chrome. Jeff Dean's been known to
un-inline chunks of core google3 library code in order to improve his
programs' performance. Chrome is not a unique snowflake in this
regard.

Jeffrey

[Jeffrey Yasskin]

For that matter, object file size has been a significant factor in
google3 too. Check out http://wiki/Main/KeyMethod for an example.
Sorry, non-googlers.

[Dana Jansens]

On Tue, Nov 17, 2015 at 7:48 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:
> On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> The rule would be, "if you have virtual methods, make sure at least 1
>> is defined in the .cc file."
>
>
> I think that's still too complicated, and way too easy to do the wrong thing
> with when refactoring code.  It's also far easier to review changes and
> simply say "don't do that" when seeing an inline virtual rather than going
> and looking to see if there's some other virtual that's non-inline.

Review effort is irrelevant when there's a clang plugin enforcing the
rule. The violation won't survive to the code review.

Right, as long as tooling enforces the rule it seems fine. I hate rules that I have to think about at code-review time also, which is why I love clang/clang-format/presubmit things that make sure I don't have to think about it. I don't think see why we should block a rule that would bring us more in line with Google (and Blink!) style, which is the goal of the style guide, especially when we can still enforce our build times don't get worse as a result.

 

> Also, the recommendations here were not just for faster builds; IIRC we got
> both binary size and performance wins from reducing the amount of inlining
> we were doing.

I don't see anything about binary size mentioned on that email. Making things not inline does in general decrease binary size, but that email also says "You can't inline virtual methods under most circumstances, even if the method would otherwise be inlined because it's very short." which suggests that writing virtuals in the header should have little-to-no impact on binary size.

 

The "no inlined virtuals" rule is justified in
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-inlining-virtual-methods
as "You can't inline virtual methods under most circumstances". So why
do you think adding a second level of inlining-prevention for virtuals
helps binary size?

> Neither build speed nor binary size are significant factors
> in google3, which is part of why it's plausible that this rule should
> actually differ for Chromium.

Icache use is a significant factor for google3, which is how reducing
inlining helped performance in Chrome. Jeff Dean's been known to
un-inline chunks of core google3 library code in order to improve his
programs' performance. Chrome is not a unique snowflake in this
regard.

Jeffrey

[Peter Kasting]

On Tue, Nov 17, 2015 at 7:11 AM, Ryan Hamilton <r...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:

Also, the recommendations here were not just for faster builds; IIRC we got both binary size and performance wins from reducing the amount of inlining we were doing.  Neither build speed nor binary size are significant factors in google3, which is part of why it's plausible that this rule should actually differ for Chromium.

I've not actually seen any numbers about the impact of this restriction. Do we have any data?​

Elliot would be the one to remember if this is accurate and what data we had.  All I recall is him ranting at length about people doing this.  Elliot, you want to share any feedback you have?

PK

[Peter Kasting]

On Tue, Nov 17, 2015 at 7:48 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:
> On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> The rule would be, "if you have virtual methods, make sure at least 1
>> is defined in the .cc file."
>
>
> I think that's still too complicated, and way too easy to do the wrong thing
> with when refactoring code.  It's also far easier to review changes and
> simply say "don't do that" when seeing an inline virtual rather than going
> and looking to see if there's some other virtual that's non-inline.

Review effort is irrelevant when there's a clang plugin enforcing the
rule. The violation won't survive to the code review.

We don't yet run clang on all our platforms (e.g. Windows, where I do all my work).  Any code compiled only on non-clang platforms won't get this benefit.  (Yes, I actually do work with Windows-only code not infrequently.)

Also, even with a clang plugin, I think it is still easier to write and review code with the simple rule "don't inline virtuals" than with a more complicated set of guidelines and considerations.  And it leads to more consistent code; I can think of plenty of cases where a plausible thing to do, if we allowed inlined virtuals, would be to inline all the methods:

class SomethingDelegate {

  // Override these if you need to handle any of these events.

  virtual void OnAction1() {}

  virtual bool OnAction2() { return false; }

  virtual int OnAction3() { return 0; }

};

With the "key function" rule, we force people to go back and decide which of these methods to de-inline.  Then some are inline and some aren't.  Now future readers of the header file may assume the de-inlined function is more complicated or that there's some particular reason for this difference beyond just "make the compiler happy".  This kind of inconsistency is IMO not to be taken lightly.

Icache use is a significant factor for google3, which is how reducing
inlining helped performance in Chrome. Jeff Dean's been known to
un-inline chunks of core google3 library code in order to improve his
programs' performance. Chrome is not a unique snowflake in this
regard.

This is an argument that caution is needed when inlining, not only in Chrome but in Google code.  You're effectively arguing that the Google style rule needs to urge more caution than it does.  I don't see that as an argument for relaxing Chrome's rules; much easier to say "don't do this" than "maybe you can do this but be careful because there are subtle performance implications you likely won't notice".

In any case, if you're proposing relaxing the rules, I think the burden would be on you to show that neither build times nor performance would suffer.  We're making assumptions in this thread about why this rule was put in place and what the effects of relaxing it would be.  That doesn't fill me with confidence.

Incidentally, part of why I'm pushing back is not only that I think the cost here is non-negligible, but because I think the benefit _is_ negligible.  I see the readability gain here as minimal to none (which clearly you do not), so given the variety of possible costs above I don't feel like slogging through this is worth it.

PK

[Daniel Cheng]

On Tue, Nov 17, 2015 at 12:00 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 7:48 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:
> On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> The rule would be, "if you have virtual methods, make sure at least 1
>> is defined in the .cc file."
>
>
> I think that's still too complicated, and way too easy to do the wrong thing
> with when refactoring code.  It's also far easier to review changes and
> simply say "don't do that" when seeing an inline virtual rather than going
> and looking to see if there's some other virtual that's non-inline.

Review effort is irrelevant when there's a clang plugin enforcing the
rule. The violation won't survive to the code review.

We don't yet run clang on all our platforms (e.g. Windows, where I do all my work).  Any code compiled only on non-clang platforms won't get this benefit.  (Yes, I actually do work with Windows-only code not infrequently.)

Though it's not perfect, we do have FYI bots that build win clang. I know that the Windows clang devs frequently make patches or file bugs to track new breakage. So it's not as if it would go completely unnoticed. I expect this situation will continue to improve in the future (e.g. win clang would become part of the CQ/waterfall).

 

Also, even with a clang plugin, I think it is still easier to write and review code with the simple rule "don't inline virtuals" than with a more complicated set of guidelines and considerations.  And it leads to more consistent code; I can think of plenty of cases where a plausible thing to do, if we allowed inlined virtuals, would be to inline all the methods:

class SomethingDelegate {

  // Override these if you need to handle any of these events.

  virtual void OnAction1() {}

  virtual bool OnAction2() { return false; }

  virtual int OnAction3() { return 0; }

};

With the "key function" rule, we force people to go back and decide which of these methods to de-inline.  Then some are inline and some aren't.  Now future readers of the header file may assume the de-inlined function is more complicated or that there's some particular reason for this difference beyond just "make the compiler happy".  This kind of inconsistency is IMO not to be taken lightly.

If the proposed rule change were in effect, I would probably just ask the developer to out-of-line define a virtual destructor.

 

Icache use is a significant factor for google3, which is how reducing
inlining helped performance in Chrome. Jeff Dean's been known to
un-inline chunks of core google3 library code in order to improve his
programs' performance. Chrome is not a unique snowflake in this
regard.

This is an argument that caution is needed when inlining, not only in Chrome but in Google code.  You're effectively arguing that the Google style rule needs to urge more caution than it does.  I don't see that as an argument for relaxing Chrome's rules; much easier to say "don't do this" than "maybe you can do this but be careful because there are subtle performance implications you likely won't notice".

In any case, if you're proposing relaxing the rules, I think the burden would be on you to show that neither build times nor performance would suffer.  We're making assumptions in this thread about why this rule was put in place and what the effects of relaxing it would be.  That doesn't fill me with confidence.

Incidentally, part of why I'm pushing back is not only that I think the cost here is non-negligible, but because I think the benefit _is_ negligible.  I see the readability gain here as minimal to none (which clearly you do not), so given the variety of possible costs above I don't feel like slogging through this is worth it.

PK

--

[Elliot Glaysher (Chromium)]

IIRC, this was almost entirely for build speed. I don't remember any binary size or runtime performance (I'm not saying there wasn't, just that I don't remember that). I do remember that the biggest wins were on compiling/linking on the mac, and the biggest wins were from banning inline ctors/dtors. IIRC, this caused major improvements in compile time on Mac because the amount of dependent symbols and template instantiations done. (thakis@ rolled gmock and got a 25% speed increase on the mac: https://chromium.googlesource.com/chromium/src/+/b58f8147cd1b3247aa3ec698eb12b9c16194d5c5 , after changes upstream to gmock and to our own gmock objects so that ctors/dtors were moved out of line. gmock was literally emitting hundreds of megabytes of redundant symbols in each translation unit. gmock now recommends that you should do this: https://code.google.com/p/googlemock/wiki/CookBook?ts=1297881104&updated=CookBook#Making_the_Compilation_Faster)

This is why the clang plugin tries to determine the weight of a ctor/dtor instead of just blindly banning ctors/dtors in headers. ( https://chromium.googlesource.com/chromium/chromium/+/master/tools/clang/plugins/FindBadConstructs.cpp#147 ). However, we want to discourage it in general because that's easier than saying "well sometimes you can, sometimes you can't, think about the compile time speed implications."

[Peter Kasting]

On Tue, Nov 17, 2015 at 12:26 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:00 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 7:48 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:
> On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> The rule would be, "if you have virtual methods, make sure at least 1
>> is defined in the .cc file."
>
>
> I think that's still too complicated, and way too easy to do the wrong thing
> with when refactoring code.  It's also far easier to review changes and
> simply say "don't do that" when seeing an inline virtual rather than going
> and looking to see if there's some other virtual that's non-inline.

Review effort is irrelevant when there's a clang plugin enforcing the
rule. The violation won't survive to the code review.

We don't yet run clang on all our platforms (e.g. Windows, where I do all my work).  Any code compiled only on non-clang platforms won't get this benefit.  (Yes, I actually do work with Windows-only code not infrequently.)

Though it's not perfect, we do have FYI bots that build win clang. I know that the Windows clang devs frequently make patches or file bugs to track new breakage. So it's not as if it would go completely unnoticed. I expect this situation will continue to improve in the future (e.g. win clang would become part of the CQ/waterfall).

Yes, I am hoping this would eventually improve, but right now Clang for Windows isn't turned on in enough situations to save you here.

With the "key function" rule, we force people to go back and decide which of these methods to de-inline.  Then some are inline and some aren't.  Now future readers of the header file may assume the de-inlined function is more complicated or that there's some particular reason for this difference beyond just "make the compiler happy".  This kind of inconsistency is IMO not to be taken lightly.

If the proposed rule change were in effect, I would probably just ask the developer to out-of-line define a virtual destructor.

That assumes this gets to you in review.  The point of the clang plugin was to force the developer to deal with this before the reviewer sees it.  So the developer chooses some solution -- either de-inlining one of these methods or defining an out-of-line destructor.  Now you as a reviewer see the now-compliant code and have to decide why the code now looks this way.  Certainly when I read code that explicitly gives a destructor I immediately think about why the author would have done that and what they're trying to tell me, as the reader, in terms of how I should use or modify that code.

The point is that the rationale for that is now subtle.  We could add comments everywhere we do this -- "declare an out-of-line destructor as a key function to allow inlining the rest of the methods here" -- but no clang plugin is going to enforce that and practically speaking it's not going to happen.

C++ is already riddled with things too subtle for their own good.  I'd rather not add more burden on this front.

PK

[Nico Weber]

On Tue, Nov 17, 2015 at 11:59 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 7:48 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 12:13 AM, Peter Kasting <pkas...@chromium.org> wrote:
> On Mon, Nov 16, 2015 at 8:50 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> The rule would be, "if you have virtual methods, make sure at least 1
>> is defined in the .cc file."
>
>
> I think that's still too complicated, and way too easy to do the wrong thing
> with when refactoring code.  It's also far easier to review changes and
> simply say "don't do that" when seeing an inline virtual rather than going
> and looking to see if there's some other virtual that's non-inline.

Review effort is irrelevant when there's a clang plugin enforcing the
rule. The violation won't survive to the code review.

I think there's an existing warning for this we could just turn on (-Wweak-vtables). (Remember the first rule for writing clang plugins, which is even the heading on https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_plugins.md: "Don't write a clang plugin")

 

We don't yet run clang on all our platforms (e.g. Windows, where I do all my work).  Any code compiled only on non-clang platforms won't get this benefit.  (Yes, I actually do work with Windows-only code not infrequently.)

We have bots building chrome on Windows and people who keep them green. Also, I think key functions aren't a thing on Windows, so having key functions is less useful there (the vtable goes into every translation unit).

Before talking about changing this, it'd probably make sense to make sure build speed etc wouldn't regress.

 

Also, even with a clang plugin, I think it is still easier to write and review code with the simple rule "don't inline virtuals" than with a more complicated set of guidelines and considerations.  And it leads to more consistent code; I can think of plenty of cases where a plausible thing to do, if we allowed inlined virtuals, would be to inline all the methods:

class SomethingDelegate {

  // Override these if you need to handle any of these events.

  virtual void OnAction1() {}

  virtual bool OnAction2() { return false; }

  virtual int OnAction3() { return 0; }

};

With the "key function" rule, we force people to go back and decide which of these methods to de-inline.  Then some are inline and some aren't.  Now future readers of the header file may assume the de-inlined function is more complicated or that there's some particular reason for this difference beyond just "make the compiler happy".  This kind of inconsistency is IMO not to be taken lightly.

Icache use is a significant factor for google3, which is how reducing
inlining helped performance in Chrome. Jeff Dean's been known to
un-inline chunks of core google3 library code in order to improve his
programs' performance. Chrome is not a unique snowflake in this
regard.

This is an argument that caution is needed when inlining, not only in Chrome but in Google code.  You're effectively arguing that the Google style rule needs to urge more caution than it does.  I don't see that as an argument for relaxing Chrome's rules; much easier to say "don't do this" than "maybe you can do this but be careful because there are subtle performance implications you likely won't notice".

In any case, if you're proposing relaxing the rules, I think the burden would be on you to show that neither build times nor performance would suffer.  We're making assumptions in this thread about why this rule was put in place and what the effects of relaxing it would be.  That doesn't fill me with confidence.

Incidentally, part of why I'm pushing back is not only that I think the cost here is non-negligible, but because I think the benefit _is_ negligible.  I see the readability gain here as minimal to none (which clearly you do not), so given the variety of possible costs above I don't feel like slogging through this is worth it.

PK

--

[Peter Kasting]

On Tue, Nov 17, 2015 at 12:43 PM, Nico Weber <tha...@chromium.org> wrote:

I think key functions aren't a thing on Windows, so having key functions is less useful there (the vtable goes into every translation unit).

Before talking about changing this, it'd probably make sense to make sure build speed etc wouldn't regress.

Are you saying that for Clang/Win, even if we followed the "key function" recommendations with our code, the compiler would still behave as if there were no key function?  That seems like a pretty major issue.

I wonder what MSVC's rules are?

PK

[Nico Weber]

Those are the Windows rules. clang/win tries to match MSVC in ~everything (else building parts of the code with MSVC and parts with clang/win wouldn't work, and that's the configuration we used during bring-up heavily). So yes, that's what I'm saying, and that's what MSVC does too.

[Jeffrey Yasskin]

Thanks for finding that. I think we'll want to tweak the rule a bit, but then we should definitely upstream the result.

> Before talking about changing this, it'd probably make sense to make sure
> build speed etc wouldn't regress.

I measured the effect on object file sizes and compile times of various combinations of inline and out-of-line empty and {return false;} functions. This doesn't check linking time (which IIRC is proportional to total input size) or the effect of longer function bodies. I've attached the script so you folks can check I did it right and can test your own ideas.

The script generates 50 interfaces, each with the same combination of void-returning and bool-returning methods, which can be inline or out-of-line. The inline ones are defined as either {} or {return false;}. It then generates uses of each interface, where a use is {return new InterfaceN;}. Uses of the form {ifaceN->method();} don't generate vtables at all, which has an interesting implication below.

​

You can see that the effect of having all-empty-inline functions is as large as having all-short-inline functions, and that as long as there's any out-of-line function, it doesn't matter whether your inline functions are empty or not.

When all functions are inline, you have to call the class's constructor in order to incur the cost. I think that means we can exempt base classes from the check, since base classes will only be constructed in a few compilation units. This diverges from the existing -Wweak-vtables check, so we should probably test it in the clang-plugin before upstreaming it.

We could try to identify a base class by the presence of a pure virtual method, although that might not help much for existing Blink classes, or by all constructors being `protected`. We could also exempt classes that are only instantiated in out-of-line Create() functions, but that rule might prove too complicated to remember.

What other measurements do folks think are relevant?

Jeffrey

[Peter Kasting]

On Mon, Nov 23, 2015 at 3:49 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

What other measurements do folks think are relevant?

Were your measurements performed under Clang/Linux?  It seemed like from the thread that Windows doesn't follow the same rules here and is potentially on the hook for much worse effects from inlining, based on what Nico reported.  I'd like to see the measurements you took under MSVC for Debug and Release.

PK 

[Jeffrey Yasskin]

I read Nico's email as saying that out-of-lining things doesn't help
on Windows (equivalently, inlining doesn't hurt), but I'll go measure
there too, to validate that.

[Peter Kasting]

Oh, that's very different than how I read it: I read it as "inlining is disaster in all cases", a sort of nail-in-the-coffin message.

PK

[Stefan Zager]

At this point in the thread, I think it's worth reiterating the original motivation for this change, as that seems to have been buried under the discussion of build times, binary size, performance, etc.:

Inlining the definition of trivial methods can make code significantly more readable.

Personally, I think that's a net benefit that ought to be weighed in the decision.

[Peter Kasting]

On Tue, Nov 24, 2015 at 11:45 AM, Stefan Zager <sza...@google.com> wrote:

At this point in the thread, I think it's worth reiterating the original motivation for this change, as that seems to have been buried under the discussion of build times, binary size, performance, etc.:

Inlining the definition of trivial methods can make code significantly more readable.

That is a subjective point of view.  I find inlining methods to make code slightly less readable.

PK 

[Stefan Zager]

On Tue, Nov 24, 2015 at 11:47 AM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Nov 24, 2015 at 11:45 AM, Stefan Zager <sza...@google.com> wrote:

At this point in the thread, I think it's worth reiterating the original motivation for this change, as that seems to have been buried under the discussion of build times, binary size, performance, etc.:

Inlining the definition of trivial methods can make code significantly more readable.

Fair enough, but this point hasn't really been discussed on this thread, as it should be.

[Peter Kasting]

I don't know how to discuss this in a constructive manner so I was hoping to avoid bikeshedding.  "It's more readable!"  "No it's not, it's less!"  "No it's not!" 

If instead we find that on Windows we take a build time hit, that's a bit easier to debate.

PK

[Stefan Zager]

On Tue, Nov 24, 2015 at 11:55 AM Peter Kasting <pkas...@chromium.org> wrote:

I don't know how to discuss this in a constructive manner so I was hoping to avoid bikeshedding.  "It's more readable!"  "No it's not, it's less!"  "No it's not!" 

If instead we find that on Windows we take a build time hit, that's a bit easier to debate.

That seems backwards to me.  If consensus is that inlining method definitions does not benefit readability, then that should be enough to scuttle the proposal.  If there *is* consensus (or at least strong interest), then we proceed to the discussion of whether there's a downside.

[Stefan Zager]

On Tue, Nov 24, 2015 at 11:58 AM Stefan Zager <sza...@google.com> wrote:

I meant: "if there *is* consensus that it's a good idea ..."

[Peter Kasting]

Debating which order to debate these in itself seems like a waste of time to me :).  We wanted to know costs, so we started discussing them.  You want agreement on benefits, so you posted about it.  People can reply to either as they wish, which I've already done.  What's the problem?

PK

[Stefan Zager]

If others feel as you do -- that it harms readability rather than helps -- then Jeffrey doesn't have to waste his time running experiments the results of which will make not one bit of difference, and we can end the torturous discussion of the minutiae of build time, performance, key methods, etc.

[Mike Reed]

fwiw -- Skia sometimes inlines those very short virtuals (e.g. return nullptr;  or return false;) for readability.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Peter Kasting]

On Tue, Nov 24, 2015 at 12:09 PM, Stefan Zager <sza...@google.com> wrote:

If others feel as you do -- that it harms readability rather than helps -- then Jeffrey doesn't have to waste his time running experiments the results of which will make not one bit of difference, and we can end the torturous discussion of the minutiae of build time, performance, key methods, etc.

We've had lengthy discussions on the readability of inlined code before which has made it clear that there is not consensus support for a strict viewpoint of "inlining harms readability".  Those debates very much did become bikeshedding, and my actions here were driven by that concrete past experience.

PK

[Yuri Wiitala]

OOC, have we done any tests on 'final' virtual methods (inlined or not)?  Seems like use of 'final' instead of 'override' could have a large impact on build times, linkage, and execution times.

--

[Jeffrey Yasskin]

cl /O2 /c use.cc /Fouse.o

​

cl /Od /Zi /c use.cc /Fouse.o

clang-cl /O2 /c use.cc /Fouse.o (http://llvm.org/releases/3.7.0/LLVM-3.7.0-win64.exe)

​

​So inlining does increase size incrementally on Windows, but the emptiness of the function doesn't affect it on Windows either. (My guess is that MSVC generates all the inline functions it has available, even if that's not enough to create a complete vtable.) On MSVC, the compile time doesn't look significantly affected, but 1) the object file size increase will affect link time, and 2) I don't entirely trust the times since they also don't track well in the #interfaces graph. I might be missing a flush of some sort. The Clang folks should also note that I'm measuring them as almost 10x as slow as MSVC, for some reason.

Given that the current rule doesn't match reality on any platform, we should change it in some direction. Either no virtual functions should be inline, or small non-empty virtual inlines should be allowed. I prefer allowing short inline virtual functions, and taking the hit on Windows. Since Elliot reported that the "biggest wins were from banning inline ctors/dtors", I actually don't expect much effect from allowing more inline virtual functions. I'm not sure how to measure this cheaply though.

Jeffrey

[Jeffrey Yasskin]

On Tue, Nov 24, 2015 at 1:30 PM, Yuri Wiitala <m...@chromium.org> wrote:
> OOC, have we done any tests on 'final' virtual methods (inlined or not)?
> Seems like use of 'final' instead of 'override' could have a large impact on
> build times, linkage, and execution times.

Please do take my script and run whatever tests you want.

[Peter Kasting]

On Tue, Nov 24, 2015 at 1:31 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

Given that the current rule doesn't match reality on any platform, we should change it in some direction. Either no virtual functions should be inline, or small non-empty virtual inlines should be allowed.

Isn't "no virtual functions should be inline" the current rule?  From the Chromium style guide: "Simple accessors should generally be the only inline functions. These should be named unix_hacker_style().  Virtual functions should never be declared this way." (emphasis original)  It then directs people to the dos-and-don'ts document which also explicitly says not to inline virtuals.

Given this, and that there is both a link time and generated code size impact on Windows, and that (as I noted before) I actually find inlining these less readable, I'm strongly in favor of keeping the current rule.

PK

[Nico Weber]

The motivation for the rule was code size / build time. Jeffrey's experiment shows that neither of those change for small functions, so I think we should default to google style (which doesn't have this rule).

I agree it doesn't matter much what individuals think is more or less readable, given that opinions are split on this. 

[Jeffrey Yasskin]

Do you want me to send a change to remove this from the Clang plugin,
or to replace it with something like -Wweak-vtables? I don't think we
want to use -Wweak-vtables itself because it has 4257 unique errors:
https://drive.google.com/file/d/0B8-yQ8-lB7qGQ2JMX2xUaHc4eXc/view?usp=sharing.
We could upstream the resulting warning once it looks like it's
working.

Jeffrey

[Bruce]

My ancient and unreliable recollection is that VC++ doesn't use a "key" function to decide when to generate the vtable (and hence the inline virtual functions), it instead does that generation in any translation unit that creates an object of that type. So, the cost is likely to be proportional to the number of translation units creating that type (or that that contain or derive from it) which in most cases will be fairly modest.

I have not validated this vague memory but I thought I'd mention it in case it informed any tests that might be run.

[Peter Kasting]

On Tue, Nov 24, 2015 at 5:45 PM, Bruce <bruce...@chromium.org> wrote:

My ancient and unreliable recollection is that VC++ doesn't use a "key" function to decide when to generate the vtable (and hence the inline virtual functions), it instead does that generation in any translation unit that creates an object of that type. So, the cost is likely to be proportional to the number of translation units creating that type (or that that contain or derive from it) which in most cases will be fairly modest.

I have not validated this vague memory but I thought I'd mention it in case it informed any tests that might be run.

If true, that would explain why I think we saw some of the largest effects from things like core protobuf objects that end up being instantiated and used in many different places.

That would also argue in favor of "prefer not to inline anything if your class will be widely used", which might be a bit too fiddly as a rule.

PK

[Jeffrey Yasskin]

On Tue, Nov 24, 2015 at 5:58 PM, Peter Kasting <pkas...@chromium.org> wrote:
> On Tue, Nov 24, 2015 at 5:45 PM, Bruce <bruce...@chromium.org> wrote:
>>
>> My ancient and unreliable recollection is that VC++ doesn't use a "key"
>> function to decide when to generate the vtable (and hence the inline virtual
>> functions), it instead does that generation in any translation unit that
>> creates an object of that type. So, the cost is likely to be proportional to
>> the number of translation units creating that type (or that that contain or
>> derive from it) which in most cases will be fairly modest.
>>
>> I have not validated this vague memory but I thought I'd mention it in
>> case it informed any tests that might be run.
>
>
> If true, that would explain why I think we saw some of the largest effects
> from things like core protobuf objects that end up being instantiated and
> used in many different places.

This points at the other way to guarantee a vtable is only generated in 1 .o file: make the constructors or creation functions out-of-line:

​This is:

// inline ctor & create

class Interface0 {

 public:

  Interface0() {}

  Interface0* Create() { return new Interface0(); }

  virtual void VoidMethod0() {}

  ...

};

// inline create

class Interface0 {

 public:

  Interface0() ;

  Interface0* Create() { return new Interface0(); }

  virtual void VoidMethod0() {}

  ...

};

// outline ctor & create

class Interface0 {

 public:

  Interface0() ;

  Interface0* Create() ;

  virtual void VoidMethod0() {}

  ...

};

[Reid Kleckner]

On Wed, Nov 25, 2015 at 2:34 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

This points at the other way to guarantee a vtable is only generated in 1 .o file: make the constructors or creation functions out-of-line:

The destructor may also reference the vtable, so you'd want to make sure you move that out of line as well. I think Chromium already encourages this to reduce code size.

This is a foolproof way to avoid duplicate vtables and code in object files, but it will also block the (not very good) devirtualization transformations LLVM currently has. They are currently built around the idea of forwarding vptr stores to vptr loads, and if you hide the vptr store, we're hosed.

That said, moving ctors/dtors of polymorphic classes out of line is probably the Right Thing To Do. The fact that LLVM can't devirtualize here is just a missed optimization. Using LTO (LTCG by another name) will make it work today. Last summer we had a intern work on devirtualization that doesn't depend on constructor inlining, but his prototype doesn't work on classes with inline virtual methods. :(

The cppcon lightning talk on Clang/LLVM devirtualization for the morbidly curious: https://www.youtube.com/watch?v=lQAxldEGOys

[Bruce Dawson]

This brings back "fond" memories of tracking down a de-virtualization code-gen bug in VC++ 2013 (Update 1 or 2 I think). Luckily I'd just watched a video talking about that exact optimization or else I would never have figured out what was going on.

[Elliott Sprehn]

On Wed, Nov 25, 2015 at 3:24 PM, 'Reid Kleckner' via Chromium-dev <chromi...@chromium.org> wrote:

On Wed, Nov 25, 2015 at 2:34 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

This points at the other way to guarantee a vtable is only generated in 1 .o file: make the constructors or creation functions out-of-line:

The destructor may also reference the vtable, so you'd want to make sure you move that out of line as well. I think Chromium already encourages this to reduce code size.

This is a foolproof way to avoid duplicate vtables and code in object files, but it will also block the (not very good) devirtualization transformations LLVM currently has. They are currently built around the idea of forwarding vptr stores to vptr loads, and if you hide the vptr store, we're hosed.

That said, moving ctors/dtors of polymorphic classes out of line is probably the Right Thing To Do. The fact that LLVM can't devirtualize here is just a missed optimization. Using LTO (LTCG by another name) will make it work today. Last summer we had a intern work on devirtualization that doesn't depend on constructor inlining, but his prototype doesn't work on classes with inline virtual methods. :(

Note that moving constructors and destructors out of line makes them more expensive since calling the super constructor/destructor then becomes a non-inline function call, that's probably sensible as the default, but we should have a way to explicitly use an inline super call as well.

- E 

[Daniel Bratell]

On Thu, 26 Nov 2015 05:16:32 +0100, Elliott Sprehn <esp...@chromium.org> wrote:

Note that moving constructors and destructors out of line makes them more expensive since calling the super constructor/destructor then becomes a non-inline function call, that's probably sensible as the default, but we should have a way to explicitly use an inline super call as well.

I think that is more in the line of "may make them more expensive". With inlining numerous parameters like instruction cache contents, call costs, code layout, code size and several more will determine what alternative is faster and what is slower. And what alternative is smaller and what is larger.

I think defaulting to outlining unless we can see the call cost appear in a profile is the sensible base rule. In particular with destructors and constructors which can be called implicitly so you may not even be aware that you are sprinkling calls to them all over the code, and since destructors and constructors implicitly call the destructors and constructors of their member variables, it may be quite hard to determine if they are trivial or not.

(This is not new, this is basically the reason the clang plugins exist today)

/Daniel

--

/* Opera Software, Linköping, Sweden: CET (UTC+1) */

[Tom Hudson]

On Thu, Nov 26, 2015 at 4:34 AM, Daniel Bratell <bra...@opera.com> wrote:

On Thu, 26 Nov 2015 05:16:32 +0100, Elliott Sprehn <esp...@chromium.org> wrote:

Note that moving constructors and destructors out of line makes them more expensive since calling the super constructor/destructor then becomes a non-inline function call, that's probably sensible as the default, but we should have a way to explicitly use an inline super call as well.

I think that is more in the line of "may make them more expensive". With inlining numerous parameters like instruction cache contents, call costs, code layout, code size and several more will determine what alternative is faster and what is slower. And what alternative is smaller and what is larger.

This kind of reasoning is very difficult to apply to mobile Chrome; last I knew we built -Os to optimize for size, and that puts a very slim limit on the number of functions that are inlined, with the compiler deciding arbitrarily to outline many functions. I don't know what flags Opera uses, but when I was experimenting with inlining bits of Blink you could see this effect; when we added force-inline pragmas the compiler just forcibly outlined other previously-inlined functions and the performance optimization was a total muddle.

Tom

[Daniel Bratell]

I think we agree. I tried to made the point that inlining code might not have the positive effects one would think and and your opinion seems to be the same.

In particular gcc has a very complicated optimization/inlining strategy which makes it very hard to argue about inlining at all. When working on binary size problems I discovered that gcc has a limit on how much it inlines per compilation unit and if you hit that limit (easily done in Blink), removing one inlined function would only inline some other, often completely unrelated, function instead.

That heuristic also made the machine code smaller if you concatenated two cpp files since it would inline less, and made the machine code larger if you split the code into more compilation units.

[Bruce Dawson]

The VC++ inlining strategy is also complicated and inscrutable. I helped recently with a project that had some DCHECK statements in crucial functions. Despite the fact that these were being compiled out they were preventing operator[] and friends from being inlined, which was pretty tragic.

The DCHECK statements compiled to something like:

    if (false && LongComplicatedExpression)

The compiler did figure out that the expression should generate no code, but it figured that out after it decided not to inline. Some careful simplifications made the compiler happy and gave a ~40x speedup. Messy.

[Jeffrey Yasskin]

On Wed, Nov 25, 2015 at 3:24 PM, Reid Kleckner <r...@google.com> wrote:
> On Wed, Nov 25, 2015 at 2:34 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> This points at the other way to guarantee a vtable is only generated in 1
>> .o file: make the constructors or creation functions out-of-line:
>
>
> The destructor may also reference the vtable, so you'd want to make sure you
> move that out of line as well. I think Chromium already encourages this to
> reduce code size.

Good point. However, MSVC doesn't appear to generate the vtable when it calls an inline *virtual* destructor.

¯\_(ツ)_/¯ 

​​Clang-3.7 looks like it's making the same optimization for non-virtual destructors, but given that it doesn't implement the key-function optimization on Windows, that's probably a mistake.

So ... trying to make everyone happy, I'm looking at several points:

1. Having a key function dramatically reduces the object size for Unix, but doesn't on Windows.
2. To accommodate Windows, let's try to do this without the key-function optimization. That is, assume adding a single inline virtual function has an incremental cost.
3. This cost is incurred where a constructor or non-virtual destructor is inlined.
4. Base class constructors tend to be constructed in few places (the subclasses), while leaf classes are instantiated in many.
5. Elliott's point: Having inline con/destructors is especially useful for base classes where you'd like the whole chain to be emitted inside the leaf class's out-of-line con/destructor.

At the moment, I'm thinking of extending the "out-line your constructors and destructors" rule (https://code.google.com/p/chromium/codesearch/#chromium/src/tools/clang/plugins/FindBadConstructsConsumer.cpp&l=258-275) to include the number of inline virtual member functions as a cost for leaf classes.

• I'd identify a leaf class as having no pure virtual methods and a non-protected constructor.
• If a class has any virtual members, I'd add 3 + the number of inline virtual members to ctor_score, and if the destructor is non-virtual, the same to dtor_score.
  

Users would get the "Complex class/struct needs an explicit out-of-line constructor." or "Complex class/struct needs an explicit out-of-line destructor." error instead of anything specifically about virtual functions. We could word-smith that, or special-case when inlined virtuals pushed it over the edge, or just let folks out-of-line their constructors instead of their methods.

Thoughts?

[Peter Kasting]

On Mon, Nov 30, 2015 at 1:52 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

At the moment, I'm thinking of extending the "out-line your constructors and destructors" rule (https://code.google.com/p/chromium/codesearch/#chromium/src/tools/clang/plugins/FindBadConstructsConsumer.cpp&l=258-275) to include the number of inline virtual member functions as a cost for leaf classes.

• I'd identify a leaf class as having no pure virtual methods and a non-protected constructor.
• If a class has any virtual members, I'd add 3 + the number of inline virtual members to ctor_score, and if the destructor is non-virtual, the same to dtor_score.
  

Users would get the "Complex class/struct needs an explicit out-of-line constructor." or "Complex class/struct needs an explicit out-of-line destructor." error instead of anything specifically about virtual functions. We could word-smith that, or special-case when inlined virtuals pushed it over the edge, or just let folks out-of-line their constructors instead of their methods.

Thoughts?

Can you summarize what sort of recommendation [change] you're proposing for the style guide at this point?  Is the proposal to remove all recommendations about inlining, let people do whatever, and then have some kind of a Clang rule that hopefully prevents some set of bad behaviors?  Or are we targeting a more specific way of relaxing or modifying the current inlining rules?  I've gotten lost in all this.

PK

[Jeffrey Yasskin]

I don't want to have a Clang-plugin rule that's totally disconnected
from anything in the style guide, but I think we can refine the
existing rule against inlining constructors and destructors to cover
this, and remove the virtual-inline recommendations.

Specifically, I'd remove "Virtual functions should never be declared
this way." from
https://www.chromium.org/developers/coding-style#TOC-Inline-functions
and the "Stop inlining virtual methods." section from
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts.
I'd add a paragraph to "Stop inlining constructors and destructors" to
mention the cost of vtable generation, and to "When you CAN inline
constructors and destructors" to outline the new clang-plugin rules
(at the same level of detail as the current outline).

Jeffrey

[Jeffrey Yasskin]

I have a patch for the clang plugin at https://codereview.chromium.org/1493813003/, and a partial list of fixes in https://codereview.chromium.org/1499793003.

A bunch of classes that hit the new check mark that they're intended as base classes by providing a protected destructor but no protected constructor. Anyone have thoughts on including that signal in the IsLeafClass check? It's a little less reliable since a base class could provide a public destructor, but this seems unlikely to happen by accident.

Jeffrey

[Jeffrey Yasskin]

Protected destructors for leaf classes turn out to happen because our style checks for RefCounted<> require that destructors be non-public. So that idea's out.