Re: [profile-stdlib] PATCH: Add three diagnostics and implement cost factor parameterization

[Silvius Rus <rus at google dot com>]

On Wed, Dec 9, 2009 at 7:19 PM, Benjamin Kosnik <bkoz@redhat.com> wrote:
>
>> 1) New diagnostics
>> ? - list-to-vector; if data is inserted at the end of the list and
>> iteration operation is common, vector is better.
>> ? - list-to-slist; slist is more likely to fit in a cache line due to
>> less memory usage.
>> ? - list-to-set; if a find operation, 'std::find', is frequently used
>> in the list, set is better.
>
> Please add docs for this in the section "Diagnostics."
>
>> 2) Cost factor parameterization; constant factors of the cost models
>> for diagnostics can be read from a file, "libstdcxx-profile.conf" or
>> environment variables.
>
> Please add docs for this in the section "Using the Profile Mode."
>
> For these changes, please try to use the include guard
> _GLIBCXX_PROFILE_(filename)
> form in a consistent manner.
>
> I'll change the rest of the impl files to do this. And do some cleanup
> on the existing impl files for formatting issues.
>
> Also, the date on copyright years for files created this year should be
> 2009, not 2008.
>
> Your patch did not apply cleanly to trunk. And needs a ChangeLog entry.
>
> Change implementation namespace to match trunk.
>
> from:
> namespace __cxxprof_impl
> to
> namespace __gnu_profile
>
> Change copyright year to 2009
>
> As an aside, which has nothing to do with your patch since you're just
> using the pattern already established, the use of __settings<0> and
> __tables<0> is a bit maddening. I assume this is just a linkage
> convenience, to avoid the need for a linked (and thus exported) symbol.
>
> profiler_trace.h
> profiler_state.h
>
> are the big users.
>
> See:
> http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00890.html
>
> And look at the
>
> * include/profile/impl/profiler.h:
>
> Changes.
>
> Can the tricks used with __reentrance_guard work for __settings and
> __tables?
>
>> There is an issue related to thel instrumentation of 'std::find'.
>> It seems that 'bits/stl_algo.h' was modified for the parallel mode so
>> that its functions declared in the namespace of '_GLIBCXX_STD_P' can
>> be used instead of original functions in the release mode.
>> Currently, I did the same thing, conditional compilation stuff, to
>> hide the original 'find' function under its wrapper function.
>>
>> This inevitably causes a modification to the original
>> 'bits/stl_algo.h'. Alternatively, it is possible to duplicate the
>> 'bits/stl_algo.h' in 'profile/stl_algo.h' and modify the copy to
>> avoid any intrusion from the profile mode to the release mode.
>> It would be appreciated if you come up with a better idea to deal
>> with this.
>
> Can your remove the parts of the patch that deal with "std::find" for
> the time being and just submit the rest?
>
> This part doesn't look right to me. You'd have to change
> bits/algorithmfwd.h too, for starters. And in that file you'd just
> make std::find conditional on both Parallel and Profile macros.
>
> Thanks,
> Benjamin
>

Hello Benjamin,

I will work on this ASAP.  I'll prepare a trunk patch with:
- cost factor parameters
- list-to-vector and list-to-slist
- minor bug fixes currently on the branch but not on trunk

I will leave out the diagnostic that would touch "algorithm", as recommended.

Thank you for the review!
Silvius