Re: [patch] debug mode

[François Dumont <francois dot cppdevs at free dot fr>]

On 08/18/2010 03:11 PM, Paolo Carlini wrote:
> Hi,
>
> a few comments before my vacation days. In general the new patch looks
> pretty good, some comments below.
>    
> > Hello
> >
> >      Here is my new patch proposition:
> >
> > - I have tested it on x86_64 linux with make check. make debug-check
> > gives me the same failures with or without the patch, the failures
> > looks normal ones to me. For instance the synopsis.cc tests are all
> > failing because of ambiguity between std::vector and
> > std::__debug::vector, other tests are failing because of tests
> > involving the profile mode.
> >      
> Well, before your patch only the synopsis.cc testcases are failing,
> right? Please make sure we don't regress about that, even by way of
> undefs, or whatever other trick, thanks.
>    
No, there were AFAIR 22 failures, the same one with or without the 
tests. Do you mean that I should undef _GLIBCXX_DEBUG when the test 
don't work with it ? If so I could perhaps do so in an other patch 
dedicated to fixing the debug-check target.
> > - 80 columns
> >
> >      Sorry about that I looked for the number of chars and not the
> > number of columns, because of tabulations it is not equal. By the way,
> > are tabulations ok ? Most of projects I work on was forbidding tabs,
> > should I consider that tabs are 8 spaces long ? To fix this problem I
> > replace many __first, __last by respectively __f, __l, I hope it is fine.
> >      
> Certainly tabs are not forbidden, but the indentations must be right, 2
> chars wide each, essentially.
Really, with such a width indentation is not right in files I work on, 
with width 8 it looks better.
>   About __first and __last, I would suggest
> not changing those to __f and __l if at all possible: short names are
> always more dangerous vs BADNAMES on some less widespread machines.
>    
Ok, I will fix it an other way then
> > - Performance gain
> >
> >      This patch contains 4 new performance tests to challenge range
> > constructors of different containers instanciated with int type:
> >      deque_construct: deque instances built from a vector [begin, end[
> > range
> >      list_construct1: list instances built from a vector [begin, end[
> > range
> >      list_construct2: list instances built from a list [begin, end[ range
> >      vector_construct: vector instances built from a vector [begin,
> > end[ range
> >
> >      When run in release mode it gives the following results:
> > deque_construct.cc
> > 11r    9u    1s        0mem    0pf
> > list_construct1.cc                                               343r
> > 341u    0s        0mem    0pf
> > list_construct2.cc                                               346r
> > 346u    0s        0mem    0pf
> > vector_construct.cc
> > 8r    2u    5s        0mem    0pf
> >
> > deque and vector build are much faster because thanks to internal
> > optimizations it relies on memmove.
> >
> >      Once run with _GLIBCXX_DEBUG defined without the patch it gives:
> >
> > deque_construct.cc                                               529r
> > 523u    2s        0mem    0pf
> > list_construct1.cc                                               895r
> > 890u    1s        0mem    0pf
> > list_construct2.cc                                               884r
> > 879u    1s        0mem    0pf
> > vector_construct.cc                                              475r
> > 467u    6s        0mem    0pf
> >
> > All tests are taking much more time. list_construct1 and
> > list_construct2 because we need more time to check safe iterators on
> > each initialization, deque_construct and vector_construct because
> > usage of safe iterators disable completely usage of memmove and so
> > once again we have to check each safe iterators each time we increment
> > the safe iterator.
> >
> >      With the proposed patch we have:
> >
> > deque_construct.cc
> > 11r    7u    2s        0mem    0pf
> > list_construct1.cc                                               341r
> > 339u    0s        0mem    0pf
> > list_construct2.cc                                               865r
> > 861u    1s        0mem    0pf
> > vector_construct.cc
> > 9r    3u    4s        0mem    0pf
> >
> >      As soon as we detect random access safe iterator we check the
> > whole range in a single operation and there is no more overhead on
> > each iterator incrementation. Moreover for deque construct and vector
> > construct we restore usage of memmove. Only list_construct2 is not
> > impacted because we are not able to validate the iterator range in a
> > single operation so we still have the overhead of checking it each
> > time we increment the safe iterator internally.
> >      
> Looks good. But if we are talking about performance, those tests belong
> to testsuite/performance and should use the timing infrastructure
> already in place.
>    
> > - Modifications of functions.h
> >
> >      The only point that I am not fully comfortable with is that I had
> > to introduce a declaration of __check_singular in formatter.h in order
> > to have functions.h to depend on formatter.h and not the opposite as
> > it used to be when formatter.h was including debug.h. If you do not
> > like it I can introduce a new header to define __check_singular and
> > associated __check_singular_aux that will be overloaded in
> > safe_iterator.h. Except that I think modifications are correct and as
> > a matter of fact the test suite did not show any problem.
> >      
> Ok, let's give that a bit more testing (maybe normal make-check too, but
> *without* PCHs? Normally I do that when I change the header dependencies).
>
> Also, I see various testcases defining explicitly _GLIBCXX_DEBUG:
> normally we don't do that: check-debug takes care of testing the
> debug-mode behavior, uniformly. Please see if you can remove those
> explicit defines in the testcases.
>    
Those tests are checking that invalid code is correctly detected thanks 
to the debug mode so I force definition of _GLIBCXX_DEBUG. Is there some 
way to make those tests unsupported when run in normal mode ?
> Finally, the *cont_traits.h headers should also use a correct style, no
> open bracket at the end of a line, I would also recommend a minimum of
> "air" between the lines, and make up your mind, either you uglify (eg,
> _It), or not (eg, first). I would say not, for testcases.
>    
Ok
> Paolo.
>