graph: store the source indices in the transition vector

... and use it to sort transitions.

* src/graph/graph.hh: Adjust storage of source index.  Provide
remove_dead_transitions_(), sort_transitions_() and
chain_transitions_() methods.
* src/tgba/tgbagraph.cc (merge_transitions): Rewrite using
above methods.
* src/tgba/tgbagraph.hh: Add a comparison operator for
transitions.
* src/tgbatest/degenlskip.test, src/tgbatest/det.test,
src/tgbatest/ltl2ta.test, src/tgbatest/neverclaimread.test,
src/tgbatest/readsave.test: Adjust expected transition order in test
cases.

src/graph/graph.hh

@@ -26,6 +26,8 @@
 #include <tuple>
 #include <cassert>
 #include <iterator>
+#include <algorithm>
+#include <iostream>
 
 namespace spot
 {
@@ -68,6 +70,11 @@ namespace spot
       {
 	return label;
       }
+
+      bool operator<(const boxed_label& other) const
+      {
+	return label < other.label;
+      }
     };
 
     template <>
@@ -143,14 +150,16 @@ namespace spot
 
     // Again two implementation: one with label, and one without.
 
-    template <typename State, typename Transition, typename Trans_Data>
+    template <typename StateIn,
+	      typename StateOut, typename Transition, typename Trans_Data>
     struct SPOT_API trans_storage: public Trans_Data
     {
       typedef Transition transition;
 
-      State dst;		// destination
+      StateOut dst;		// destination
       Transition next_succ;	// next outgoing transition with same
 				// source, or 0
+      StateIn src;		// source
 
       explicit trans_storage()
 	: Trans_Data{}
@@ -158,11 +167,26 @@ namespace spot
       }
 
       template <typename... Args>
-      trans_storage(State dst, Transition next_succ, Args&&... args)
+      trans_storage(StateOut dst, Transition next_succ,
+		    StateIn src, Args&&... args)
 	: Trans_Data{std::forward<Args>(args)...},
-	  dst(dst), next_succ(next_succ)
+	dst(dst), next_succ(next_succ), src(src)
       {
       }
+
+      bool operator<(const trans_storage& other) const
+      {
+	if (src < other.src)
+	  return true;
+	if (src > other.src)
+	  return false;
+	// This might be costly if the destination is a vector
+	if (dst < other.dst)
+	  return true;
+	if (dst < other.dst)
+	  return false;
+	return this->data() < other.data();
+      }
     };
 
     //////////////////////////////////////////////////
@@ -200,12 +224,12 @@ namespace spot
       {
       }
 
-      bool operator==(trans_iterator o)
+      bool operator==(trans_iterator o) const
       {
 	return t_ == o.t_;
       }
 
-      bool operator!=(trans_iterator o)
+      bool operator!=(trans_iterator o) const
       {
 	return t_ != o.t_;
       }
@@ -387,7 +411,7 @@ namespace spot
     typedef internal::distate_storage<transition,
 				      internal::boxed_label<State_Data>>
       state_storage_t;
-    typedef internal::trans_storage<out_state, transition,
+    typedef internal::trans_storage<state, out_state, transition,
 				    internal::boxed_label<Trans_Data>>
       trans_storage_t;
     typedef std::vector<state_storage_t> state_vector;
@@ -520,7 +544,7 @@ namespace spot
       assert(src < states_.size());
 
       transition t = transitions_.size();
-      transitions_.emplace_back(dst, 0, std::forward<Args>(args)...);
+      transitions_.emplace_back(dst, 0, src, std::forward<Args>(args)...);
 
       transition st = states_[src].succ_tail;
       assert(st < t || !st);
@@ -610,40 +634,95 @@ namespace spot
       return t.next_succ == index_of_transition(t);
     }
 
