1049263 - Improve ccache stats reporting

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

bug 947256 added ccache stats, but it reports the stats whether ccache was used or not. It shouldn't display them unless ccache was used (which can be determined by the difference between the stats at the beginning and the end being 0).

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Do not report ccache stats unless ccache was used.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8469171 [details] [diff] [review]
Do not report ccache stats unless ccache was used.

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

::: python/mozbuild/mozbuild/controller/building.py
@@ +611,5 @@
>  
>          return '\n'.join(lines)
>  
> +    def __nonzero__(self):
> +        return sum(v for k, v in self._values.items() if k not in self.ABSOLUTE_KEYS) > 0

If a ccache -z is emitted during the build, some numbers are likely to end up being negative, and summing everything may or may not end up being positive, depending on the values. It would be better to check that all values are positive and non zero individually (with all())

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

> If a ccache -z is emitted during the build, some numbers are likely to end
> up being negative, and summing everything may or may not end up being
> positive, depending on the values. It would be better to check that all
> values are positive and non zero individually (with all())

I didn't consider ccache -z is emitted during the build. Thanks! 

However, I think check that all values are positive and non-zero is too strict. Some rare cases such as "couldn't find the compiler" may not advance during a normal build. Perhaps we could:

1) Check that all values are non-negative, and
2) At least one of the three values involving hit rate calculation, "cache hit (direct)", "cache hit (preprocessed), and "cache miss", should be positive.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Do not report ccache stats unless ccache was used. (v2)

* Improve __nonzero__ to address comments.
* Add negative stats test cases.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8469883 [details] [diff] [review]
Do not report ccache stats unless ccache was used. (v2)

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

::: python/mozbuild/mozbuild/controller/building.py
@@ +613,5 @@
>  
> +    def __nonzero__(self):
> +        hit_and_miss_keys = ('cache_hit_direct', 'cache_hit_preprocessed', 'cache_miss')
> +        return (all(v >= 0 for v in self._values.values()) and
> +                any(self._values[k] > 0 for k in hit_and_miss_keys))

Mmmmm come to think of it, all >= 0 and at least one > 0 might be better. You can end up building only one object, in which case only one of those values is going to be > 0.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Do not report ccache stats unless ccache was used. (v3)

Check all relative values >=0 and at least one relative values > 0.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Mike, thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0b72645733

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef0b72645733