Re: Profile mode maintenance patch
On 04/10/14 22:54 +0200, François Dumont wrote:
On 23/09/2014 13:27, Jonathan Wakely wrote:
Yes, OK for trunk - thanks very much.
Hi
There was in fact one last test failing, ext/profile/mh.cc, a
profile mode specific test. It must have been failing for quite a
while since malloc hooks has been deprecated.
I don't understand why that would make it fail, deprecated doesn't
mean it doesn't work.
However, the declaration of __malloc_initialize_hook has changed at
some point between glibc 2.11 and 2.18, so the test doesn't compile
for me on Fedora 20.
Secondly, the test is useless if it isn't actually run, so the
{ dg-do compile } must be removed.
Finally, the test returns 1, so even if it is executed it causes a
FAIL.
What a mess.
It is normally testing
the profile mode protection against recursion if memory allocation
functions are redefined. It was based on malloc but we use in fact new
operator. So I rewrite the test using new/delete operators.
OK.
This new test version is attached, I removed those 2 lines at the
beginning:
// { dg-do compile { target *-*-linux* *-*-gnu* } }
// { dg-xfail-if { uclibc } { * } { } }
I think that this test can now be executed and see no reason why
it should fail with uclibc. Do you confirm ?
Yes, the test should be portable now, and should actually PASS.
The name of the test doesn't make much sense now though, maybe rename
it from mh.cc to replace_new.cc or something.
I attached the full patch again. I also remove useless virtual
destructor or methods, no need for polymorphism.
OK.
(I still think we should just remove the whole Profile Mode ...)
Re: Profile mode maintenance patch
On 23/09/2014 13:27, Jonathan Wakely wrote:
Yes, OK for trunk - thanks very much.
Hi
There was in fact one last test failing, ext/profile/mh.cc, a
profile mode specific test. It must have been failing for quite a while
since malloc hooks has been deprecated. It is normally testing the
profile mode protection against recursion if memory allocation functions
are redefined. It was based on malloc but we use in fact new operator.
So I rewrite the test using new/delete operators.
This new test version is attached, I removed those 2 lines at the
beginning:
// { dg-do compile { target *-*-linux* *-*-gnu* } }
// { dg-xfail-if { uclibc } { * } { } }
I think that this test can now be executed and see no reason why it
should fail with uclibc. Do you confirm ?
I attached the full patch again. I also remove useless virtual
destructor or methods, no need for polymorphism.
François
profile.patch.bz2
Description: application/bzip
// -*- C++ -*-
// Copyright (C) 2006-2014 Free Software Foundation, Inc.
//
// This file is part of the GNU ISO C++ Library. This library is free
// software; you can redistribute it and/or modify it under the
// terms of the GNU General Public License as published by the
// Free Software Foundation; either version 3, or (at your option)
// any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License along
// with this library; see the file COPYING3. If not see
// http://www.gnu.org/licenses/.
// { dg-require-profile-mode }
#include vector
using std::vector;
void* operator new(std::size_t size) throw(std::bad_alloc)
{
void* p = std::malloc(size);
if (!p)
throw std::bad_alloc();
return p;
}
void* operator new (std::size_t size, const std::nothrow_t) throw()
{
// With _GLIBCXX_PROFILE, the instrumentation of the vector constructor
// will call back into this new operator.
vectorint v;
return std::malloc(size);
}
void operator delete(void* p) throw()
{
if (p)
std::free(p);
}
int
main()
{
vectorint v;
return 0;
}
Re: Profile mode maintenance patch
On 21/09/14 23:29 +0200, François Dumont wrote: With all those modifications I have been able to run all testsuite in profile mode with success. I've looked over the patch and it looks fine. I don't know the details of the Profile Mode, so if you're happy that these changes are an improvement and all tests pass then that's good enough for me. Ok to commit ? Yes, OK for trunk - thanks very much.
Re: Profile mode maintenance patch
On 23/09/2014 13:27, Jonathan Wakely wrote:
On 21/09/14 23:29 +0200, François Dumont wrote:
With all those modifications I have been able to run all testsuite
in profile mode with success.
I've looked over the patch and it looks fine.
I don't know the details of the Profile Mode, so if you're happy that
these changes are an improvement and all tests pass then that's good
enough for me.
Ok to commit ?
Yes, OK for trunk - thanks very much.
Ok but could you just let me know what you think of this method:
templatetypename __object_info, typename __stack_info
__object_info*
__trace_base__object_info, __stack_info::
__add_object(const __object_info __info)
{
if (__max_mem() != 0 __objects_byte_size = __max_mem())
{
delete __info.__stack();
return 0;
}
__object_info* __ret = new(std::nothrow) __object_info(__info);
if (!__ret)
{
delete __info.__stack();
return 0;
}
__gnu_cxx::__atomic_add(__objects_byte_size, sizeof(__object_info));
return __ret;
}
This method can be called from several threads. I check condition
accessing __object_byte_size and then update it with an atomic operation
to make sure it stays consistent. Does it look ok to you too ?
François
Re: Profile mode maintenance patch
On 23 September 2014 22:19, François Dumont wrote:
On 23/09/2014 13:27, Jonathan Wakely wrote:
On 21/09/14 23:29 +0200, François Dumont wrote:
With all those modifications I have been able to run all testsuite in
profile mode with success.
I've looked over the patch and it looks fine.
I don't know the details of the Profile Mode, so if you're happy that
these changes are an improvement and all tests pass then that's good
enough for me.
Ok to commit ?
Yes, OK for trunk - thanks very much.
Ok but could you just let me know what you think of this method:
templatetypename __object_info, typename __stack_info
__object_info*
__trace_base__object_info, __stack_info::
__add_object(const __object_info __info)
{
if (__max_mem() != 0 __objects_byte_size = __max_mem())
{
delete __info.__stack();
return 0;
}
__object_info* __ret = new(std::nothrow) __object_info(__info);
if (!__ret)
{
delete __info.__stack();
return 0;
}
__gnu_cxx::__atomic_add(__objects_byte_size, sizeof(__object_info));
return __ret;
}
This method can be called from several threads. I check condition accessing
__object_byte_size and then update it with an atomic operation to make sure
it stays consistent. Does it look ok to you too ?
It can result in a data race if the non-atomic access happens
concurrently with the atomic update.
The read should be an atomic load to solve that, but we don't have a
dispatch function in ext/atomicity.h for atomic loads.
Profile mode maintenance patch
Hi Here is the promise major patch for the profile mode. Here are the most important modifications. Now instance of profiling structs are kept as pointers in the containers themselves. It has an impact on the container ABI but it greatly enhance performances as we do not need to move through a search in an unordered container which also imply a lock during this research. I have even been able to remove those unordered containers eventually just keeping a counter of allocated bytes to know if we should stop creating new profiling structs. I get rid of the re-entrancy mechanism. The only reason for it was a potential hook in the memory allocator potentially creating new profiling structs and so long forever. I prefer to put it just where it is necessary that is to say when we first allocate memory for profiling which is then we create the back-trace. I wonder if we shouldn't emit a #error when trying to activate profiling mode without backtrace feature cause in this case we simply won't collect anything. I finalize ordered to unordered profiling by adding the missing __iterator_tracker on the ordered containers (map, multimap, set, multiset). I clean all useless stuff like __stack_info_base class. I fixed many memory leak and added a cleanup at exit of the application. Profiling of containers is reseted as soon as one of the following operations occur: copy assignment, move assignment, initialization from an initialization list, clear. I have added usage of atomic operations to maintain some counters that might be updated from different threads. Do not hesitate to review those closely. Especially __objects_byte_size which I am using in profiler_trace.h without atomic operation, is it fine ? With all those modifications I have been able to run all testsuite in profile mode with success. Ok to commit ? François profile.patch.bz2 Description: application/bzip
Re: profile mode maintenance patch
Hi,
On 05/24/2014 11:39 PM, Jonathan Wakely wrote:
On 24 May 2014 22:10, François Dumont wrote:
Done but I forgot to fix the spelling. I will fix it in the future patch.
No problem, it's clear what the comment means.
I'm committing the below.
Paolo.
/
2014-05-25 Paolo Carlini paolo.carl...@oracle.com
* include/profile/map.h: Fix typo in comment; minor formatting fix.
* include/profile/multimap.h: Likewise.
* include/profile/set.h: Likewise.
* include/profile/multiset.h: Likewise.
Index: include/profile/map.h
===
--- include/profile/map.h (revision 210911)
+++ include/profile/map.h (working copy)
@@ -451,15 +451,15 @@
* operations have equivalent insertion cost so we do not update metrics
* about it.
* Note that to find out if hint has been used is libstdc++
- * implementation dependant.
+ * implementation dependent.
*/
bool
_M_hint_used(const_iterator __hint, iterator __res)
{
- return (__hint == __res ||
- (__hint == this-end() ++__res == this-end()) ||
- (__hint != this-end() (++__hint == __res ||
- ++__res == --__hint)));
+ return (__hint == __res
+ || (__hint == this-end() ++__res == this-end())
+ || (__hint != this-end() (++__hint == __res
+ || ++__res == --__hint)));
}
};
Index: include/profile/multimap.h
===
--- include/profile/multimap.h (revision 210911)
+++ include/profile/multimap.h (working copy)
@@ -419,15 +419,15 @@
* operations have equivalent insertion cost so we do not update metrics
* about it.
* Note that to find out if hint has been used is libstdc++
- * implementation dependant.
+ * implementation dependent.
*/
bool
_M_hint_used(const_iterator __hint, iterator __res)
{
- return (__hint == __res ||
- (__hint == this-end() ++__res == this-end()) ||
- (__hint != this-end() (++__hint == __res ||
- ++__res == --__hint)));
+ return (__hint == __res
+ || (__hint == this-end() ++__res == this-end())
+ || (__hint != this-end() (++__hint == __res
+ || ++__res == --__hint)));
}
};
Index: include/profile/multiset.h
===
--- include/profile/multiset.h (revision 210911)
+++ include/profile/multiset.h (working copy)
@@ -413,15 +413,15 @@
* operations have equivalent insertion cost so we do not update metrics
* about it.
* Note that to find out if hint has been used is libstdc++
- * implementation dependant.
+ * implementation dependent.
*/
bool
_M_hint_used(const_iterator __hint, iterator __res)
{
- return (__hint == __res ||
- (__hint == this-end() ++__res == this-end()) ||
- (__hint != this-end() (++__hint == __res ||
- ++__res == --__hint)));
+ return (__hint == __res
+ || (__hint == this-end() ++__res == this-end())
+ || (__hint != this-end() (++__hint == __res
+ || ++__res == --__hint)));
}
};
Index: include/profile/set.h
===
--- include/profile/set.h (revision 210911)
+++ include/profile/set.h (working copy)
@@ -399,15 +399,15 @@
* operations have equivalent insertion cost so we do not update metrics
* about it.
* Note that to find out if hint has been used is libstdc++
- * implementation dependant.
+ * implementation dependent.
*/
bool
_M_hint_used(const_iterator __hint, iterator __res)
{
- return (__hint == __res ||
- (__hint == this-end() ++__res == this-end()) ||
- (__hint != this-end() (++__hint == __res ||
- ++__res == --__hint)));
+ return (__hint == __res
+ || (__hint == this-end() ++__res == this-end())
+ || (__hint != this-end() (++__hint == __res
+ || ++__res == --__hint)));
}
};
Re: profile mode maintenance patch
On 12/05/14 22:14 +0200, François Dumont wrote: Hi Here is a maintenance patch for profile mode. It does: - Use inheritance to limit duplication of code in constructors to register for the different profiling mode diagnostics data structure. - Remove many code keeping only instrumented methods or methods that where the container type itself appears in the signature.. - Extend the map to unordered_map to all ordered containers. And of course code cleanup and usage of default implementation for special methods as much as possible. Regarding Makefile.in I miss last time. I moved to a new system lately, a Ubuntu based one, and still need to find out what version of automake/autoreconf I need to install. For the moment I have updated Makefile.in manually. This is OK (I'm in favour of any change that reduces the amount of code in the Profile Mode :) Please correct a minor spelling mistake (in two places) before committing: + /** If hint is used we consider that the map and unordered_map + * operations have equivalent insertion cost so we do not update metrics + * about it. + * Note that to find out if hint has been used is libstdc++ + * implementation dependant. s/dependant/dependent/ Thanks!
Re: profile mode maintenance patch
On 24/05/2014 13:33, Jonathan Wakely wrote: On 12/05/14 22:14 +0200, François Dumont wrote: Hi Here is a maintenance patch for profile mode. It does: - Use inheritance to limit duplication of code in constructors to register for the different profiling mode diagnostics data structure. - Remove many code keeping only instrumented methods or methods that where the container type itself appears in the signature.. - Extend the map to unordered_map to all ordered containers. And of course code cleanup and usage of default implementation for special methods as much as possible. Regarding Makefile.in I miss last time. I moved to a new system lately, a Ubuntu based one, and still need to find out what version of automake/autoreconf I need to install. For the moment I have updated Makefile.in manually. This is OK (I'm in favour of any change that reduces the amount of code in the Profile Mode :) Please correct a minor spelling mistake (in two places) before committing: + /** If hint is used we consider that the map and unordered_map + * operations have equivalent insertion cost so we do not update metrics + * about it. + * Note that to find out if hint has been used is libstdc++ + * implementation dependant. s/dependant/dependent/ Thanks! Done but I forgot to fix the spelling. I will fix it in the future patch. François
Re: profile mode maintenance patch
On 24 May 2014 22:10, François Dumont wrote: Done but I forgot to fix the spelling. I will fix it in the future patch. No problem, it's clear what the comment means.
Re: profile mode maintenance patch
Hi, On 05/12/2014 10:14 PM, François Dumont wrote: Regarding Makefile.in I miss last time. I moved to a new system lately, a Ubuntu based one, and still need to find out what version of automake/autoreconf I need to install. For the moment I have updated Makefile.in manually. Isn't this clear enough http://gcc.gnu.org/install/prerequisites.html ? Paolo.
Re: profile mode maintenance patch
On 12/05/2014 22:42, Paolo Carlini wrote: Hi, On 05/12/2014 10:14 PM, François Dumont wrote: Regarding Makefile.in I miss last time. I moved to a new system lately, a Ubuntu based one, and still need to find out what version of automake/autoreconf I need to install. For the moment I have updated Makefile.in manually. Isn't this clear enough http://gcc.gnu.org/install/prerequisites.html ? Paolo. Perfectly clear, thanks. François