-    void defrag()
-    {
-      if (killed_trans_ == 0)	// Nothing to do.
-	return;
 
-      // Shift all transitions in transitions_.  The algorithm is
+    // To help debugging
-      // similar to remove_if, but it also keeps the correspondence
+    void dump_storage(std::ostream& o) const
-      // between the old and new index as newidx[old] = new.
+    {
       unsigned tend = transitions_.size();
-      std::vector<transition> newidx(tend);
+      for (unsigned t = 1; t < tend; ++t)
-      unsigned dest = 1;
+	{
+	  o << 't' << t << ": (s"
+	    << transitions_[t].src << ", s"
+	    << transitions_[t].dst << ") t"
+	    << transitions_[t].next_succ << '\n';
+	}
+      unsigned send = states_.size();
+      for (unsigned s = 0; s < send; ++s)
+	{
+	  o << 's' << s << ": t"
+	    << states_[s].succ << " t"
+	    << states_[s].succ_tail << '\n';
+	}
+    }
+
+    // Remove all dead transitions.  The transitions_ vector is left
+    // in a state that is incorrect and should eventually be fixed by
+    // a call to chain_transitions_() before any iteration on the
+    // successor of a state is performed.
+    void remove_dead_transitions_()
+    {
+      if (killed_trans_ == 0)
+	return;
+      auto i = std::remove_if(transitions_.begin() + 1, transitions_.end(),
+			      [this](const trans_storage_t& t) {
+				return this->is_dead_transition(t);
+			      });
+      transitions_.erase(i, transitions_.end());
+      killed_trans_ = 0;
+    }
+
+    // This will invalidate all iterators, and also destroy transition
+    // chains.  Call chain_transitions_() immediately afterwards
+    // unless you know what you are doing.
+    template<class Predicate = std::less<trans_storage_t>>
+    void sort_transitions_(Predicate p = Predicate())
+    {
+      //std::cerr << "\nbefore\n";
+      //dump_storage(std::cerr);
+      std::sort(transitions_.begin() + 1, transitions_.end(), p);
+    }
+
+    // Should be called only when it is known that all transitions
+    // with the same destination are consecutive in the vector.
+    void chain_transitions_()
+    {
+      state last_src = -1U;
+      transition tend = transitions_.size();
       for (transition t = 1; t < tend; ++t)
 	{
-	  if (is_dead_transition(t))
+	  state src = transitions_[t].src;
-	    continue;
+	  if (src != last_src)
-	  if (t != dest)
-	    transitions_[dest] = std::move(transitions_[t]);
-	  newidx[t] = dest;
-	  ++dest;
-	}
-
-      transitions_.resize(dest);
-      killed_trans_ = 0;
-
-      // Adjust next_succ pointers in all transitions.
-      for (transition t = 1; t < dest; ++t)
-	transitions_[t].next_succ = newidx[transitions_[t].next_succ];
-
-      // Adjust succ and succ_tails pointers in all states.
-      for (auto& s: states_)
 	    {
-	  s.succ = newidx[s.succ];
+	      states_[src].succ = t;
-	  s.succ_tail = newidx[s.succ_tail];
+	      if (last_src != -1U)
+		{
+		  states_[last_src].succ_tail = t - 1;
+		  transitions_[t - 1].next_succ = 0;
 		}
+	      while (++last_src != src)
+		{
+		  states_[last_src].succ = 0;
+		  states_[last_src].succ_tail = 0;
+		}
+	    }
+	  else
+	    {
+	      transitions_[t - 1].next_succ = t;
+	    }
+	}
+      if (last_src != -1U)
+	{
+	  states_[last_src].succ_tail = tend - 1;
+	  transitions_[tend - 1].next_succ = 0;
+	}
+      unsigned send = states_.size();
+      while (++last_src != send)
+	{
+	  states_[last_src].succ = 0;
+	  states_[last_src].succ_tail = 0;
+	}
+      //std::cerr << "\nafter\n";
+      //dump_storage(std::cerr);
     }
 
     void defrag_states(std::vector<unsigned>&& newst, unsigned used_states)
@@ -651,6 +730,9 @@ namespace spot
       assert(newst.size() == states_.size());
       assert(used_states > 0);
 
+      //std::cerr << "\nbefore defrag\n";
+      //dump_storage(std::cerr);
+
       // Shift all states in states_, as indicated by newst.
       unsigned send = states_.size();
       for (state s = 0; s < send; ++s)
@@ -696,6 +778,7 @@ namespace spot
 	  auto& tr = transitions_[t];
 	  tr.next_succ = newidx[tr.next_succ];
 	  tr.dst = newst[tr.dst];
+	  tr.src = newst[tr.src];
 	  assert(tr.dst != -1U);
 	}
 
@@ -705,6 +788,9 @@ namespace spot
 	  s.succ = newidx[s.succ];
 	  s.succ_tail = newidx[s.succ_tail];
 	}
+
+      //std::cerr << "\nafter defrag\n";
+      //dump_storage(std::cerr);
     }
 
   };

