Skip to content

Commit

Permalink
Remove Background state priority changes
Browse files Browse the repository at this point in the history 
Summary: These aren't used anywhere and we're going to start refactoring the priority implementation.

Reviewed By: jbeshay, hanidamlaj

Differential Revision: D68657305

fbshipit-source-id: e4a3a5a991bac99afef1362adb2663a50766d2c7
  • Loading branch information
@afrind @facebook-github-bot
afrind authored and facebook-github-bot committed Jan 28, 2025
1 parent ce3e9b0 commit e5bbd62
Show file tree
Hide file tree
Showing 8 changed files with 1 addition and 374 deletions.
42 changes: 0 additions & 42 deletions quic/api/QuicTransportBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,48 +824,6 @@ void QuicTransportBase::appendCmsgs(const folly::SocketCmsgMap& options) {
socket_->appendCmsgs(options);
}

void QuicTransportBase::setBackgroundModeParameters(
PriorityLevel maxBackgroundPriority,
float backgroundUtilizationFactor) {
backgroundPriorityThreshold_.assign(maxBackgroundPriority);
backgroundUtilizationFactor_.assign(backgroundUtilizationFactor);
conn_->streamManager->setPriorityChangesObserver(this);
onStreamPrioritiesChange();
}

void QuicTransportBase::clearBackgroundModeParameters() {
backgroundPriorityThreshold_.clear();
backgroundUtilizationFactor_.clear();
conn_->streamManager->resetPriorityChangesObserver();
onStreamPrioritiesChange();
}

// If backgroundPriorityThreshold_ and backgroundUtilizationFactor_ are set
// and all streams have equal or lower priority than the threshold (value >=
// threshold), set the connection's congestion controller to use background
// mode with the set utilization factor. In all other cases, turn off the
// congestion controller's background mode.
void QuicTransportBase::onStreamPrioritiesChange() {
if (conn_->congestionController == nullptr) {
return;
}
if (!backgroundPriorityThreshold_.hasValue() ||
!backgroundUtilizationFactor_.hasValue()) {
conn_->congestionController->setBandwidthUtilizationFactor(1.0);
return;
}
bool allStreamsBackground = conn_->streamManager->getHighestPriorityLevel() >=
backgroundPriorityThreshold_.value();
float targetUtilization =
allStreamsBackground ? backgroundUtilizationFactor_.value() : 1.0f;
VLOG(10) << fmt::format(
"Updating transport background mode. Highest Priority={} Threshold={} TargetUtilization={}",
conn_->streamManager->getHighestPriorityLevel(),
backgroundPriorityThreshold_.value(),
targetUtilization);
conn_->congestionController->setBandwidthUtilizationFactor(targetUtilization);
}

