[2/2] ir: Cache the pretty representation used during pre-canonicalization type sorting
Commit Message
Hello,
Profiling showed that pre-canonicalization type sorting (which uses
the type_topo_comp sorting functor) we spend a lot of time computing
the pretty representation of types and decls invoked from
type_topo_comp::operator() and decl_topo_comp::operator().
This patch thus uses type_base::get_cached_pretty_representation and
introduces a new decl_base::get_cached_pretty_representation to cache
the pretty representation of types and decls and re-use that
representation instead of computing it over and over.
Together with the previous patch of this series, the time spent to
analyze a Linux Kernel tree went from ~ 55 minutes to around 1m15s
using a non-optimized build of libabigail.
Note that these two patches are logically independent but I'd like to
apply them in tandem as they are the result of the profiling and
optimization work suggested by Claudiu Zissulescu Ianculescu.
This patch has been tested on the sourceware CI and is thus ready to be
applied to the mainline once the previous one gets in.
* include/abg-ir.h (decl_base::get_cached_pretty_representation):
Declare new member function.
* src/abg-ir-priv.h ({type,decl}_topo_comp::operator()): Use
{type,decl}_base::get_cached_pretty_representation instead of
get_pretty_representation.:
* src/abg-ir.cc (decl_base::priv::{internal_cached_repr_,
cached_repr}): Define new data members.
(decl_base::get_cached_pretty_representation):
Define new member function.
(type_base::get_cached_pretty_representation): Cache pretty
representation even for non-canonicalized types. Fix comments.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
include/abg-ir.h | 3 +++
src/abg-ir-priv.h | 38 +++++++++++++++---------------
src/abg-ir.cc | 59 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 76 insertions(+), 24 deletions(-)
Comments
Dodji Seketeli <dodji@redhat.com> a écrit:
[...]
> Profiling showed that pre-canonicalization type sorting (which uses
> the type_topo_comp sorting functor) we spend a lot of time computing
> the pretty representation of types and decls invoked from
> type_topo_comp::operator() and decl_topo_comp::operator().
>
> This patch thus uses type_base::get_cached_pretty_representation and
> introduces a new decl_base::get_cached_pretty_representation to cache
> the pretty representation of types and decls and re-use that
> representation instead of computing it over and over.
>
> Together with the previous patch of this series, the time spent to
> analyze a Linux Kernel tree went from ~ 55 minutes to around 1m15s
> using a non-optimized build of libabigail.
>
> Note that these two patches are logically independent but I'd like to
> apply them in tandem as they are the result of the profiling and
> optimization work suggested by Claudiu Zissulescu Ianculescu.
>
> This patch has been tested on the sourceware CI and is thus ready to be
> applied to the mainline once the previous one gets in.
>
> * include/abg-ir.h (decl_base::get_cached_pretty_representation):
> Declare new member function.
> * src/abg-ir-priv.h ({type,decl}_topo_comp::operator()): Use
> {type,decl}_base::get_cached_pretty_representation instead of
> get_pretty_representation.:
> * src/abg-ir.cc (decl_base::priv::{internal_cached_repr_,
> cached_repr}): Define new data members.
> (decl_base::get_cached_pretty_representation):
> Define new member function.
> (type_base::get_cached_pretty_representation): Cache pretty
> representation even for non-canonicalized types. Fix comments.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to the mainline.
[...]
Cheers,
@@ -1622,6 +1622,9 @@ public:
decl_base(const environment&, const location&);
+ const interned_string&
+ get_cached_pretty_representation(bool internal = false) const;
+
virtual bool
operator==(const decl_base&) const;
@@ -1327,8 +1327,8 @@ struct decl_topo_comp
// of their pretty representation before we start looking at IR
// nodes' locations down the road.
if (is_unique_type(is_type(f)) || is_unique_type(is_type(s)))
- return (get_pretty_representation(f, /*internal=*/false)
- < get_pretty_representation(s, /*internal=*/false));
+ return (f->get_cached_pretty_representation(/*internal=*/false)
+ < s->get_cached_pretty_representation(/*internal=*/false));
// If both decls come from an abixml file, keep the order they
// have from that abixml file.
@@ -1353,12 +1353,12 @@ struct decl_topo_comp
// We reach this point if location data is useless.
if (f->get_is_anonymous()
&& s->get_is_anonymous()
- && (get_pretty_representation(f, /*internal=*/false)
- == get_pretty_representation(s, /*internal=*/false)))
+ && (f->get_cached_pretty_representation(/*internal=*/false)
+ == s->get_cached_pretty_representation(/*internal=*/false)))
return f->get_name() < s->get_name();
- return (get_pretty_representation(f, /*internal=*/false)
- < get_pretty_representation(s, /*internal=*/false));
+ return (f->get_cached_pretty_representation(/*internal=*/false)
+ < s->get_cached_pretty_representation(/*internal=*/false));
}
/// The "Less Than" comparison operator of this functor.
@@ -1454,8 +1454,8 @@ struct type_topo_comp
&& !has_artificial_or_natural_location(f)
&& !has_artificial_or_natural_location(s))
{
- string s1 = get_pretty_representation(f, /*internal=*/false);
- string s2 = get_pretty_representation(s, /*internal=*/false);
+ interned_string s1 = f->get_cached_pretty_representation(/*internal=*/false);
+ interned_string s2 = s->get_cached_pretty_representation(/*internal=*/false);
if (s1 == s2)
{
if (qualified_type_def * q = is_qualified_type(f))
@@ -1497,8 +1497,8 @@ struct type_topo_comp
type_base *peeled_s =
peel_pointer_or_reference_type(s, true);
- s1 = get_pretty_representation(peeled_f, /*internal=*/false);
- s2 = get_pretty_representation(peeled_s, /*internal=*/false);
+ s1 = peeled_f->get_cached_pretty_representation(/*internal=*/false);
+ s2 = peeled_s->get_cached_pretty_representation(/*internal=*/false);
if (s1 != s2)
return s1 < s2;
@@ -1508,25 +1508,23 @@ struct type_topo_comp
peeled_f = peel_typedef_pointer_or_reference_type(peeled_f, true);
peeled_s = peel_typedef_pointer_or_reference_type(peeled_s, true);
- s1 = get_pretty_representation(peeled_f, false);
- s2 = get_pretty_representation(peeled_s, false);
+ s1 = peeled_f->get_cached_pretty_representation(false);
+ s2 = peeled_s->get_cached_pretty_representation(false);
if (s1 != s2)
return s1 < s2;
}
}
- string s1 = get_pretty_representation(f, false);
- string s2 = get_pretty_representation(s, false);
+ interned_string s1 = f->get_cached_pretty_representation(false);
+ interned_string s2 = s->get_cached_pretty_representation(false);
if (s1 != s2)
return s1 < s2;
if (is_typedef(f) && is_typedef(s))
{
- s1 = get_pretty_representation(is_typedef(f)->get_underlying_type(),
- false);
- s2 = get_pretty_representation(is_typedef(s)->get_underlying_type(),
- false);
+ s1 = is_typedef(f)->get_underlying_type()->get_cached_pretty_representation(false);
+ s2 = is_typedef(s)->get_underlying_type()->get_cached_pretty_representation(false);
if (s1 != s2)
return s1 < s2;
}
@@ -1534,8 +1532,8 @@ struct type_topo_comp
type_base *peeled_f = peel_typedef_pointer_or_reference_type(f, true);
type_base *peeled_s = peel_typedef_pointer_or_reference_type(s, true);
- s1 = get_pretty_representation(peeled_f, false);
- s2 = get_pretty_representation(peeled_s, false);
+ s1 = peeled_f->get_cached_pretty_representation(false);
+ s2 = peeled_s->get_cached_pretty_representation(false);
if (s1 != s2)
return s1 < s2;
@@ -4358,6 +4358,8 @@ struct decl_base::priv
interned_string qualified_name_;
interned_string temporary_internal_qualified_name_;
interned_string internal_qualified_name_;
+ interned_string internal_cached_repr_;
+ interned_string cached_repr_;
// Unline qualified_name_, scoped_name_ contains the name of the
// decl and the name of its scope; not the qualified name of the
// scope.
@@ -4855,6 +4857,48 @@ decl_base::get_pretty_representation(bool internal,
return get_name();
}
+/// Get the pretty representation of the current decl.
+///
+/// The pretty representation is retrieved from a cache. If the cache
+/// is empty, this function computes the pretty representation, put it
+/// in the cache and returns it.
+///
+/// Please note that if this function is called too early in the life
+/// cycle of the decl (before it is fully constructed), then the
+/// pretty representation that is cached is going to represent a
+/// non-complete (and thus wrong) representation of the decl. Thus
+/// this function must be called only once the decl is fully
+/// constructed.
+///
+/// @param internal if true, then the pretty representation is to be
+/// used for purpuses that are internal to the libabigail library
+/// itself. If you don't know what this means, then you probably
+/// should set this parameter to "false".
+///
+/// @return a reference to a cached @ref interned_string holding the
+/// pretty representation of the current decl.
+const interned_string&
+decl_base::get_cached_pretty_representation(bool internal) const
+{
+ if (internal)
+ {
+ if (priv_->internal_cached_repr_.empty())
+ {
+ string r = ir::get_pretty_representation(this, internal);
+ priv_->internal_cached_repr_ = get_environment().intern(r);
+ }
+ return priv_->internal_cached_repr_;
+ }
+
+ if (priv_->cached_repr_.empty())
+ {
+ string r = ir::get_pretty_representation(this, internal);
+ priv_->cached_repr_ = get_environment().intern(r);
+ }
+
+ return priv_->cached_repr_;
+}
+
/// Return the qualified name of the decl.
///
/// This is the fully qualified name of the decl. It's made of the
@@ -15780,19 +15824,26 @@ type_base::get_naked_canonical_type() const
/// is empty, this function computes the pretty representation, put it
/// in the cache and returns it.
///
-/// Note that if the type is *NOT* canonicalized, the pretty
-/// representation is never cached.
+/// Please note that if this function is called too early in the life
+/// cycle of the type (before the type is fully constructed), then the
+/// pretty representation that is cached is going to represent a
+/// non-complete (and thus wrong) representation of the type. Thus
+/// this function must be called only once the type is fully
+/// constructed.
///
/// @param internal if true, then the pretty representation is to be
/// used for purpuses that are internal to the libabigail library
/// itself. If you don't know what this means, then you probably
/// should set this parameter to "false".
+///
+/// @return a reference to a cached @ref interned_string holding the
+/// pretty representation of the current type.
const interned_string&
type_base::get_cached_pretty_representation(bool internal) const
{
if (internal)
{
- if (!get_naked_canonical_type() || priv_->internal_cached_repr_.empty())
+ if (priv_->internal_cached_repr_.empty())
{
string r = ir::get_pretty_representation(this, internal);
priv_->internal_cached_repr_ = get_environment().intern(r);
@@ -15800,7 +15851,7 @@ type_base::get_cached_pretty_representation(bool internal) const
return priv_->internal_cached_repr_;
}
- if (!get_naked_canonical_type() || priv_->cached_repr_.empty())
+ if (priv_->cached_repr_.empty())
{
string r = ir::get_pretty_representation(this, internal);
priv_->cached_repr_ = get_environment().intern(r);