Sort perf improvements

core/algorithms/sort_product.cc

@@ -46,7 +46,7 @@ Algorithm::result_t sort_product::apply(iterator& st)
 	// about using stable_sort, and then the tree.sort() doesn't do that,
 	// and anyhow you would perhaps want exceptions. Let's just use a bubble
 	// sort since how many times do we have more than 100 items in a product?
-
+	// Revised version below uses improved bubble sort.
 	result_t ret=result_t::l_no_action;
 
 	Ex::sibling_iterator one, two;
@@ -58,12 +58,22 @@ Algorithm::result_t sort_product::apply(iterator& st)
 	//	tr.print_recursive_treeform(std::cout, st);
 
 	unsigned int num=tr.number_of_children(st);
-	for(unsigned int i=1; i<num; ++i) {
+
+	// for(unsigned int i = 1; i < num; ++i) {
+
+	// Record location of last swap:  [last_swap, num) are sorted and larger than [0, last_swap)
+	unsigned int last_swap = num;
+	while (last_swap > 1) {
 		one=tr.begin(st);
 		two=one;
 		++two;
 		//		for(unsigned int j=i+1; j<=num; ++j) { // this loops too many times, no?
-		while(two!=tr.end(st)) {
+
+		unsigned int current_pos = 1;		// Current index location of `two`
+		unsigned int new_last_swap = 0;		// Keep updating where the next last swap happens
+
+		// Inner loop does not need to check inside [last_swap, num) because that's sorted
+		while(two!=tr.end(st) && current_pos < last_swap) {
 			compare.clear();
 			auto es = compare.equal_subtree(one, two);
 			if(compare.should_swap(one, es)) {
@@ -82,12 +92,15 @@ Algorithm::result_t sort_product::apply(iterator& st)
 						//						std::cout << "MINUS" << std::endl;
 						flip_sign(st->multiplier);
 						}
+					new_last_swap = current_pos;
 					ret=result_t::l_applied;
 					}
 				}
 			++one;
 			++two;
+			++current_pos;
 			}
+		last_swap = new_last_swap;
 		}
 
 	if(ret==result_t::l_no_action) {

core/algorithms/sort_sum.cc

@@ -17,34 +17,24 @@ bool sort_sum::can_apply(iterator st)
 
 Algorithm::result_t sort_sum::apply(iterator& st)
 	{
-	// This bubble sort is of course a disaster, but it'll have to do for now.
-
-	result_t ret=result_t::l_no_action;
-	sibling_iterator one, two;
-	unsigned int num=tr.number_of_children(st);
-	for(unsigned int i=1; i<num; ++i) {
-		one=tr.begin(st);
-		two=one;
-		++two;
-		for(unsigned int j=i+1; j<=num; ++j) { // this loops too many times, no?
-			int es=subtree_compare(&kernel.properties, one, two, -2, true, 0, true);
-			if(should_swap(one, es)) {
-				tr.swap(one);
-				std::swap(one,two);  // put the iterators back in order
-				ret=result_t::l_applied;
-				}
-			++one;
-			++two;
-			}
-		}
-
-	return ret;
+	bool changed = tr.subtree_sort(st.begin(), st.end(), 
+		[this](const sibling_iterator& sib1, const sibling_iterator& sib2) {
+            /* The less function for stable_sort requires less(a,a) = false.
+             * Simplest fix is to swap order of arguments below.
+             */
+			int es=subtree_compare(&kernel.properties, sib2, sib1, -2, true, 0, true);
+			return should_swap(sib2, sib1, es);
+			});
+
+	// check if anything actually moved.
+    if (changed) return result_t::l_applied;
+	else return result_t::l_no_action;
 	}
 
-bool sort_sum::should_swap(iterator obj, int subtree_comparison) const
+
+bool sort_sum::should_swap(iterator obj1, iterator obj2, int subtree_comparison) const
 	{
-	sibling_iterator one=obj, two=obj;
-	++two;
+	sibling_iterator one=obj1, two=obj2;
 
 	// Find a SortOrder property which contains both one and two.
 	int num1, num2;
@@ -67,5 +57,4 @@ bool sort_sum::should_swap(iterator obj, int subtree_comparison) const
 		}
 
 	return false;
-	}
-
+	}

