[PATCH, applied] ir: Cache more aggregate type comparison results

[PATCH, applied] ir: Cache more aggregate type comparison results

[Dodji Seketeli]

Hello,

In the aftermath of
https://sourceware.org/bugzilla/show_bug.cgi?id=29857, I figured
caching comparison results at one place (in a macro) leads to better
maintainability.  Also, using that macro in the equal() overload for
class_decl, union_decl and function_type results in slightly more
results of aggregate type comparison being cached during type
canonicalization.  And that might speed things up.

When measuring the impact on the comparison of the bug mentionned
above, comparison that takes ~ 43 minutes, we gain 30secs with this
patch.  We went from:

    $ time ~/build/tools/abidiff --d1 ghostscript-9.07-31.el7.x86_64/usr/lib/debug --d2 ghostscript-9.52-5.oe1.x86_64/usr/lib/debug/ ghostscript-9.07-31.el7.x86_64/usr/lib64/libgs.so.9 ghostscript-9.52-5.oe1.x86_64/usr/lib64/libgs.so.9

    Functions changes summary: 342 Removed, 940 Changed (2084 filtered out), 586 Added functions
    Variables changes summary: 70 Removed, 432 Changed (333 filtered out), 81 Added variables
    Function symbols changes summary: 2 Removed, 19 Added function symbols not referenced by debug info
    Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info

    (...)

    real	42m33,633s
    user	41m54,699s
    sys	0m1,381s
    $

to:

    $ time ~/build/tools/abidiff --d1 ghostscript-9.07-31.el7.x86_64/usr/lib/debug --d2 ghostscript-9.52-5.oe1.x86_64/usr/lib/debug/ ghostscript-9.07-31.el7.x86_64/usr/lib64/libgs.so.9 ghostscript-9.52-5.oe1.x86_64/usr/lib64/libgs.so.9

    Functions changes summary: 342 Removed, 940 Changed (2084 filtered out), 586 Added functions
    Variables changes summary: 70 Removed, 432 Changed (333 filtered out), 81 Added variables
    Function symbols changes summary: 2 Removed, 19 Added function symbols not referenced by debug info
    Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info

    (...)

    real	42m2,364s
    user	41m23,911s
    sys	0m0,326s
    $

	* src/abg-ir.cc (CACHE_AND_RETURN_COMPARISON_RESULT): Define new
	macro.
	(equals): In the overloads for function_type, class_decl, and
	union_decl use the new CACHE_AND_RETURN_COMPARISON_RESULT.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-ir.cc | 63 ++++++++++++++++-----------------------------------
 1 file changed, 20 insertions(+), 43 deletions(-)

diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 0c9a32df..08fc3ad0 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1081,6 +1081,13 @@ return_comparison_result(T& l, T& r, bool value,
   ABG_RETURN(value);
 }
 
+#define CACHE_AND_RETURN_COMPARISON_RESULT(value)			\
+  do									\
+    {									\
+      bool res = return_comparison_result(l, r, value);		\
+      l.get_environment().priv_->cache_type_comparison_result(l, r, res); \
+      return res;							\
+    } while (false)
 /// Getter of all types types sorted by their pretty representation.
 ///
 /// @return a sorted vector of all types sorted by their pretty
@@ -19584,11 +19591,9 @@ function_type::is_variadic() const
 ///
 ///@return true if lhs == rhs, false otherwise.
 bool
-equals(const function_type& l,
-       const function_type& r,
-       change_kind* k)
+equals(const function_type& l, const function_type& r, change_kind* k)
 {
-#define RETURN(value) return return_comparison_result(l, r, value)
+#define RETURN(value) CACHE_AND_RETURN_COMPARISON_RESULT(value)
 
   RETURN_TRUE_IF_COMPARISON_CYCLE_DETECTED(l, r);
 
@@ -19731,17 +19736,6 @@ equals(const function_type& l,
 	RETURN(result);
     }
 
-  // We are done comparing these two types and we have a full
-  // understanding of how they might be different, if they are.  Let's
-  // cache the result of this comparison -- in case we are asked in a
-  // very near future to compare them again.
-  //
-  // TODO: If further profiling shows its necessity, maybe we should
-  // perform this caching also on the earlier return points of this
-  // function.  That would basically mean to redefine the RETURN macro
-  // to make it perform this caching for us.
-  l.get_environment().priv_->cache_type_comparison_result(l, r, result);
-
   RETURN(result);
 #undef RETURN
 }
