FEAT: Single CC MPLB #500
FEAT: Single CC MPLB #500
Conversation
ArtyomPeshkov
left a comment
ArtyomPeshkov
left a comment
There was a problem hiding this comment.
Move AckInfo to utils and add on_ack overload
Think about MPLB packet distribution (delays management)
PaulRalnikov
force-pushed
the
single-cc-mplb
branch
from
d0fe102 to
b9a6f09
Compare
| flow_weak.lock()->send( | ||
| std::vector<PacketInfo>(1, std::move(packet_info))); | ||
| }); | ||
| m_packets_in_flight++; |
There was a problem hiding this comment.
m_packets_in_flight increases but does not decrease anywhere
There was a problem hiding this comment.
Fixed with your suggestion abobe
| std::weak_ptr<SingleCCMplb> mplb_weak = shared_from_this(); | ||
| PacketCallback packet_callback = [observer, mplb_weak, | ||
| flow_weak](PacketAckInfo info) { | ||
| observer->on_single_callback(); |
There was a problem hiding this comment.
If the CallbackObserver calls the final callback() on the last packet, the custom callback may be triggered before the CC/delivered/inflight update of the last packet. This can break tests/logic if the user immediately looks at get_context() in the callback.
Please check it
There was a problem hiding this comment.
Good one, will be changed
| // returns true if congestion detected; false otherwice | ||
| virtual void on_ack(TimeNs rtt, TimeNs avg_rtt, bool ecn_flag) = 0; | ||
|
|
||
| virtual void on_ack(PacketAckInfo info) { |
There was a problem hiding this comment.
| virtual void on_ack(PacketAckInfo info) { | |
| virtual void on_ack(const PacketAckInfo& info) { |
There was a problem hiding this comment.
It is not significant here - size of PacketAckInfo is too camll (just two doubles + bool)
| packet_info.packet_size; | ||
| } | ||
| flow_weak.lock()->send( | ||
| std::vector<PacketInfo>(1, std::move(packet_info))); |
There was a problem hiding this comment.
vector have this signature - vector(size_type count, const T& value);
Suggestion:
std::vector<PacketInfo> packets;
packets.push_back(std::move(packet_info));
flow->send(std::move(packets));
There was a problem hiding this comment.
I think your variant is abbigious, but mine is not perfect too. I will replace it with std::vector<PacktInfo>({packet_info})
| if (mplb_weak.expired()) { | ||
| LOG_ERROR( | ||
| "MPLB pointer expired; could not increase sent data " | ||
| "size"); |
There was a problem hiding this comment.
No, we may use just code section flow_weak.lock()->send(...) if mplb pointer is expired
| #include "connection/flow/tcp/i_tcp_cc.hpp" | ||
|
|
||
| namespace test { | ||
| class CCMock : public sim::ITcpCC { |
| : m_flows(a_flows) {}; | ||
|
|
||
| virtual std::shared_ptr<sim::INewFlow> choose_flow() final { | ||
| return *m_flows.begin(); |
There was a problem hiding this comment.
Its just a mock so its not so important
Co-authored-by: Alexey Zharkov <62723911+Az3git@users.noreply.github.com>
closes #496