Dan sliding window #42
Conversation
| void SlidingWindow<Message>::Send(int clientId, const Message& message) | ||
| { | ||
| if (mClientBuffers.count(clientId) == 0) | ||
| mClientBuffers.insert(std::pair<int, Buffer<Message> >(clientId, Buffer<Message>(mWindowConfig))); |
| if (mClientBuffers.count(clientId) == 0) | ||
| mClientBuffers.insert(std::pair<int, Buffer<Message> >(clientId, Buffer<Message>(mWindowConfig))); | ||
|
|
||
| Buffer<Message>& buffer = mClientBuffers.at(clientId); |
There was a problem hiding this comment.
You are doing the lookup twice
|
|
||
| private: | ||
| WindowConfig mWindowConfig; | ||
| std::map<int, Buffer<Message> > mClientBuffers; |
There was a problem hiding this comment.
if it doesnt need to be ordered, its better to use unordered_map
| public: | ||
| using value_type = Message; | ||
|
|
||
| SlidingWindow(const WindowConfig& conf); |
|
|
||
| SlidingWindow(const WindowConfig& conf); | ||
| void Send(int clientId, const Message& message); | ||
| std::vector<Message> DumpMessages(int clientId) const; |
There was a problem hiding this comment.
This is potentially a very expensive operation as you would copy all messages. A better patern is to visit elements (visitor).
| public: | ||
| using value_type = Message; | ||
|
|
||
| Buffer(const WindowConfig& conf); |
| }; | ||
|
|
||
| template<class Message> | ||
| class Buffer |
There was a problem hiding this comment.
I think that this Buffer is effectively a sliding window buffer, and the other Sliding Window class is the per user throttler
| bool CanAddMessage(std::chrono::time_point<std::chrono::steady_clock> currentTime) const; | ||
|
|
||
| WindowConfig mWindowConfig; | ||
| std::deque<TimestampedMessage<Message> > mTimestampedMessages; |
There was a problem hiding this comment.
Why not, although it is not mandatory to copy the messages themselves if you only want to throttle. You only need the timestamps. Depending on the type, the message can be quite big and expensive to copy.
|
|
||
| struct WindowConfig | ||
| { | ||
| std::chrono::seconds mDuration = std::chrono::seconds(10); |
There was a problem hiding this comment.
Glad you didnt use time_t - you can also use chrono literals to write = 10s
| mWindowConfig(conf) | ||
| { | ||
| if (conf.mMaxNumMessages <= 0) | ||
| throw std::runtime_error("Invalid configuration: max number of messages per time window must be positive!"); |
There was a problem hiding this comment.
Good that you check this, although a better place should be in WindowConfig's ctor.
No description provided.