runtime: do not share runtime snapshot between worker threads#18601
runtime: do not share runtime snapshot between worker threads#18601lkpdn wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
Currently, every worker thread accesses the shared_ptr of the runtime snapshot. When the data plane frequently needs to do runtime feature checking under heavy load, the resource contention becomes a scaling bottleneck. The created snapshot does not need to be shared in that way. This patch eliminates the scaling bottleneck. Signed-off-by: Koichiro Den <den@valinux.co.jp>
|
Hi @lkpdn, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
To see why this PR was factored out from #18485, please refer to the original thread #18485 (comment). |
rojkov
left a comment
rojkov
left a comment
There was a problem hiding this comment.
Thanks! Added a couple of comments.
| static void parseEntryFractionalPercentValue(Entry& entry); | ||
|
|
||
| const std::vector<OverrideLayerConstPtr> layers_; | ||
| const std::shared_ptr<std::vector<OverrideLayerConstPtr>> layers_; |
There was a problem hiding this comment.
| const std::shared_ptr<std::vector<OverrideLayerConstPtr>> layers_; | |
| const std::shared_ptr<const std::vector<OverrideLayerConstPtr>> layers_; |
Also since values_ do not mutate outside the constructor it would be nice to make them const too and to set them in the initializer list (with a private static method).
| std::shared_ptr<SnapshotImpl> ptr = createNewSnapshot(); | ||
| tls_->set([ptr](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { | ||
| return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(ptr); | ||
| return std::make_shared<SnapshotImpl>(ptr.get()); |
There was a problem hiding this comment.
So, now instead of one shared snapshot we create and keep a copy of values_ for every worker to avoid contention on incref'ing a shared_ptr. This should work, but seems suboptimal memory-wise and adds latency upon creating a new snapshot.
Could you also try @mattklein123 's suggestion to return a reference to the content of the TLS slot, not a copy of shared_ptr, and check if it works? After looking at the flamegraph (btw, thanks! it helps a lot) I think this could be achieved by changing Loader's interface:
- virtual SnapshotConstSharedPtr threadsafeSnapshot() PURE;
+ virtual const Snapshot& threadsafeSnapshot() PURE;and its implementation accordingly. Exactly like you described in the original PR.
This blog post explains the RCU-like semantics.
There was a problem hiding this comment.
Yeah ^ is what I was suggesting we try just to make sure we understand the problem. I think many/all callsites never store the shared_ptr and just reference the snapshot. This might be a very nice perf win across the board.
There was a problem hiding this comment.
@rojkov @mattklein123 Thank you so much! I'll try and check the resulting performance in detail later.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
5 participants
Commit Message:
Currently, every worker thread accesses the shared_ptr of the runtime snapshot. When the data plane frequently needs to do runtime feature checking under heavy load, the resource contention becomes a scaling bottleneck. The created snapshot does not need to be shared in that way. This patch eliminates the scaling bottleneck.
An alternative way to resolve the contention may be to change LoaderImpl::threadsafeSnapshot so that it never touch refcount of the original shared_ptr to a new snapshot, and returns a plain reference instead of SnapshotConstSharedPtr. (#18485 (comment) )
This is a fllow-up PR of #18485.
Ref: #18600
Additional Description: n/a
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]