Re: [PATCH, applied] Consistently use std::unique_ptr for private implementations (pimpl)

[Giuliano Procida <gprocida@google.com>]

I was benchmarking the kernel ABI use case. So it looks like perhaps the
more complex C++ ABIs really benefit from this change.

Matthias, I'm glad we found the performance improvement after all!


On Fri, 16 Jul 2021, 10:49 Matthias Männich, <maennich@google.com> wrote:

> On Fri, Jul 16, 2021 at 10:13 AM Dodji Seketeli <dodji@seketeli.org>
> wrote:
> >
> > Hello,
> >
> > Matthias Maennich <maennich@google.com> a écrit:
> >
> > > In the absence of non-refcounting smart pointers before C++11,
> > > std::shared_ptr was commonly used instead. Having bumped the standard
> to
> > > C++11, allows us to use std::unique_ptr consistently avoiding any costs
> > > involved with shared_ptr ref counting. Hence do that and add default
> > > virtual destructors where required.
> >
> > Thanks for doing this!
> >
> > So, after testing on an old AMD machine, here is the difference in the
> > time taken (as reported by the 'time' command) by "make check" between
> > the master branch without the patch, and with the patch.
> >
> > Without the patch:
> >
> >     real        3m41,260s
> >     user        10m44,672s
> >     sys 1m27,097s
> >
> > With the patch:
> >
> >     real        2m7,102s
> >     user        11m48,024s
> >     sys 1m18,589s
> >
> > Pretty neat isn't it? :-)
>
> Oh wow! That is indeed neat! Guiliano was benchmarking the change and
> came to an improvement there as well. The effect was not as strong,
> though:
>
> abidw: 1:43 or 1:47
> abidiff: 1:31
>
> after:
>
> abidw: 1:42 or 1:45
> abidiff: 1:30
>
> Cheers,
> Matthias
>
> >
> > So thanks a lot for that!
> >
> > >
> > >       * include/abg-comparison.h (diff_maps): use unique_ptr for priv_
> > >       (diff_context): Likewise.
> > >       (diff_traversable_base): Likewise.
> > >       (type_diff_base): Likewise.
> > >       (decl_diff_base): Likewise.
> > >       (distinct_diff): Likewise.
> > >       (var_diff): Likewise.
> > >       (pointer_diff): Likewise.
> > >       (reference_diff): Likewise.
> > >       (array_diff): Likewise.
> > >       (qualified_type_diff): Likewise.
> > >       (enum_diff): Likewise.
> > >       (class_or_union_diff): Likewise.
> > >       (class_diff): Likewise.
> > >       (base_diff): Likewise.
> > >       (scope_diff): Likewise.
> > >       (fn_parm_diff): Likewise.
> > >       (function_type_diff): Likewise.
> > >       (function_decl_diff): Likewise.
> > >       (typedef_diff): Likewise.
> > >       (translation_unit_diff): Likewise.
> > >       (diff_stats): Likewise.
> > >       (diff_node_visitor): Likewise.
> > >       * include/abg-corpus.h (corpus): Likewise.
> > >       (exported_decls_builder): Likewise.
> > >       (corpus_group): Likewise.
> > >       * include/abg-ini.h (property): Likewise.
> > >       (property_value): Likewise.
> > >       (string_property_value): Likewise.
> > >       (list_property_value): Likewise.
> > >       (tuple_property_value): Likewise.
> > >       (simple_property): Likewise.
> > >       (list_property): Likewise.
> > >       (tuple_property): Likewise.
> > >       (config): Likewise.
> > >       (section): Likewise.
> > >       (function_call_expr): Likewise.
> > >       * include/abg-interned-str.h (interned_string_pool): Likewise.
> > >       * include/abg-ir.h (environment): Likewise.
> > >       (location_manager): Likewise.
> > >       (type_maps): Likewise.
> > >       (translation_unit): Likewise.
> > >       (elf_symbol::version): Likewise.
> > >       (type_or_decl_base): Likewise.
> > >       (scope_decl): Likewise.
> > >       (qualified_type_def): Likewise.
> > >       (pointer_type_def): Likewise.
> > >       (array_type_def): Likewise.
> > >       (subrange_type): Likewise.
> > >       (enum_type_decl): Likewise.
> > >       (enum_type_decl::enumerator): Likewise.
> > >       (typedef_decl): Likewise.
> > >       (dm_context_rel): Likewise.
> > >       (var_decl): Likewise.
> > >       (function_decl::parameter): Likewise.
> > >       (function_type): Likewise.
> > >       (method_type): Likewise.
> > >       (template_decl): Likewise.
> > >       (template_parameter): Likewise.
> > >       (type_tparameter): Likewise.
> > >       (non_type_tparameter): Likewise.
> > >       (template_tparameter): Likewise.
> > >       (type_composition): Likewise.
> > >       (function_tdecl): Likewise.
> > >       (class_tdecl): Likewise.
> > >       (class_decl::base_spec): Likewise.
> > >       (ir_node_visitor): Likewise.
> > >       * include/abg-suppression.h (suppression_base): Likewise.
> > >       (type_suppression::insertion_range): Likewise.
> > >       (type_suppression::insertion_range::boundary): Likewise.
> > >       (type_suppression::insertion_range::integer_boundary): Likewise.
> > >       (type_suppression::insertion_range::fn_call_expr_boundary):
> Likewise.
> > >       (function_suppression): Likewise.
> > >       (function_suppression::parameter_spec): Likewise.
> > >       (file_suppression): Likewise.
> > >       * include/abg-tools-utils.h (temp_file): Likewise.
> > >       (timer): Likewise.
> > >       * include/abg-traverse.h (traversable_base): Likewise.
> > >       * include/abg-workers.h (queue): Likewise.
> > >       * src/abg-comparison.cc (diff_context): add default destructor.
> > >       (diff_maps): Likewise.
> > >       (corpus_diff): Likewise.
> > >       (diff_node_visitor): Likewise.
> > >       (class_or_union_diff::get_priv): adjust return type.
> > >       (class_diff::get_priv): adjust return type.
> > >       * src/abg-corpus.cc (corpus): add default destructor.
> > >       * src/abg-ir.cc (location_manager): Likewise.
> > >       (type_maps): Likewise.
> > >       (elf_symbol::version): Likewise.
> > >       (array_type_def::subrange_type): Likewise.
> > >       (enum_type_decl::enumerator): Likewise.
> > >       (function_decl::parameter): Likewise.
> > >       (class_decl::base_spec): Likewise.
> > >       (ir_node_visitor): Likewise.
> > >
> > > Signed-off-by: Matthias Maennich <maennich@google.com>
> >
> > Applied to master.  Thanks!
> >
> > [...]
> >
> > Cheers,
> >
> > --
> >                 Dodji
>