PSA: ScopedTempDir's path() is deprecated, use GetPath()

[Václav Brožek]

Hi all,

To fix http://crbug.com/640599 I am adding a simple DCHECK against uninitialised use to the path accessor of ScopedTempDir.

To accomplish that without #including base/logging.h in the ScopedTempDir header, I needed to turn the inline path() accessor into an out-of-line GetPath. See https://codereview.chromium.org/2275553005/ for a discussion. That means changing callsites in almost 600 files, so I'll run a cascade of CLs to achieve that gradually. Until that's finished, both path() and GetPath() are available, but path() is deprecated.

Cheers,

Vaclav

[Peter Kasting]

On Thu, Sep 8, 2016 at 3:04 AM, Václav Brožek <va...@chromium.org> wrote:

To accomplish that without #including base/logging.h in the ScopedTempDir header, I needed to turn the inline path() accessor into an out-of-line GetPath. See https://codereview.chromium.org/2275553005/ for a discussion.

You cited increased compile time as the primary motivation for not just #including base/logging.h so this could be inline.

What is the actual compile time impact?  I'm skeptical it's meaningful.

In general, I discourage people from de-inlining accessors just because they have a DCHECK, and the Google Style Guide discourages people from jumping through hoops to avoid #includes.  That seems to go double when the #include is (a) as common as logging.h and (b) this function is primarily used in test code.

I think you should reconsider this aspect of your change before making this cascade of CLs.  Unless you have hard numerical evidence that this is a significant compile-time impact, especially for Chrome itself, this should just be path(), not GetPath().

PK

[Brett Wilson]

I personally think this kind of change is fine generally.

This case it seems like a significant amount of work because of the number of cases that need to be updated. And it seems like of relatively low benefit because most files include logging.h already. You might want to consider whether the benefit is worth the effort.

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

[Ryan Hamilton]

I'm a bit confused by the need to rename the method simply because it has been moved out-of-line. The style guide say:

Functions that are very cheap to call may instead follow the style for variable names (all lower-case, with underscores between words). 

https://google.github.io/styleguide/cppguide.html#Function_Names

It doesn't say that such methods HAVE to be inlined, does it?

[Daniel Cheng]

I think another factor to consider is that it's nice to minimize the number of exposed headers when reasonable and possible: we have a lot of IWYU problems already in Chromium. Exposing that header via base/files/scoped_temp_dir.h gives another way to indirectly include base/logging.h.

Daniel

[dan...@chromium.org]

On Thu, Sep 8, 2016 at 3:06 PM, Ryan Hamilton <r...@chromium.org> wrote:

I'm a bit confused by the need to rename the method simply because it has been moved out-of-line. The style guide say:

Functions that are very cheap to call may instead follow the style for variable names (all lower-case, with underscores between words). 

https://google.github.io/styleguide/cppguide.html#Function_Names

It doesn't say that such methods HAVE to be inlined, does it?

It's super vague about what is "cheap enough" and I think the whole rule is problematic.. but it does say "A canonical example is an inline method that just returns one of the class's member variables." It's not clear to me that a function that can't be inlined should be allowed to be hacker_case.

The guide *used* to say that hacker_case() could be used for inline accessors only IIRC.

[Peter Kasting]

On Thu, Sep 8, 2016 at 3:06 PM, Ryan Hamilton <r...@chromium.org> wrote:

I'm a bit confused by the need to rename the method simply because it has been moved out-of-line. The style guide say:

Functions that are very cheap to call may instead follow the style for variable names (all lower-case, with underscores between words). 

https://google.github.io/styleguide/cppguide.html#Function_Names

It doesn't say that such methods HAVE to be inlined, does it?

No, but Chromium code is largely consistent about keeping a correspondence between hacker_case()-naming and inlining into the header.  I would prefer not to break that.  (The current direction, of deinlining and renaming it, seems better to me than deinlining and not renaming it.)

PK 

[Václav Brožek]

Thank you all for the helpful comments!

The main concern seems to be about whether the path accessor should stay inline or move out of line. The secondary concern is about the impact on naming. The biggest cost in the whole operation is the burden put on reviewers of the cascade of CLs because of renaming.

Inline vs. out-of-line

The 2 reasons for moving the accessor out of line are:

• [style] it now does a DCHECK in addition to producing the value, and
• [compilation time] it requires #include "base/logging.h" in the place of its definition (which is header if inline and .cc if out-of-line).

The current impact on compilation time seems negligible (*). 

