Profile mode maintenance patch

[François Dumont]

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

[Jonathan Wakely]

Post by François Dumont
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.

Post by François Dumont
Ok to commit ?

Yes, OK for trunk - thanks very much.

[François Dumont]

Post by Jonathan Wakely

Post by François Dumont
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.

Post by François Dumont
Ok to commit ?

Yes, OK for trunk - thanks very much.

Ok but could you just let me know what you think of this method:

template<typename __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

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely

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.

template<typename __object_info, typename __stack_info>
__object_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.

[François Dumont]

Post by Jonathan Wakely
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

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely
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.

Post by François Dumont
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.

Post by François Dumont
This new test version is attached, I removed those 2 lines at the
// { 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.

Post by François Dumont
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 ...)