GTTF: Enable glibcxx debug mode for Debug builds by default.

GTTF: Enable glibcxx debug mode for Debug builds by default.

This will help us catch problems earlier.

This CL actually fixes several problems detected by this mode.

BUG=65151, 267870, 267891
TEST=none
R=avi@chromium.org, jar@chromium.org, thakis@chromium.org, thestig@chromium.org, yoz@chromium.org

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

[Paweł Hajdan Jr., 2013-08-05 18:33:01]

Lei, please take an initial look.

If it seems OK, I'll then add reviewers for the specific non-trivial fixes.

[Lei Zhang, 2013-08-05 20:20:10]

lgtm

[Paweł Hajdan Jr., 2013-11-05 01:10:19]

+Matt for chrome/browser/safe_browsing/prefix_set_unittest.cc
+Dale for media/filters/source_buffer_stream.cc
+Jim for base/ (trivial changes)

[DaleCurtis, 2013-11-05 01:12:05]

over to acolwell since I'm not sure what the original code was trying to check.

[mattm, 2013-11-05 01:17:30]

passing prefix_set_unittest to Scott

[Scott Hess - ex-Googler, 2013-11-05 03:04:58]

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
File chrome/browser/safe_browsing/prefix_set_unittest.cc (right):

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
chrome/browser/safe_browsing/prefix_set_unittest.cc:33: for (size_t i = 0; i <
5000; ++i) {
In theory, this many values should be able to spread out in the 32-bit space
with only singleton runs.  What's the rationale for doing this?  Is it just the
cost of push_back?  If things were inserted in bulk would that help?

[Paweł Hajdan Jr., 2013-11-05 03:10:05]

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
File chrome/browser/safe_browsing/prefix_set_unittest.cc (right):

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
chrome/browser/safe_browsing/prefix_set_unittest.cc:33: for (size_t i = 0; i <
5000; ++i) {
On 2013/11/05 03:04:58, shess wrote:
> In theory, this many values should be able to spread out in the 32-bit space
> with only singleton runs.  What's the rationale for doing this?  Is it just
the
> cost of push_back?  If things were inserted in bulk would that help?

I'm not sure now - it's definitely about the speed.

I actually suspect quadratic complexity somewhere when GLIBCXX_DEBUG is used.

How would you insert these random values in bulk? If there is some specific
alternative you have in mind I'm happy to try it. Note that it's likely to need
another container, which would run into same problem.

[Scott Hess - ex-Googler, 2013-11-05 03:17:23]

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
File chrome/browser/safe_browsing/prefix_set_unittest.cc (right):

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
chrome/browser/safe_browsing/prefix_set_unittest.cc:33: for (size_t i = 0; i <
5000; ++i) {
On 2013/11/05 03:10:06, Paweł Hajdan Jr. wrote:
> On 2013/11/05 03:04:58, shess wrote:
> > In theory, this many values should be able to spread out in the 32-bit space
> > with only singleton runs.  What's the rationale for doing this?  Is it just
> the
> > cost of push_back?  If things were inserted in bulk would that help?
> 
> I'm not sure now - it's definitely about the speed.
> 
> I actually suspect quadratic complexity somewhere when GLIBCXX_DEBUG is used.
> 
> How would you insert these random values in bulk? If there is some specific
> alternative you have in mind I'm happy to try it. Note that it's likely to
need
> another container, which would run into same problem.

for (size_t i = 0; i < 500; ++i) {
  SBPrefix a[100];
  for (size_t j = 0; j < 100; ++j) {
    a[j] = static_cast<SBPrefix>(base::RandUint64());
  }
  shared_prefixes_.insert(shared_prefixes_.end(), &a[0], &a[100]);
}

though if the quadratic part is in the users of this rather than the creation,
doomed.

The data structure delta-encodes items closer together than 2^16.  So with 50000
items, there's a fair number of runs of such items.  With 5000, less so, though
I'd expect it to be non-zero.  Unfortunately, I don't have the math to calculate
this off-the-cuff, but I'll think on where the knee might be.

[Paweł Hajdan Jr., 2013-11-05 22:42:25]

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
File chrome/browser/safe_browsing/prefix_set_unittest.cc (right):

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
chrome/browser/safe_browsing/prefix_set_unittest.cc:33: for (size_t i = 0; i <
5000; ++i) {
On 2013/11/05 03:17:24, shess wrote:
> though if the quadratic part is in the users of this rather than the creation,
> doomed.

I've tried it and it doesn't help in a meaningful way (I've tried with 50000 of
course - for 5000 the difference is similarly small).

Numbers are 495 s for original non-batch version and 490 s for batch version.

Total test run time is 973 s and 983 s respectively (not sure why batch would be
slower there - maybe more runs would flesh out any differences, but then I don't
think we get any real speed-up from this).

[Paweł Hajdan Jr., 2013-11-05 22:45:59]

+Avi for content OWNERS
+Nico for ui OWNERS
+Yoyo for extensions OWNERS

[Yoyo Zhou, 2013-11-05 22:50:10]

LGTM for extensions

[Scott Hess - ex-Googler, 2013-11-06 00:47:13]

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
File chrome/browser/safe_browsing/prefix_set_unittest.cc (right):

https://chromiumcodereview.appspot.com/22243002/diff/30001/chrome/browser/saf...
chrome/browser/safe_browsing/prefix_set_unittest.cc:33: for (size_t i = 0; i <
5000; ++i) {
On 2013/11/05 22:42:26, Paweł Hajdan Jr. wrote:
> On 2013/11/05 03:17:24, shess wrote:
> > though if the quadratic part is in the users of this rather than the
creation,
> > doomed.
> 
> I've tried it and it doesn't help in a meaningful way (I've tried with 50000
of
> course - for 5000 the difference is similarly small).
> 
> Numbers are 495 s for original non-batch version and 490 s for batch version.
> 
> Total test run time is 973 s and 983 s respectively (not sure why batch would
be
> slower there - maybe more runs would flesh out any differences, but then I
don't
> think we get any real speed-up from this).

495 _seconds_?  Normally the entire PrefixSetTest.* group runs in 843
_milliseconds_.  This doesn't make much sense, and I'm not sure I'm happy to
make this kind of change without a stronger rationale.  As I noted the
difference between 50k and 5k is pretty material to this data structure.

Note that shared_prefixes_ is static, and should only be initialized once.  So
it somewhat makes sense not to see much gain from initialization, I guess.  Do
you have a better breakdown of where the slowdown comes from?  Perhaps there's a
specific case which is hitting it.  This doesn't seem like a very extreme
structure to me, WRT Chromium in general.

[Nico, 2013-11-06 00:49:34]

I think Jochen enabled or tried to enable debug iterators for msvs (?) which
also had O(n^2) issues. I don't remember what the conclusion from that was.

[jochen (gone - plz use gerrit), 2013-11-06 11:01:35]

On 2013/11/06 00:49:34, Nico (ooo until Nov 12) wrote:
> I think Jochen enabled or tried to enable debug iterators for msvs (?) which
> also had O(n^2) issues. I don't remember what the conclusion from that was.

I tried to disable them.

People didn't like this, and instead we decided to suck it up and bumped the
timeouts for windows debug bots

[Nico, 2013-11-06 21:30:40]

If windows has them turned on, I think it makes sense to try and match this, so
lgtm. If things slow down too much, we can reconsider.

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi#newcod...
build/common.gypi:2648: 'cflags_cc+': ['-g'],
Shouldn't this (-g) be on for Debug already? Do you need this?

[Lei Zhang, 2013-11-06 21:37:30]

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi#newcod...
build/common.gypi:2648: 'cflags_cc+': ['-g'],
On 2013/11/06 21:30:42, Nico (ooo until Nov 12) wrote:
> Shouldn't this (-g) be on for Debug already? Do you need this?

Probably not, looks like it's just copy + pasted from the old location which is
outside the the debug block.

[Nico, 2013-11-06 22:07:45]

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi#newcod...
build/common.gypi:2648: 'cflags_cc+': ['-g'],
On 2013/11/06 21:37:31, Lei Zhang wrote:
> On 2013/11/06 21:30:42, Nico (ooo until Nov 12) wrote:
> > Shouldn't this (-g) be on for Debug already? Do you need this?
> 
> Probably not, looks like it's just copy + pasted from the old location which
is
> outside the the debug block.

Then please remove this before landing :-)

Thanks for working on this!

[Paweł Hajdan Jr., 2013-11-07 21:05:44]

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/22243002/diff/170001/build/common.gypi#newcod...
build/common.gypi:2648: 'cflags_cc+': ['-g'],
On 2013/11/06 22:07:47, Nico (ooo until Nov 12) wrote:
> On 2013/11/06 21:37:31, Lei Zhang wrote:
> > On 2013/11/06 21:30:42, Nico (ooo until Nov 12) wrote:
> > > Shouldn't this (-g) be on for Debug already? Do you need this?
> > 
> > Probably not, looks like it's just copy + pasted from the old location which
> is
> > outside the the debug block.
> 
> Then please remove this before landing :-)
> 
> Thanks for working on this!

Done.

[jar (doing other things), 2013-12-09 23:26:54]

(miniscule) changes in patch set 9 in net/* LGTM

[Nico, 2013-12-09 23:58:13]

base lgtm as well

[Avi (use Gerrit), 2013-12-10 00:33:23]

Content LGTM

[Paweł Hajdan Jr., 2013-12-10 18:52:29]

Committed patchset #10 manually as r239811 (presubmit successful).