This is the mail archive of the
mailing list for the libstdc++ project.
Re: profile mode: statistics for vector and map
- From: François Dumont <frs dot dumont at gmail dot com>
- To: libstdc++ at gcc dot gnu dot org
- Date: Thu, 09 May 2013 22:41:25 +0200
- Subject: Re: profile mode: statistics for vector and map
- References: <CAC82fA12kOENEb2NJa37zjU8LKc4+6HueNzRgvu_MqCsAz3zDw at mail dot gmail dot com> <51896A93 dot 8040507 at gmail dot com> <CAC82fA0G45UokXZyZfXyogst837p51_1+S5P2-Xke9BRg5=s1w at mail dot gmail dot com> <CAC82fA25-mNydLCd88DJkHMUYuNK8G1mKbEi+tGchsrVvCUKdg at mail dot gmail dot com>
On 05/08/2013 12:57 AM, Gedare Bloom wrote:
On Tue, May 7, 2013 at 6:16 PM, Gedare Bloom <firstname.lastname@example.org> wrote:
On Tue, May 7, 2013 at 4:56 PM, François Dumont <email@example.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;
- 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.