src/tgba/tgbagraph.cc

@@ -25,41 +25,62 @@ namespace spot
 
   void tgba_digraph::merge_transitions()
   {
-    // Map a pair (dest state, acc) to the first transition seen
+    g_.remove_dead_transitions_();
-    // with such characteristic.
-    typedef std::pair<graph_t::state, acc_cond::mark_t> key_t;
-    std::unordered_map<key_t, graph_t::transition, pair_hash> trmap;
-    for (auto& s: g_.states())
-      {
-	// Get a clear map for each state.
-	trmap.clear();
 
-	auto t = g_.out_iteraser(s);
+    typedef graph_t::trans_storage_t tr_t;
-	while (t)
+    g_.sort_transitions_([](const tr_t& lhs, const tr_t& rhs)
 			 {
-	    // Simply skip false transitions.
+			   if (lhs.src < rhs.src)
-	    if (t->cond == bddfalse)
+			     return true;
+			   if (lhs.src > rhs.src)
+			     return false;
+			   if (lhs.dst < rhs.dst)
+			     return true;
+			   if (lhs.dst > rhs.dst)
+			     return false;
+			   return lhs.acc < rhs.acc;
+			   // Do not sort on conditions, we'll merge
+			   // them.
+			 });
+
+    auto& trans = this->transitions();
+    unsigned tend = trans.size();
+    unsigned out = 0;
+    unsigned in = 1;
+    // Skip any leading false transition.
+    while (in < tend && trans[in].cond == bddfalse)
+      ++in;
+    if (in < tend)
       {
-		t.erase();
+	++out;
+	if (out != in)
+	  trans[out] = trans[in];
+	for (++in; in < tend; ++in)
+	  {
+	    if (trans[in].cond == bddfalse) // Unusable transition
 	      continue;
-	      }
+	    // Merge transitions with the same source, destination, and
-
+	    // acceptance.  (We test the source last, because this is the
-	    key_t k(t->dst, t->acc);
+	    // most likely test to be true as transitions are ordered by
-	    auto p = trmap.emplace(k, t.trans());
+	    // sources and then destinations.)
-	    if (!p.second)
+	    if (trans[out].dst == trans[in].dst
+		&& trans[out].acc == trans[in].acc
+		&& trans[out].src == trans[in].src)
 	      {
-		// A previous transitions exists for k.  Merge the
+		trans[out].cond |= trans[in].cond;
-		// condition, and schedule the transition for removal.
-		g_.trans_data(p.first->second).cond |= t->cond;
-		t.erase();
 	      }
 	    else
 	      {
-		++t;
+		++out;
+		if (in != out)
+		  trans[out] = trans[in];
 	      }
 	  }
       }
-    g_.defrag();
+    if (++out != tend)
+      trans.resize(out);
+
+    g_.chain_transitions_();
   }
 
   void tgba_digraph::purge_unreachable_states()

src/tgba/tgbagraph.hh

@@ -87,6 +87,15 @@ namespace spot
       : cond(cond), acc(acc)
     {
     }
+
+    bool operator<(const tgba_graph_trans_data& other) const
+    {
+      if (cond.id() < other.cond.id())
+	return true;
+      if (cond.id() > other.cond.id())
+	return false;
+      return acc < other.acc;
+    }
   };
 
 

src/tgbatest/degenlskip.test

@@ -38,15 +38,15 @@ digraph G {
   0 -> 1
   1 [label="0", peripheries=2]
   1 -> 1 [label="a & b\n{0}"]
-  1 -> 2 [label="!a & b\n{0}"]
+  1 -> 2 [label="!b\n{0}"]
-  1 -> 3 [label="!b\n{0}"]
+  1 -> 3 [label="!a & b\n{0}"]
   2 [label="1"]
-  2 -> 1 [label="a"]
+  2 -> 1 [label="a & b"]
-  2 -> 2 [label="!a"]
+  2 -> 2 [label="!b"]
+  2 -> 3 [label="!a & b"]
   3 [label="2"]
-  3 -> 1 [label="a & b"]
+  3 -> 1 [label="a"]
-  3 -> 2 [label="!a & b"]
+  3 -> 3 [label="!a"]
-  3 -> 3 [label="!b"]
 }
 EOF
 
