This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: profile mode: statistics for vector and map


On 05/08/2013 12:57 AM, Gedare Bloom wrote:
On Tue, May 7, 2013 at 6:16 PM, Gedare Bloom <gedare@rtems.org> wrote:
On Tue, May 7, 2013 at 4:56 PM, François Dumont <frs.dumont@gmail.com> wrote:
     IMHO we should adopt the same kind of design I have introduced for the
unordered containers that is to say a base class containing in its
constructor the call to the instance registration and in the destructor the
unregistration. This way special functions could be defaulted which will
simplify maintenance and we would never forget the registration like in the
vector constructor from initializer_list<value_type> one.

That's fine. If I have time I'll study your approach in more detail. I
had a look at the patch you sent, and your approach seems more robust.
The mechanisms here with macros and hand-inserted function call-outs
to wrap the "real" operations is easy to mess up.

If someone can rework the existing vector code for the
profiler_vector_size.h/profiler_container_size.h files using the
approach suggested, then I can probably generalize the approach to the
container statistics that I implemented without too much trouble. I
used the vector size as the basis for my design and implementation.

We can deal with this later for sure. I might do so after what I am working on at the moment.

Regarding the patch itself I took a closer look:
- I saw a FIXME comment, can it be fixed or removed before going in ? I also saw a XXX one, can it just be removed or explained ? - I have been surprised by the *_statistics_invalid_operator macros. It doesn't seem to be used anymore and I can't imagine what is an invalid operator in the profile mode so it should be removed.
- Sometimes code could be simplified like in:

+        size_type rv;
         __profcxx_map_to_unordered_map_find(this, size());
-        return _Base::count(__x);
+        __profcxx_map_statistics_find(this, size()); // FIXME
+        rv = _Base::count(__x);
+        return rv;

The temporary rv local variable doesn't seem to have any interest.
- There is a 80 columns limit, it looks like sometimes the lines you have added exceeded this limit. - We might need some documentation about the new statistics. Profile mode is well documented, check doc/xml/manual/profile_mode.xml for the place to add your doc.

Did you complete what is necessary to commit this kind of patch yourself ? I won't be able to give you the ok to commit it but people that can will ask you to have signed the necessary papers.

François


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]