Use std::vector in Signal

[DanRStevens]

Reference: #1015

There are two main reasons for this change:

1. To make it easier to use std::function internally rather than the custom Delegate class
2. Speed (for the typical use case)

Using a std::set requires the stored object to be "less than" comparable. The custom Delegate class has an operator<. That being a bit odd aside, the std::function class does not have operator<. That means switching to std::function would either require a custom operator< function, or we could change to a different collection class that doesn't require operator< for the stored elements.

In terms of speed, a std::vector is about the fastest collection class you can likely get for small amounts of data. It's simple, and caches well, so pretty much all operations will be fast for small collections. Things like std::set may have insert and delete speed advantages for larger collections, due to better asymptotic complexity, but the more complex algorithms and lower cache locality mean it'll be slower for small collections. Most Signal objects only have a single listener. That far favours using a simpler collection, such as std::vector. Further, the main operation will generally be emit, rather than connect or disconnect. That means collection traversal speed matters more than insert and delete speed. In that case, the std::vector will definitely perform better, due to simpler iteration and better cache locality.

[DanRStevens]

As a bit of an aside, I did try some speed testing, just out of curiosity. I played with a few parameters in a very ad hoc manner. Here's some sample test code to compare speed:

namespace {
	class MockHandler {
	public:
		MOCK_CONST_METHOD0(MockMethod, void());
		MOCK_CONST_METHOD0(MockMethod2, void());
		MOCK_CONST_METHOD0(MockMethod3, void());
		MOCK_CONST_METHOD0(MockMethod4, void());
		MOCK_CONST_METHOD0(MockMethod5, void());
		MOCK_CONST_METHOD0(MockMethod6, void());
		MOCK_CONST_METHOD0(MockMethod7, void());
		MOCK_CONST_METHOD0(MockMethod8, void());
		MOCK_CONST_METHOD0(MockMethod9, void());
		MOCK_CONST_METHOD0(MockMethod10, void());
	};
}

// ...

TEST(Signal, Timing) {
	MockHandler handler{};
	auto delegate = NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod);

	for (int i = 0; i < 100000; ++i) {
		NAS2D::Signal<> signal;
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod2));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod3));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod4));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod5));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod6));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod7));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod8));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod9));
		// signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod10));
		for (int j = 0; j < 100; ++j) {
			signal.connect(delegate);
			// signal.disconnect(delegate);
		}
	}
}

Without extra connected methods (that are unique, since duplicates are ignored), the std::vector performs as well as or better than std::set. Sometimes std::set took about 50% more time. This was true even when iterating the connect/disconnect calls many times (tested ~ 1 .. 1000). The main factor in speed differences was the presence of other listeners. Once you get to about 10 connected listeners, it starts to become a little bit faster (~10%) to call connect/disconnect with an underlying std::set collection type.

Doing a similar test with emit() shows std::vector performs the same as or better than std::vector. The std::set code would sometimes take about an extra 25% more time to run. I used a plain class for that, since using Google Test mock methods had significant overhead, which would drown out the measurements for the dispatch timing.

TEST(Signal, Timing) {
	struct MockHandler {
		void MockMethod() {}
		void MockMethod2() {}
		void MockMethod3() {}
		void MockMethod4() {}
		void MockMethod5() {}
		void MockMethod6() {}
		void MockMethod7() {}
		void MockMethod8() {}
		void MockMethod9() {}
		void MockMethod10() {}
	};
	MockHandler handler{};
	auto delegate = NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod);

	for (int i = 0; i < 100000; ++i) {
		NAS2D::Signal<> signal;
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod2));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod3));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod4));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod5));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod6));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod7));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod8));
		signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod9));
		// signal.connect(NAS2D::MakeDelegate(&handler, &MockHandler::MockMethod10));
		signal.connect(delegate);
		for (int j = 0; j < 100; ++j) {
			signal.emit();
		}
		signal.disconnect(delegate);
	}
}

[cugone]

Additionally, deletion from, and insertion to, the end of a vector is very fast via the .pop_back() and .push_back() member functions. Leveraging this fact, if order doesn't matter in the collection, deletetion of an element can be very fast via the swap-and-pop idiom:

1. Swap the element to be deleted with the last element in the vector.
2. Call .pop_back().

This makes insertion, traversal, and deletion using a vector very fast, sometimes faster than both the ordered and unordered associative containers (map, unordered_map, set, unordered_set) due to cache locality and the above.

[DanRStevens]

That's a valid point. I don't believe insertion order was preserved by std::set, so technically we've never made any guarantees about the order of connect calls matching the order of callbacks during an emit.

We could avoid re-seating costs by overwriting the deleted element with the last entry in the std::vector, and then popping to reduce size. We wouldn't necessarily need to swap per se, as there's no need to copy/move the item being deleted.

---

Of course, the change here wasn't originally to optimize the Signal code, but rather to help replace Delegate with std::function. That effort has kind of failed since std::function is not equality comparable (and neither are lambdas), so can't replace Delegate with the current design of Signal. In particular, the std::find calls in connect and disconnect require equality comparable.

Of course, if we don't check for duplicates in connect, and don't offer disconnect other than through destruction of the Signal object, that problem kind of goes away. Most connect/disconnect calls are related to the way UI event handling is currently done. If we implement the event bubbling idea, we could potentially remove the vast bulk of connect and disconnect calls (and maybe even most uses of Signal). If we get to that point, it may be worth revisiting, and seeing if we still need Signal, or if the disconnect function can be removed and rely solely on the destructor to disconnect listeners. Might not even need a connect method either, if all connections are set in a constructor call. But then, that's getting to be more like SignalConnection.