AntelopeIO / appbase

Other
1 stars 6 forks source link

multiple priority queues support #8

Closed linh2931 closed 1 year ago

linh2931 commented 1 year ago

This is a part of the parallelizing read-only transaction execution effort (https://github.com/eosnetworkfoundation/product/blob/main/transactions/read-only/parallel.md)

With multiple priority queues support, users can add additional priority queues into AppBase, decide which queue a function is posted to, and manage how functions in the queues are executed. To do so, Call multi_queue_add_queue to add a new queue and get an ID to reference the queue. Then call multi_queue_register_next_handler_func to register a function which returns which queue's top priority function is to be executed at the time of the call. Use multi_queue_exec instead of exec to start execution loop. multi_queue_empty, multi_queue_size, and multi_queue_less_than are provided to help writing a next_handler_func.

See tests/pri_queue_tests.cpp for example uses.

greg7mdp commented 1 year ago

Hey Lin, I understand the need for multiple queues (and probably in the future multiple execution threads), but I think these are specific needs for nodeos, and it would be preferable to keep appbase generic.

I think there is a way to do that. The priority queue member (pri_queue) is accessed only in 4 locations in the code. What I suggest is encapsulating the enqueueing of tasks and their execution is an Executor class, and make application a templated class like:

template <class EXECUTOR>
class application {
...
}

and appbase would provide a default Executor class such as:

class Executor
{
public:
   template <typename Func>
   auto post( int priority, Func&& func ) {
      return boost::asio::post(io_serv, pri_queue.wrap(priority, std::forward<Func>(func)));
   }

   auto& get_priority_queue() {
      return pri_queue;
   }

   bool execute_highest() {
      return pri_queue.execute_highest();
   }

   void clear() {
      pri_queue.clear();
   }

private:
   execution_priority_queue pri_queue;
   boost::asio::io_service& io_serv;
};

When using appbase from nodeos, we would provide a different Executor which would include multiple queues and has a different execute_highest member function.

linh2931 commented 1 year ago

Thanks Greg for the detailed suggestion.

I was thinking about further decoupling from AppBase along a similar line and decided not doing so. The main reasons were 1). With built-in multiple queues support, other users of AppBase might benefit from it if they need to use. 2). If users need to manage queues directly, they would have to duplicate what execution_priority_queue does now. 3). The users need to manage io_service which is tricky. 4) nodeos would only use multiple queues mode only when multi-threaded read-only transaction execution is enabled.

I will look it further.

linh2931 commented 1 year ago

Thanks Lin! I still think it is important to decouple the way tasks are enqueued and executed on the appbase thread from the rest of the appbase functionality. By default, the users would not need to manage io_service anymore than they do today, as a default executor class would be provided.

The multi-queue is very specific to what you want to implement in nodeos. It is unlikely that another user would have the exact same need (we could expose it as an example though).

If we want appbase to remain a separate module, I think the decoupling I am suggesting is required.

Turned out more complicated than it appeared if simply making application as a templated class

template <class EXECUTOR>
class application {
...
}

All the code in application.cpp has to be refactored into application.hpp, which makes an otherwise clean application.hpp cluttered. Every instance of singleton call app() in nodeos would become something like app<nodeos_executor>().

linh2931 commented 1 year ago

Thanks Greg. Talked to Greg. Will continue with this route.

greg7mdp commented 1 year ago

All the code in application.cpp has to be refactored into application.hpp

Yes, that is true, no good way around it. Most of the STL including large complex things like unordered_map are like this. We can still keep the .hpp file nice looking I think.

linh2931 commented 1 year ago

Both Greg and I realized it was very complicated to template the application class (too many changes to AppBase and the user code and came up with similar idea to just template post and exec. Close this PR. A new one will be created.