@@ -58,13 +58,14 @@ digraph G {
   1 [label="0", peripheries=2]
   1 -> 2 [label="1\n{0}"]
   2 [label="1"]
-  2 -> 3 [label="b"]
   2 -> 2 [label="!b"]
+  2 -> 3 [label="b"]
   3 [label="2"]
   3 -> 1 [label="a"]
   3 -> 3 [label="!a"]
 }
 EOF
 
+# FIXME: use are-isomorphic once it is available
 diff out2 expected2
 diff out3 expected3

src/tgbatest/det.test

@@ -72,23 +72,24 @@ EOF
 # FIXME: we should improve this output
 cat >ex.tgba <<'EOF'
 acc = "0";
-"0", "1", "a",;
 "0", "0", "!a",;
+"0", "1", "a",;
 "0", "3", "!a",;
-"1", "2", "a",;
 "1", "0", "!a",;
+"1", "2", "a",;
 "1", "3", "!a",;
 "3", "3", "!a", "0";
-"2", "2", "a",;
 "2", "0", "!a",;
-"2", "5", "a",;
+"2", "2", "a",;
 "2", "3", "!a",;
-"5", "5", "a", "0";
+"2", "5", "a",;
 "5", "3", "!a", "0";
+"5", "5", "a", "0";
 EOF
 
 
 run 0 ../ltl2tgba -b -DC -X in.tgba > out.tgba
+# FIXME: use are-isomorphic once it is available
 diff out.tgba ex.tgba
 
 

src/tgbatest/ltl2ta.test

@@ -403,7 +403,7 @@ in: FG((WaitRight4 M (HasRight1 W GWaitLeft0)) M HasLeft4)
 -TA -DS -RT          |     42 |    600 |     19
 -TA -DS -lv          |     55 |    800 |     19
 -TA -DS -lv -RT      |     44 |    694 |     13
--TA -DS -sp          |     54 |    797 |     18
+-TA -DS -sp          |     54 |    795 |     18
 -TA -DS -sp -RT      |     43 |    683 |     12
 -TA -DS -lv -sp      |     55 |    800 |     19
 -TA -DS -lv -sp -RT  |     44 |    694 |     13

src/tgbatest/neverclaimread.test

@@ -51,9 +51,9 @@ digraph G {
   1 -> 1 [label="1"]
   1 -> 2 [label="p0 & p1"]
   2 [label="1"]
-  2 -> 3 [label="p1 & !p0"]
-  2 -> 2 [label="!p1"]
   2 -> 1 [label="!p1"]
+  2 -> 2 [label="!p1"]
+  2 -> 3 [label="p1 & !p0"]
   3 [label="2", peripheries=2]
   3 -> 3 [label="1\n{0}"]
 }
@@ -63,6 +63,7 @@ EOF
 # (The order is not guaranteed by SPOT.)
 sed -e 's/!p0 & p1/p1 \& !p0/g' -e 's/p1 & p0/p0 \& p1/g' stdout \
     > tmp_ && mv tmp_ stdout
+# FIXME: use are-isomorphic once it is available
 diff stdout expected
 
 rm input stdout expected
@@ -95,9 +96,9 @@ digraph G {
   1 -> 1 [label="1"]
   1 -> 2 [label="p0 & p1"]
   2 [label="1"]
-  2 -> 3 [label="p1 & !p0"]
-  2 -> 2 [label="!p1"]
   2 -> 1 [label="!p1"]
+  2 -> 2 [label="!p1"]
+  2 -> 3 [label="p1 & !p0"]
   3 [label="2", peripheries=2]
   3 -> 3 [label="1\n{0}"]
 }
@@ -107,6 +108,7 @@ EOF
 # (The order is not guaranteed by SPOT.)
 sed -e 's/!p0 & p1/p1 \& !p0/g' -e 's/p1 & p0/p0 \& p1/g' stdout \
     > tmp_ && mv tmp_ stdout
+# FIXME: use are-isomorphic once it is available
 diff stdout expected
 
 rm input stdout expected

src/tgbatest/readsave.test

@@ -72,13 +72,14 @@ EOF
 
 cat >expected <<\EOF
 acc = "0";
-"0", "1", "a", "0";
 "0", "1", "!b",;
+"0", "1", "a", "0";
 "1", "0", "!b",;
 "1", "0", "a", "0";
 EOF
 
 run 0 ../ltl2tgba -b -X input > stdout
+# FIXME: use are-isomorphic once it is available
 diff stdout expected
 
 # Likewise, with a randomly generated TGBA.