Marc-André Lureau <marcandre.lur...@gmail.com> writes: > Hi > > On Fri, Sep 25, 2015 at 4:00 PM, Markus Armbruster <arm...@redhat.com> wrote: >> VSERPORT_CHANGE is emitted when the guest opens or closes a >> virtio-serial port. The event's member "id" identifies the port. >> >> When several events arrive quickly, throttling drops all but the last >> of them. Because of that, a QMP client must assume that *any* port >> may have changed state when it receives a VSERPORT_CHANGE event and >> throttling may have happened. >> >> Make the event more useful by throttling it for each port separately. >> >> Marc-André recently posted 'monitor: throttle VSERPORT_CHANGED by >> "id"' to do the same thing. Why am I redoing his work? Let me >> explain. >> >> In master, event throttling works per QAPIEvent. Throttling state is >> kept in MonitorQAPIEventState monitor_qapi_event_state[]. Going from >> event to throttling state is a simple array subscript. >> >> Marc-André's series makes the code governing throttling pluggable per >> event. MonitorQAPIEventState gets two new members, the governor >> function and an opaque. It loses the actual throttling state. That >> one's now in new type MonitorQAPIEventPending. >> >> The standard governor has the opaque point to a single >> MonitorQAPIEventPending. >> >> The governor for VSERPORT_CHANGED has the opaque point to a hash table >> mapping "id" to MonitorQAPIEventPending. >> >> In my review, I challenged the complexity of this solution, but >> Marc-André thinks it's justified. That leaves me two choices: concede >> the point, or come up with something that's actually simpler. >> >> My solution doesn't make anything pluggable. Instead, I simply >> generalize the map from event to throttling state. Instead of using >> just the QAPIEvent as key for looking up state, I permit additional >> use of event-specific data. For VSERPORT_CHANGED, I additionally use >> "id". monitor_qapi_event_state[] becomes a hash table. >> >> No callbacks, no type-punning, less code, and fewer code paths to >> test. > > I dislike the approach to put all event filters in the same hashtable. > The simple case doesn't need it, and I kept it that way. But > certainly, it shorten a bit the code. Since it is probably not > performance critical, it doesn't matter, so you may prefer less code.
Yes, I do prefer simpler code. Fewer bugs, cheaper to maintain. The performance differences aren't entirely obvious. Sure, going through a hash table is slower than a simple array subscript. But your indirections aren't free, either. Mispredicted indirect calls, in particular. > I would still argue that it's cleaner and faster the way I proposed. > ymmv [...]