core/algorithms/sort_sum.hh

@@ -13,7 +13,8 @@ namespace cadabra {
 			virtual result_t apply(iterator&) override;
 
 		private:
-			bool should_swap(iterator obj, int subtree_comparison) const;
+			// bool should_swap(iterator obj, int subtree_comparison) const;
+			bool should_swap(iterator obj1, iterator obj2, int subtree_comparison) const;
 		};
 
 	}

core/tree.hh

@@ -34,6 +34,8 @@
 #include <algorithm>
 #include <cstddef>
 #include <string>
+#include <vector>
+
 
 /// A node in the tree, combining links to other nodes as well as the actual data.
 template<class T>
@@ -432,6 +434,11 @@ class tree {
 		void     sort(sibling_iterator from, sibling_iterator to, bool deep=false);
 		template<class StrictWeakOrdering>
 		void     sort(sibling_iterator from, sibling_iterator to, StrictWeakOrdering comp, bool deep=false);
+		/// Sort the sibling subtrees using the user-supplied subtree_comp (which must take sibling_iterator args).
+		/// Returns true/false if a sort occurs.
+		template<class StrictWeakOrdering>
+		bool     subtree_sort(sibling_iterator from, sibling_iterator to, StrictWeakOrdering subtree_comp);
+
 		/// Compare two ranges of nodes (compares nodes as well as tree structure).
 		template<typename iter>
 		bool     equal(const iter& one, const iter& two, const iter& three) const;
@@ -2187,6 +2194,52 @@ void tree<T, tree_node_allocator>::sort(sibling_iterator from, sibling_iterator
 		}
 	}
 
+
+template <class T, class tree_node_allocator>
+template <class StrictWeakOrdering>
+bool tree<T, tree_node_allocator>::subtree_sort(sibling_iterator from,
+                                               sibling_iterator to,
+                                               StrictWeakOrdering subtree_comp)
+	{
+    if (from == to) return false;
+
+	// Populate a buffer with the subtrees to be sorted
+	// Use sibling_iterators since tree_node structure hidden outside tree.hh
+    std::vector<sibling_iterator> buf;
+    buf.reserve(std::distance(from, to));
+    for (auto it = from; it != to; ++it) buf.push_back(it);
+
+	// Checks if sort is needed.
+	if (std::is_sorted(buf.begin(), buf.end(), subtree_comp)) return false;
+
+	// Sort using the user-supplied subtree_comp function
+    std::stable_sort(buf.begin(), buf.end(), subtree_comp);
+
+	// What nodes precede and follow the sorted range?
+	tree_node* prev = from.node->prev_sibling;   // may be nullptr
+    sibling_iterator last = to; --last;          // to is exclusive, last is inclusive
+    tree_node* next = last.node->next_sibling;   // may be nullptr
+
+	// Fix the sibling relationships
+	const std::size_t n = buf.size();
+    for (std::size_t i = 0; i < n; ++i)
+		{
+        buf[i].node->prev_sibling = (i == 0)     ? prev : buf[i - 1].node;
+        buf[i].node->next_sibling = (i + 1 == n) ? next : buf[i + 1].node;
+    	}
+
+	// Fix references from parent or siblings outside the sorted range
+    if (prev)  prev->next_sibling  = buf.front().node;
+    else if (buf.front().node->parent)  buf.front().node->parent->first_child = buf.front().node;
+
+    if (next)  next->prev_sibling  = buf.back().node;
+    else if (buf.back().node->parent)   buf.back().node->parent->last_child   = buf.back().node;
+
+	return true;
+	}
+
+
+
 template <class T, class tree_node_allocator>
 template <typename iter>
 bool tree<T, tree_node_allocator>::equal(const iter& one_, const iter& two, const iter& three_) const