Apply harmless and harmful filters in one pass

When comparing linux kernels with lots of changes, walking changes
twice just to apply harmless and harmful change filters was dominating
the performance profile.

This patch performs the harmless and harmful filtering in one pass.
This makes the overall comparison go from 15 minutes to 10 minutes
when comparing a 4.7 kernel from fedora24 and a 4.8 kernel from
fedora26.

	* include/abg-comp-filter.h (class harmless_harmful_filter):
	Decalre new class.
	(typedef harmless_harmful_filter_sptr): Declare new typedef.
	(class harmless_filter, class harmful_filter): Remove these class
	declarations.
	(typedef harmful_filter_sptr, harmless_filter_sptr): Remove these
	typedefs.
	* src/abg-comp-filter.cc (categorize_harmless_diff_node)
	(categorize_harmful_diff_node): Define new static functions.
	({harmless, harmful}_filter::{visit, visit_end}): Remove these
	member functions.
	(harmless_harmful_filter::{visit, visit_end}): Define new member
	functions.
	* src/abg-comparison.cc (diff_context::diff_context): Register the
	new harmless_harmful_filter, and remove the premier
	harmless_filter and harmful_filter.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
	# Please enter the commit message for your changes. Lines starting
	# with '#' will be ignored, and an empty message aborts the
	commit.  # On branch kabidiff-dedup # Changes to be committed: #
	(use "git reset HEAD <file>..." to unstage) # # modified:
	include/abg-comp-filter.h # modified: src/abg-comp-filter.cc #
	modified: src/abg-comparison.cc # # Untracked files: # (use "git
	add <file>..." to include in what will be committed) # # diff.txt
	# prtests/ # tests/data/test-read-dwarf/libtest23.so.abi.conflict

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h
index 2a42e6aec8f866443f09575c96e3c356ca8316f2..4a7be7a98160d6e1a4c7d310a38ee42b1c85a903 100644 (file)
--- a/include/abg-comp-filter.h
+++ b/include/abg-comp-filter.h
@@ -86,21 +86,21 @@ class harmless_filter : public filter_base
   visit_end(diff*);
 }; // end class harmless_filter
 
-class harmful_filter;
+class harmless_harmful_filter;
 /// A convenience typedef for a shared pointer to harmful_filter.
-typedef shared_ptr<harmful_filter> harmful_filter_sptr;
+typedef shared_ptr<harmless_harmful_filter> harmful_harmless_filter_sptr;
 
 /// A filter that walks the diff nodes tree and tags relevant diff
-/// nodes into categories considered to represent potentially harmful
-/// changes.
-class harmful_filter : public filter_base
+/// nodes into categories considered to represent potentially harmless
+/// or harmful changes.
+class harmless_harmful_filter : public filter_base
 {
   virtual bool
   visit(diff*, bool);
 
   virtual void
   visit_end(diff*);
-}; // end class harmful_filter
+}; // end class harmless_harmful_filter
 
 } // end namespace filtering
 } // end namespace comparison

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index 62c6213932d8941a885462b87e56efb8ec761453..c9ffb4b8f085b2d8b655a4ae3dad80d9c879f18c 100644 (file)
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -764,8 +764,6 @@ has_harmful_enum_change(const diff* diff)
   return false;
 }
 
-/// The visiting code of the harmless_filter.
-///
 /// Detect if the changes carried by a given diff node are deemed
 /// harmless and do categorize the diff node accordingly.
 ///
@@ -776,16 +774,16 @@ has_harmful_enum_change(const diff* diff)
 ///
 /// @return true iff the traversal shall keep going after the
 /// completion of this function.