bool QuicTransportBase::checkCustomRetransmissionProfilesEnabled() const {
return quic::checkCustomRetransmissionProfilesEnabled(*conn_);
}
Expand Down
34 changes: 1 addition & 33 deletions quic/api/QuicTransportBase.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ namespace quic {
* of the object that holds it to send graceful close messages to the peer.
*/
class QuicTransportBase : public QuicSocket,
virtual public QuicTransportBaseLite,
QuicStreamPrioritiesObserver {
virtual public QuicTransportBaseLite {
public:
QuicTransportBase(
std::shared_ptr<QuicEventBase> evb,
Expand Down Expand Up @@ -170,23 +169,6 @@ class QuicTransportBase : public QuicSocket,
ApplicationErrorCode error,
folly::StringPiece errorMsg) override;

/*
* Set the background mode priority threshold and the target bw utilization
* factor to use when in background mode.
*
* If all streams have equal or lower priority compares to the threshold
* (value >= threshold), the connection is considered to be in background
* mode.
*/
void setBackgroundModeParameters(
PriorityLevel maxBackgroundPriority,
float backgroundUtilizationFactor);

/*
* Disable background mode by clearing all related parameters.
*/
void clearBackgroundModeParameters();

virtual void setQLogger(std::shared_ptr<QLogger> qLogger);

void setLoopDetectorCallback(std::shared_ptr<LoopDetectorCallback> callback) {
Expand Down Expand Up @@ -257,13 +239,6 @@ class QuicTransportBase : public QuicSocket,
}

protected:
/*
* Observe changes in stream priorities and handle background mode.
*
* Implements the QuicStreamPrioritiesObserver interface
*/
void onStreamPrioritiesChange() override;

folly::Expected<folly::Unit, LocalErrorCode> pauseOrResumeRead(
StreamId id,
bool resume);
Expand All @@ -282,13 +257,6 @@ class QuicTransportBase : public QuicSocket,

uint64_t qlogRefcnt_{0};

// Priority level threshold for background streams
// If all streams have equal or lower priority to the threshold
// (value >= threshold), the connection is considered to be in background
// mode.
Optional<PriorityLevel> backgroundPriorityThreshold_;
Optional<float> backgroundUtilizationFactor_;

private:
/**
* Helper to check if using custom retransmission profiles is feasible.
Expand Down
77 changes: 0 additions & 77 deletions quic/api/test/QuicTransportBaseTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4408,83 +4408,6 @@ TEST_P(QuicTransportImplTestBase, Cmsgs) {
transport->appendCmsgs(cmsgs);
}

TEST_P(QuicTransportImplTestBase, BackgroundModeChangeWithStreamChanges) {
// Verify that background mode is correctly turned on and off
// based upon stream creation, priority changes, stream removal.
// For different steps try local (uni/bi)directional streams and remote
// streams
InSequence s;
auto& conn = transport->getConnectionState();
auto mockCongestionController =
std::make_unique<NiceMock<MockCongestionController>>();
auto rawCongestionController = mockCongestionController.get();
conn.congestionController = std::move(mockCongestionController);
auto& manager = *conn.streamManager;
EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(_))
.Times(0); // Background params not set
auto stream = manager.createNextUnidirectionalStream().value();
manager.setStreamPriority(stream->id, Priority(1, false));

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(0.5))
.Times(1); // On setting the background params
transport->setBackgroundModeParameters(1, 0.5);

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(0.5))
.Times(1); // On removing a closed stream
stream->sendState = StreamSendState::Closed;
stream->recvState = StreamRecvState::Closed;
manager.removeClosedStream(stream->id);

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(0.5))
.Times(2); // On stream creation - create two streams - one bidirectional
auto stream2Id = manager.createNextUnidirectionalStream().value()->id;
auto stream3id = manager.createNextBidirectionalStream().value()->id;

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(1.0))
.Times(1); // On increasing the priority of one of the streams
manager.setStreamPriority(stream3id, Priority(0, false));

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(1.0))
.Times(1); // a new lower priority stream does not affect the utlization
// factor
auto streamLower = manager.createNextBidirectionalStream().value();

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(1.0))
.Times(1); // On removing a closed stream
streamLower->sendState = StreamSendState::Closed;
streamLower->recvState = StreamRecvState::Closed;
manager.removeClosedStream(streamLower->id);

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(0.5))
.Times(1); // On removing a closed stream
CHECK_NOTNULL(manager.getStream(stream3id))->sendState =
StreamSendState::Closed;
CHECK_NOTNULL(manager.getStream(stream3id))->recvState =
StreamRecvState::Closed;
manager.removeClosedStream(stream3id);

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(0.5))
.Times(1); // On stream creation - remote stream
auto peerStreamId = 20;
ASSERT_TRUE(isRemoteStream(conn.nodeType, peerStreamId));
auto stream4 = manager.getStream(peerStreamId);

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(1.0))
.Times(1); // On clearing the background parameters
transport->clearBackgroundModeParameters();

EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(_))
.Times(0); // Background params not set
stream4->sendState = StreamSendState::Closed;
stream4->recvState = StreamRecvState::Closed;
manager.removeClosedStream(stream4->id);
CHECK_NOTNULL(manager.getStream(stream2Id))->sendState =
StreamSendState::Closed;
CHECK_NOTNULL(manager.getStream(stream2Id))->recvState =
StreamRecvState::Closed;
manager.removeClosedStream(stream2Id);
}

class QuicTransportImplTestCounters : public QuicTransportImplTest {};

TEST_F(QuicTransportImplTestCounters, TransportResetClosesStreams) {
Expand Down
83 changes: 0 additions & 83 deletions quic/state/QuicStreamManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,17 +228,6 @@ bool QuicStreamManager::setStreamPriority(StreamId id, Priority newPriority) {
return false;
}
stream->priority = newPriority;
if (!stream->isControl) {
auto priorityMapEntry = streamPriorityLevelsNoCtrl_.find(id);
if (priorityMapEntry == streamPriorityLevelsNoCtrl_.end()) {
throw QuicTransportException(
"Active stream not in stream priority map",
TransportErrorCode::STREAM_STATE_ERROR);
} else {
priorityMapEntry->second = newPriority.level;
}
notifyStreamPriorityChanges();
}
updateWritableStreams(*stream);
writeQueue_.updateIfExist(id, stream->priority);
return true;
Expand Down Expand Up @@ -274,7 +263,6 @@ QuicStreamManager::getOrCreateOpenedLocalStream(StreamId streamId) {
throw QuicTransportException(
"Creating an active stream", TransportErrorCode::STREAM_STATE_ERROR);
}
addToStreamPriorityMap(it.first->second);
return &it.first->second;
}
return nullptr;
Expand Down Expand Up @@ -357,7 +345,6 @@ QuicStreamState* FOLLY_NULLABLE QuicStreamManager::instantiatePeerStream(
std::piecewise_construct,
std::forward_as_tuple(streamId),
std::forward_as_tuple(streamId, groupId, conn_));
addToStreamPriorityMap(it.first->second);
QUIC_STATS(conn_.statsCallback, onNewQuicStream);
return &it.first->second;
}
Expand Down Expand Up @@ -542,7 +529,6 @@ QuicStreamManager::createStream(
std::piecewise_construct,
std::forward_as_tuple(streamId),
std::forward_as_tuple(streamId, streamGroupId, conn_));
addToStreamPriorityMap(it.first->second);
QUIC_STATS(conn_.statsCallback, onNewQuicStream);
updateAppIdleState();
return &it.first->second;
Expand Down Expand Up @@ -575,15 +561,6 @@ void QuicStreamManager::removeClosedStream(StreamId streamId) {
windowUpdates_.erase(streamId);
stopSendingStreams_.erase(streamId);
flowControlUpdated_.erase(streamId);
if (!it->second.isControl) {
const auto streamPriorityIt = streamPriorityLevelsNoCtrl_.find(streamId);
if (streamPriorityIt == streamPriorityLevelsNoCtrl_.end()) {
throw QuicTransportException(
"Removed stream is not in the priority map",
TransportErrorCode::STREAM_STATE_ERROR);
}
streamPriorityLevelsNoCtrl_.erase(streamPriorityIt);
}
if (it->second.isControl) {
DCHECK_GT(numControlStreams_, 0);
numControlStreams_--;
Expand Down Expand Up @@ -632,7 +609,6 @@ void QuicStreamManager::removeClosedStream(StreamId streamId) {
}

updateAppIdleState();
notifyStreamPriorityChanges();
}

void QuicStreamManager::addToReadableStreams(const QuicStreamState& stream) {
Expand Down Expand Up @@ -738,7 +714,6 @@ void QuicStreamManager::setStreamAsControl(QuicStreamState& stream) {
if (!stream.isControl) {
stream.isControl = true;
numControlStreams_++;
streamPriorityLevelsNoCtrl_.erase(stream.id);
}
updateAppIdleState();
}
Expand All @@ -747,64 +722,6 @@ bool QuicStreamManager::isAppIdle() const {
return isAppIdle_;
}

PriorityLevel QuicStreamManager::getHighestPriorityLevel() const {
// Highest priority is minimum value
auto min = kDefaultMaxPriority;
for (auto& entry : streamPriorityLevelsNoCtrl_) {
if (entry.second < min) {
min = entry.second;
}
if (min == 0) {
break;
}
}
return min;
}

void QuicStreamManager::setPriorityChangesObserver(
QuicStreamPrioritiesObserver* observer) {
priorityChangesObserver_ = observer;
}

void QuicStreamManager::resetPriorityChangesObserver() {
if (!priorityChangesObserver_) {
return;
}
priorityChangesObserver_ = nullptr;
}

void QuicStreamManager::notifyStreamPriorityChanges() {
if (priorityChangesObserver_) {
priorityChangesObserver_->onStreamPrioritiesChange();
}
}

void QuicStreamManager::addToStreamPriorityMap(
const QuicStreamState& streamState) {
if (streamState.isControl) {
return;
}
auto entry = streamPriorityLevelsNoCtrl_.emplace(
streamState.id, PriorityLevel(streamState.priority.level));

// Verify stream didn't already exist in streamPriorityLevelsNoCtrl_
if (!entry.second) {
throw QuicTransportException(
"Attempted to add stream already in priority map",
TransportErrorCode::STREAM_STATE_ERROR);
}

// Verify inserted item
if (entry.first->second != PriorityLevel(streamState.priority.level)) {
throw QuicTransportException(
"Failed to add stream to priority map",
TransportErrorCode::STREAM_STATE_ERROR);
}

// Notify observer (if set)
notifyStreamPriorityChanges();
}

void QuicStreamManager::clearOpenStreams() {
QUIC_STATS_FOR_EACH(
streams().cbegin(),
Expand Down
Loading

0 comments on commit e5bbd62

Please sign in to comment.