Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft] Evict the longer time stamp subscription #37025

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Draft] Evict the longer time stamp subscription #37025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/app/InteractionModelEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,15 @@ bool InteractionModelEngine::TrimFabricForSubscriptions(FabricIndex aFabricIndex
{
candidate = handler;
}
// The last report time of this handler is longer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this the right logic? What is the actual behavior we are trying to implement?

If it's "evict the least newly created subscription", that's what the GetTransactionStartGeneration() thing above was trying to be a proxy for, but without adding new fields to ReadHandler, to avoid memory bloat. But if we are going to add new fields anyway, why wouldn't we just use those fields to track that information? But ideally we would not add fields.

If we really do want to evict handlers based on when they report, then this PR really needs to explain why that's a useful criterion, in the commit message or code comments (or maybe both).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not the right logic.
GetTransactionStartGeneration returns the generation recorded during the creation of the ReadHandler. If the least newly created subscription has the same generation and resource usage as others, it will be evicted when a new subscription is added. This pattern would repeat, with each subsequent new subscription causing the eviction of the least recently created one. As a result, only the least recently created subscription and those created before the handler became full would remain active until the generation updates. Is this the intended behavior?"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic evicts the oldest subscription that satisfies the other criteria, right?

The new code being added would evict the subscription that reported least recently, assuming things are all the same generation. But why is when a subscription reported a relevant thing at all?

Copy link
Contributor Author

@DejinChen DejinChen Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
It has to report once receiving a new subscription.
Maybe we should try not to fill the ReadHandler instead of modifying the evict mechanism?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by "try not to fill the ReadHandler"...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means not to fill the ReadHandler pool in the InteractionModel.

else if (handler->GetLastReportTime() < candidate->GetLastReportTime() &&
// Not older and the level of resource usage is the same (both exceed or neither exceed)
handler->GetTransactionStartGeneration() >= candidate->GetTransactionStartGeneration() &&
((attributePathsUsed > perFabricPathCapacity || eventPathsUsed > perFabricPathCapacity) ==
(candidateAttributePathsUsed > perFabricPathCapacity || candidateEventPathsUsed > perFabricPathCapacity)))
{
candidate = handler;
}
return Loop::Continue;
});

Expand Down
2 changes: 2 additions & 0 deletions src/app/ReadHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ ReadHandler::ReadHandler(ManagementCallback & apCallback, Messaging::ExchangeCon
mInteractionType = aInteractionType;
mLastWrittenEventsBytes = 0;
mTransactionStartGeneration = mManagementCallback.GetInteractionModelEngine()->GetReportingEngine().GetDirtySetGeneration();
mLastReportTime = System::SystemClock().GetMonotonicTimestamp();
mFlags.ClearAll();
SetStateFlag(ReadHandlerFlags::PrimingReports);

Expand Down Expand Up @@ -363,6 +364,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
{
mObserver->OnSubscriptionReportSent(this);
}
mLastReportTime = System::SystemClock().GetMonotonicTimestamp();
}
if (!aMoreChunks)
{
Expand Down
4 changes: 4 additions & 0 deletions src/app/ReadHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,8 @@ class ReadHandler : public Messaging::ExchangeDelegate

auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; }

auto GetLastReportTime() const { return mLastReportTime; }

/// @brief Forces the read handler into a dirty state, regardless of what's going on with attributes.
/// This can lead to scheduling of a reporting run immediately, if the min interval has been reached,
/// or after the min interval is reached if it has not yet been reached.
Expand Down Expand Up @@ -560,6 +562,8 @@ class ReadHandler : public Messaging::ExchangeDelegate
// When we don't have enough resources for a new subscription, the oldest subscription might be evicted by interaction model
// engine, the "oldest" subscription is the subscription with the smallest generation.
uint64_t mTransactionStartGeneration = 0;
// If not older, the subscription with the longest timestamp might be evicted.
System::Clock::Timestamp mLastReportTime;

SubscriptionId mSubscriptionId = 0;
uint16_t mMinIntervalFloorSeconds = 0;
Expand Down
Loading