-bool
-harmless_filter::visit(diff* d, bool pre)
+static bool
+categorize_harmless_diff_node(diff *d, bool pre)
 {
   if (!d->has_changes())
     return true;
 
-  diff_category category = NO_CHANGE_CATEGORY;
-
   if (pre)
     {
+      diff_category category = NO_CHANGE_CATEGORY;
+
       decl_base_sptr f = is_decl(d->first_subject()),
        s = is_decl(d->second_subject());
 
@@ -825,36 +823,8 @@ harmless_filter::visit(diff* d, bool pre)
   return true;
 }
 
-/// Part of the visiting code of the harmless_filter.
-///
-/// This function is called after the visiting of a given diff node.
-/// Note that when this function is called, the visiting might not
-/// have taken place *if* the node (or an equivalent node) has already
-/// been visited.
-///
-/// @param d the diff node that has either been visited or skipped
-/// (because it has already been visited during this traversing).
-void
-harmless_filter::visit_end(diff* d)
-{
-  if (d->context()->diff_has_been_visited(d))
-    {
-      // This node or one of its equivalent node has already been
-      // visited.  That means at this moment, harmless_filter::visit()
-      // has *not* been called prior to this
-      // harmless_filter::visit_end() is called.  In other words, only
-      // harmless_filter::visit_begin() and
-      // harmless_filter::visit_end() are called.
-      //
-      // So let's update the category of this diff node from its
-      // canonical node.
-      diff* canonical = d->get_canonical_diff();
-      if (canonical)
-       d->add_to_local_and_inherited_categories(canonical->get_local_category());
-    }
-}
-
-/// The visiting code of the harmful_filter.
+/// Detect if the changes carried by a given diff node are deemed
+/// harmful and do categorize the diff node accordingly.
 ///
 /// @param d the diff node being visited.
 ///
@@ -863,16 +833,15 @@ harmless_filter::visit_end(diff* d)
 ///
 /// @return true iff the traversal shall keep going after the
 /// completion of this function.
-bool
-harmful_filter::visit(diff* d, bool pre)
+static bool
+categorize_harmful_diff_node(diff *d, bool pre)
 {
-  diff_category category = NO_CHANGE_CATEGORY;
-
   if (!d->has_changes())
     return true;
 
   if (pre)
     {
+      diff_category category = NO_CHANGE_CATEGORY;
       decl_base_sptr f = is_decl(d->first_subject()),
        s = is_decl(d->second_subject());
 
@@ -903,7 +872,23 @@ harmful_filter::visit(diff* d, bool pre)
   return true;
 }
 
-/// Part of the visiting code of the harmless_filter.
+/// The visiting code of the harmless_harmful_filter.
+///
+/// @param d the diff node being visited.
+///
+/// @param pre this is true iff the node is being visited *before* the
+/// children nodes of @p d.
+///
+/// @return true iff the traversal shall keep going after the
+/// completion of this function.
+bool
+harmless_harmful_filter::visit(diff* d, bool pre)
+{
+  return (categorize_harmless_diff_node(d, pre)
+         && categorize_harmful_diff_node(d, pre));
+}
+
+/// Part of the visiting code of the harmless_harmful_filter.
 ///
 /// This function is called after the visiting of a given diff node.
 /// Note that when this function is called, the visiting might not
@@ -913,25 +898,23 @@ harmful_filter::visit(diff* d, bool pre)
 /// @param d the diff node that has either been visited or skipped
 /// (because it has already been visited during this traversing).
 void
-harmful_filter::visit_end(diff* d)
+harmless_harmful_filter::visit_end(diff* d)
 {
   if (d->context()->diff_has_been_visited(d))
     {
       // This node or one of its equivalent node has already been
-      // visited.  That means at this moment, harmful_filter::visit()
-      // has *not* been called prior to this
-      // harmful_filter::visit_end() is called.  In other words, only
-      // harmful_filter::visit_begin() and harmful_filter::visit_end()
-      // are called.
+      // visited.  That means at this moment,
+      // harmless_harmful_filter::visit() has *not* been called prior
+      // to this harmless_harmful_filter::visit_end() is called.  In
+      // other words, only harmless_harmful_filter::visit_begin() and
+      // harmless_harmful_filter::visit_end() are called.
       //
-      // So let's update the category of this diff node from it's
-      // cnanonical node.
-      diff* canonical = d->get_canonical_diff();
-      if (canonical)
-       d->add_to_local_and_inherited_categories(canonical->get_local_category());
+      // So let's update the category of this diff node from its
+      // canonical node.
+      if (diff* c = d->get_canonical_diff())
+       d->add_to_local_and_inherited_categories(c->get_local_category());
     }
 }
-
 } // end namespace filtering
 } // end namespace comparison
 } // end namespace abigail

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index f2acb25d25fdb1c09391341a601e74836fd3c453..a599997b2387a1110cad847fb9c162744209f80e 100644 (file)
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -557,11 +557,14 @@ diff_context::diff_context()
   // Setup all the diff output filters we have.
   filtering::filter_base_sptr f;
 
-  f.reset(new filtering::harmless_filter);
+  f.reset(new filtering::harmless_harmful_filter);
   add_diff_filter(f);
 
-  f.reset(new filtering::harmful_filter);
-  add_diff_filter(f);
+  // f.reset(new filtering::harmless_filter);
+  // add_diff_filter(f);
+
+  // f.reset(new filtering::harmful_filter);
+  // add_diff_filter(f);
 }
 
 /// Set the corpora that are being compared into the context, so that