@@ -21963,9 +21957,6 @@ class_or_union::operator==(const class_or_union& other) const
 bool
 equals(const class_or_union& l, const class_or_union& r, change_kind* k)
 {
-#define RETURN(value) return return_comparison_result(l, r, value,	\
-						      /*propagate_canonical_type=*/false);
-
   // if one of the classes is declaration-only, look through it to
   // get its definition.
   bool l_is_decl_only = l.get_is_declaration_only();
@@ -22064,6 +22055,15 @@ equals(const class_or_union& l, const class_or_union& r, change_kind* k)
   if (types_defined_same_linux_kernel_corpus_public(l, r))
     return true;
 
+  //TODO: Maybe remove this (cycle detection and canonical type
+  //propagation handling) from here and have it only in the equal
+  //overload for class_decl and union_decl because this one ( the
+  //equal overload for class_or_union) is just a sub-routine of these
+  //two above.
+#define RETURN(value)							\
+  return return_comparison_result(l, r, value,				\
+				  /*propagate_canonical_type=*/false);
+
   RETURN_TRUE_IF_COMPARISON_CYCLE_DETECTED(l, r);
 
   mark_types_as_being_compared(l, r);
@@ -23621,7 +23621,7 @@ equals(const class_decl& l, const class_decl& r, change_kind* k)
 
   mark_types_as_being_compared(l, r);
 
-#define RETURN(value) return return_comparison_result(l, r, value);
+#define RETURN(value) CACHE_AND_RETURN_COMPARISON_RESULT(value)
 
   // Compare bases.
   if (l.get_base_specifiers().size() != r.get_base_specifiers().size())
@@ -23735,17 +23735,6 @@ equals(const class_decl& l, const class_decl& r, change_kind* k)
 	}
     }
 
-  // We are done comparing these two types and we have a full
-  // understanding of how they might be different, if they are.  Let's
-  // cache the result of this comparison -- in case we are asked in a
-  // very near future to compare them again.
-  //
-  // TODO: If further profiling shows its necessity, maybe we should
-  // perform this caching also on the earlier return points of this
-  // function.  That would basically mean to redefine the RETURN macro
-  // to make it perform this caching for us.
-  l.get_environment().priv_->cache_type_comparison_result(l, r, result);
-
   RETURN(result);
 #undef RETURN
 }
@@ -24746,8 +24735,7 @@ equals(const union_decl& l, const union_decl& r, change_kind* k)
       return result;
   }
 
-#define RETURN(value)				\
-  return return_comparison_result(l, r, value);
+#define RETURN(value) CACHE_AND_RETURN_COMPARISON_RESULT(value)
 
   bool result = equals(static_cast<const class_or_union&>(l),
 		       static_cast<const class_or_union&>(r),
@@ -24755,17 +24743,6 @@ equals(const union_decl& l, const union_decl& r, change_kind* k)
 
   mark_types_as_being_compared(l, r);
 
-  // We are done comparing these two types and we have a full
-  // understanding of how they might be different, if they are.  Let's
-  // cache the result of this comparison -- in case we are asked in a
-  // very near future to compare them again.
-  //
-  // TODO: If further profiling shows its necessity, maybe we should
-  // perform this caching also on the earlier return points of this
-  // function.  That would basically mean to redefine the RETURN macro
-  // to make it perform this caching for us.
-  l.get_environment().priv_->cache_type_comparison_result(l, r, result);
-
   RETURN(result);
 }
 
-- 
2.31.1


-- 
		Dodji