Style-wise, opinions differ on whether a simple accessor + DCHECK is still good to inline. Sample two arguments:

• Pro-inline: performance is only important in user builds, and DCHECKs are not present there.
• Against-inline: Growing the method will invite more growing without moving out-of-line in the future. 

The code (//base) OWNER suggested moving this out of line, and I found that reasonable, so that's what I went with.

Naming

It is a prevalent pattern in Chrome that hacker_case() methods are inline and CamelCase() are out of line. The naming seems to be dictated here by consistency. So I am weighing the impact on consistency to the reviewers' work on the cascade of CLs.

Cost

The reviewers of the cascade of CLs have to verify two things:

• (1) That all the places with path() replaced by GetPath() compile and are properly formatted.
• (2) That all the changes to tests to fix use of uninitialised ScopedTempDir are OK.

Note that (2) would be also needed without renaming, and (1) is checked by compilators and ensured by clang-format. And I make sure to highlight (2) in the reviews and point out the mechanical nature of (1). Keeping the style consistency should therefore be a reasonable gain for the lowered cost.

Cheers,

Vaclav

(*) Full details of the simple measurement: On my Z620+goma, I did three runs of a clean complete release build with DCHECKs enabled: out-of-line, inline and out-of-line again. The timings I got for out-of-line were:

real    8m36.621s, 4m36.506s

user    21m25.947s, 22m6.458s

sys     7m20.215s, 7m39.631s

For inline I got

real    5m5.172s

user    22m10.033s

sys     7m34.140s

IIUC, the important one is "user". While out-of-line performs slightly better in both instances, the difference between the two out-of-line runs is bigger than between inline and out-of-line, so the inline vs. out-of-line difference is likely statistically unimportant.

[James Cook]

I wonder what the compile time is like without goma.

--

[Peter Kasting]

This is a very well-written reply, thank you!

On Fri, Sep 9, 2016 at 3:00 AM, Václav Brožek <va...@chromium.org> wrote:

Inline vs. out-of-line

The current impact on compilation time seems negligible (*).

Cool, thanks for checking.

 

Style-wise, opinions differ on whether a simple accessor + DCHECK is still good to inline. Sample two arguments:

• Pro-inline: performance is only important in user builds, and DCHECKs are not present there.
• Against-inline: Growing the method will invite more growing without moving out-of-line in the future. 

The code (//base) OWNER suggested moving this out of line, and I found that reasonable, so that's what I went with.

Since this is a case where the Google style guide does not mandate one behavior, I defer to said OWNER(s).

Cost

The reviewers of the cascade of CLs have to verify two things:

• (1) That all the places with path() replaced by GetPath() compile and are properly formatted.
• (2) That all the changes to tests to fix use of uninitialised ScopedTempDir are OK.

Note that (2) would be also needed without renaming, and (1) is checked by compilators and ensured by clang-format. And I make sure to highlight (2) in the reviews and point out the mechanical nature of (1). Keeping the style consistency should therefore be a reasonable gain for the lowered cost.

If you want all the callers explicitly audited to ensure (2) (i.e. you're not willing to just add the DCHECK and "let test failures sort it out"), then there indeed seems less advantage to avoiding the rename CLs.

PK

[Matthew Menke]

+1 to this.  We have some header files in net which include base/logging.h, and whenever we remove dependencies on the things declared in those header files, we always find ourselves digging into random files outside of net/ to add missing dependencies.

[Bruce]

> The current impact on compilation time seems negligible

While I suspect that this is true, we have no evidence to support it. All we can say for sure is that the compilation difference is less than the measurement noise, but since the measurement noise is enormous this is not a strong claim. The compile slowdown could be 10%, or it could be 0.001%.

As James suggested, accurate measurements would probably require disabling goma and then doing a dozen builds overnight on an otherwise idle machine in hopes of lowering the noise level. I don't think it's worth it for this change, but I keep thinking I should do that in order to better understand why our build times keep growing.

[Václav Brožek]

Hi Bruce,

While I agree that my 3-pass trial was not a proper measurement, let me emphasise that the solution chosen for http://crbug.com/640599 does not add the #include "base/logging.h" to any header files. Therefore it is unlikely to impact compilation time and none of the alternatives was likely to have better compilation time (unless one expects more #includes to speed up compilation).

The compilation time was considered here as a piece of motivation for the chosen solution (on the hypothesis that it will improve it). My conclusion was that even assuming the solution won't have effect on the compilation time, it was still worth doing.

Cheers,
Vaclav