[Pubsub] reduce memory usage for channels that do not require total memory cap #23985
Conversation
mwtian
added
the
tests-ok
src/ray/pubsub/publisher.h
Outdated
|
|
||
| /// State for an entity that streams published messages to subscribers, with total size | ||
| /// cap. | ||
| class StreamEntityState : public EntityState { |
There was a problem hiding this comment.
I think "Stream" is not really the right word here since they both send streams to the subscribers. Maybe something like "CappedEntityState" vs "BufferedEntityState"?
Also, could you update the comment to explain what happens when we exceed the cap and to explain when Basic vs Streamed should be used/why we have two different kinds?
There was a problem hiding this comment.
Good point. Renamed to CappedEntityState and added comment on their differences.
| @@ -90,24 +101,32 @@ const absl::flat_hash_map<SubscriberID, SubscriberState *> &EntityState::Subscri | |||
| return subscribers_; | |||
| } | |||
|
|
|||
| SubscriptionIndex::SubscriptionIndex(rpc::ChannelType channel_type) | |||
There was a problem hiding this comment.
nit: I feel it's a little bit wired to put channel_type into SubscriptionIndex. it seems that it's only used to construct EntityState. We don't need to store it inside channel_type_.
There was a problem hiding this comment.
Another thing I feel bad about is that it's an application layer decision, but here it's hardcoded in the infra layer. I'm wondering whether we can make it better?
There was a problem hiding this comment.
synced offline. I'm good with this right now since it's still maintainable.
Why are these changes needed?
In a1e06f6, memory bound was added for each subscribed entity in the publisher. It adds two extra
std::dequeper subscribed entity, which turns out to cost a lot more memory when there are a large number ofObjectRefs: #23853 (comment)This PR avoids the extra memory usage for entities in channels unlikely to grow too large, i.e. all channels except those for logs and error info. Subscribed entity memory usage no longer shows up in the memory profile when there are 1M object refs:
Raw data: profile006.pb.gz
Related issue number
#23604
Checks
scripts/format.shto lint the changes in this PR.