google/vulkan-performance-layers

Introduce the event dispatch abstraction

Closed this issue · 2 comments

It is good to separate the function which creates an event from the function that triggers the logger(s) for that event. This is beneficial when we want to add support for a new format, change the current output format for a set of existing events, etc. Consider we have the CSV logger and add support for the Trace Event format. In this case we need to find all the call sites of the CSV logger and change the code so that it uses the newly introduced logger. This is too much change and it's error prone as we might miss some sections. One of the alternatives is to have a dispatcher that forwards an event from its creator to a logger(s) registered for that event. It basically maps the event names to a vector of loggers. This way, when we add a new format and want to use it for a specific type of event, we only append it to the logger vector of the event.
The dispatcher has two public interfaces, HandleEvent() and RegisterHandler(). The first one receives an event and invokes the loggers for that event. The second one adds a logger to the loggers of an event.

flowchart LR
    EventCreator--Event-->Dispatcher
    subgraph Dispatcher
      direction LR
      EventType -.-> EventLogger*      
    end
    Dispatcher -- Event --> EventLogger
Loading

A sample implementation would like this:

class Dispatcher {
 public:
  // Invokes the loggers registered for this event.
  void HanldeEvent(Event *event) {
  }
  // Adds the logger to the event's logger vector.
  void RegisterLogger(EventLogger* logger, std::string event_name) {
    ...
  }

 private:
  std::map<std::string, std::vector<EventLogger*> > name_to_logger_vector;

}
kuhar commented

Thanks for taking care of the issues! @miladHakimi can you add a comment to briefly explain why this is not longer needed?

In the initial design of the EventLogger, I didn't know that each layer has its own instance of LayerData. I assumed that the LayerData is a shared object among all the layers and should contain all the Loggers. In that scenario, we needed a mechanism in the LayerData to forward the incoming events to their corresponding loggers. I thought using a key-value map (Dispatcher) seemed like a good idea.
However, since each layer has its own instance of LayerData, it can have its logger in its LayerData. The Events created by a layer will be passed to the layer's logger residing in its LayerData. Hence, there is no need for keeping track of the origins/destinations of the events and the Dispatcher.
This is why I closed this issue.