Skip oversized logs when persisting logs instead of discarding them.

Skip oversized logs when persisting logs instead of discarding them.

BUG=372911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284826

[Alexei Svitkine (slow), 2014-07-15 18:53:00]

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:89: max_log_size_(max_log_size ?
max_log_size : static_cast<size_t>(-1)),
Can you use SIZE_MAX instead of casting -1 to a size_t? Also, please make the
check != 0, since it's not a bool or pointer.

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:160: // Keep the most recent logs which are
smaller than max_log_size_.
Nit: Surround variable names in |'s.

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:167: size_t log_size =
list_[start-1].compressed_log_data.length();
Nit: spaces around the minus.

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:172: // Oversized logs won't be persisted,
so don't count them.
I don't think the piece of code is needed, since the loop below would already
ignore them. Plus, by avoiding this and the early return below, we won't lose
the histogram for these large logs.

[Steven Holte, 2014-07-15 20:23:03]

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:89: max_log_size_(max_log_size ?
max_log_size : static_cast<size_t>(-1)),
On 2014/07/15 18:52:59, Alexei Svitkine wrote:
> Can you use SIZE_MAX instead of casting -1 to a size_t? Also, please make the
> check != 0, since it's not a bool or pointer.

Done.

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:160: // Keep the most recent logs which are
smaller than max_log_size_.
On 2014/07/15 18:52:59, Alexei Svitkine wrote:
> Nit: Surround variable names in |'s.

Done.

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:167: size_t log_size =
list_[start-1].compressed_log_data.length();
On 2014/07/15 18:52:59, Alexei Svitkine wrote:
> Nit: spaces around the minus.

Done.

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:172: // Oversized logs won't be persisted,
so don't count them.
On 2014/07/15 18:52:59, Alexei Svitkine wrote:
> I don't think the piece of code is needed, since the loop below would already
> ignore them. Plus, by avoiding this and the early return below, we won't lose
> the histogram for these large logs.

The second loop iterates in the reverse direction from where this one stops, and
we wouldn't go back as far if we don't skip these.

[Steven Holte, 2014-07-15 20:25:47]

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/397443004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:172: // Oversized logs won't be persisted,
so don't count them.
On 2014/07/15 20:23:03, Steven Holte wrote:
> On 2014/07/15 18:52:59, Alexei Svitkine wrote:
> > I don't think the piece of code is needed, since the loop below would
already
> > ignore them. Plus, by avoiding this and the early return below, we won't
lose
> > the histogram for these large logs.
> 
> The second loop iterates in the reverse direction from where this one stops,
and
> we wouldn't go back as far if we don't skip these.

Also, I've now removed the early return below, since it is unnecessary, so the
histogram counts won't go missing.

[Alexei Svitkine (slow), 2014-07-16 15:35:24]

LGTM

[Steven Holte, 2014-07-16 16:55:06]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-07-16 16:58:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/holte@chromium.org/397443004/80001

[commit-bot: I haz the power, 2014-07-16 21:20:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-16 22:03:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 22:03:22]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[Alexei Svitkine (slow), 2014-07-22 20:59:37]

On 2014/07/16 22:03:22, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

Try including <stdint.h> for SIZE_MAX?

[Steven Holte, 2014-07-22 21:10:43]

On 2014/07/22 20:59:37, Alexei Svitkine wrote:
> On 2014/07/16 22:03:22, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_gn_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
> 
> Try including <stdint.h> for SIZE_MAX?

grepping for SIZE_MAX turns up 6 results outside of third_party, where libraries
define it themselves.

base/strings/string_number_conversions_unittest.cc:#if SIZE_MAX > 4294967295U
base/strings/string_number_conversions_unittest.cc:#if SIZE_MAX > 4294967295U
components/domain_reliability/config.cc:// Make sure stdint.h includes SIZE_MAX.
(See C89, p259, footnote 221.)
components/domain_reliability/config.cc:const size_t
DomainReliabilityConfig::kInvalidResourceIndex = SIZE_MAX;
pdf/chunk_stream.cc:  if (SIZE_MAX - size < offset)
pdf/chunk_stream.cc:  if (SIZE_MAX - size < offset)

Notably in components/domain_reliability/config.cc:

// Make sure stdint.h includes SIZE_MAX. (See C89, p259, footnote 221.)
#ifndef __STDC_LIMIT_MACROS
#define __STDC_LIMIT_MACROS 1
#endif

Stack overflow says it's not part of the C++ standard:
http://stackoverflow.com/questions/986426/what-do-stdc-limit-macros-and-stdc-...

And there about 37 uses of static_cast<size_t>(-1) outside third_party, so I
think it's probably better to stick to that.

[Steven Holte, 2014-07-22 21:32:20]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-07-22 21:33:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/holte@chromium.org/397443004/100001

[commit-bot: I haz the power, 2014-07-23 01:39:42]

Change committed as 284826