Buffer: Implement tracking for BufferMemoryAccounts in WatermarkFactory.#17093
Buffer: Implement tracking for BufferMemoryAccounts in WatermarkFactory.#17093htuch merged 16 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/assign @antoniovicente |
antoniovicente
left a comment
There was a problem hiding this comment.
Here's some initial feedback. Looks like a good start.
/wait
| * | ||
| * @param reason the reason for reseting the stream. | ||
| */ | ||
| virtual void resetStream(Http::StreamResetReason reason) PURE; |
There was a problem hiding this comment.
Consider changing the name to resetDownstream(reason) or resetDownstreamDueToOverload()
There was a problem hiding this comment.
Changed to resetDownstream
| /** | ||
| * Unregister a buffer memory account. | ||
| */ | ||
| virtual void unregisterAccount(const BufferMemoryAccountSharedPtr& account) PURE; |
There was a problem hiding this comment.
I don't think these two methods need to be part of the general factory interface. I think accounts can depend on the concrete implementation in order to do a more optimized account balance update only in cases where there's a significant change in account balance which requires moving the account from one bucket to another. By having some of the update logic in the account proper we can optimize away unnecessary calls to update the balance.
There was a problem hiding this comment.
SGTM. Moved the BufferMemoryAccountImpl to the WatermarkBuffer impl's file, since it's the factory that produces that object. This inturn allows us to more easily couple between the two, while external classes use there interfaces.
| hdrs = ["buffer_impl.h"], | ||
| deps = [ | ||
| "//envoy/buffer:buffer_interface", | ||
| "//envoy/http:codec_interface", |
There was a problem hiding this comment.
I think this depends on stream_reset_handler_interface, not codec_interface
There was a problem hiding this comment.
Whoops, must of got mixed up when breaking this out. Done.
| ++class_idx; | ||
| } | ||
|
|
||
| return class_idx; |
There was a problem hiding this comment.
Consider the following version. Also, consider that class_idx in your implementation is simply the location of the most significant bit that is set. See std::bit_width or std::countl_zero or the abseil equivalents (since I think c++20 is not allowed yet) for optimized O(1) MSB computation functions.
int class_idx = absl::bit_width(shifted_balance);
return std::min(class_idx, size_class_account_sets_.size() - 1);
| done_notification.WaitForNotification(); | ||
| } | ||
|
|
||
| void TrackedWatermarkBufferFactory::inspectMemoryClasses( |
There was a problem hiding this comment.
function seems unused.
There was a problem hiding this comment.
Used in test/common/buffer/buffer_memory_account_test.cc
| }; | ||
|
|
||
| using WatermarkBufferPtr = std::unique_ptr<WatermarkBuffer>; | ||
| using MemoryClassesToAccountsSet = std::array<absl::flat_hash_set<BufferMemoryAccountSharedPtr>, 8>; |
There was a problem hiding this comment.
Please move type definition to the protected section below, just before it is used.
using MemoryClassesToAccountsSet = std::array<absl::flat_hash_set<BufferMemoryAccountSharedPtr>, 8>;
MemoryClassesToAccountsSet size_class_account_sets_;
|
|
||
| protected: | ||
| // Enable subclasses to inspect the mapping. | ||
| MemoryClassesToAccountsSet size_class_account_sets_; |
There was a problem hiding this comment.
Verify in WatermarkBufferFactory destructor that size_class_account_sets_ is empty
abstract interfaces coupling the concrete impls. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
KBaichoo
left a comment
There was a problem hiding this comment.
Thanks for the feedback @antoniovicente, addressed most of your comments
| * | ||
| * @param reason the reason for reseting the stream. | ||
| */ | ||
| virtual void resetStream(Http::StreamResetReason reason) PURE; |
There was a problem hiding this comment.
Changed to resetDownstream
| done_notification.WaitForNotification(); | ||
| } | ||
|
|
||
| void TrackedWatermarkBufferFactory::inspectMemoryClasses( |
There was a problem hiding this comment.
Used in test/common/buffer/buffer_memory_account_test.cc
|
|
||
| protected: | ||
| // Enable subclasses to inspect the mapping. | ||
| MemoryClassesToAccountsSet size_class_account_sets_; |
| }; | ||
|
|
||
| using WatermarkBufferPtr = std::unique_ptr<WatermarkBuffer>; | ||
| using MemoryClassesToAccountsSet = std::array<absl::flat_hash_set<BufferMemoryAccountSharedPtr>, 8>; |
| ++class_idx; | ||
| } | ||
|
|
||
| return class_idx; |
| hdrs = ["buffer_impl.h"], | ||
| deps = [ | ||
| "//envoy/buffer:buffer_interface", | ||
| "//envoy/http:codec_interface", |
There was a problem hiding this comment.
Whoops, must of got mixed up when breaking this out. Done.
| /** | ||
| * Unregister a buffer memory account. | ||
| */ | ||
| virtual void unregisterAccount(const BufferMemoryAccountSharedPtr& account) PURE; |
There was a problem hiding this comment.
SGTM. Moved the BufferMemoryAccountImpl to the WatermarkBuffer impl's file, since it's the factory that produces that object. This inturn allows us to more easily couple between the two, while external classes use there interfaces.
| return BufferMemoryAccountImpl::createAccount(this, reset_handler); | ||
| } | ||
|
|
||
| void WatermarkBufferFactory::onAccountBalanceUpdate(const BufferMemoryAccountSharedPtr& account, |
There was a problem hiding this comment.
Would be worried about lifetime issues that might occur from this.
| uint64_t prior_balance) { | ||
|
|
||
| int prev_idx = accountBalanceToClassIndex(prior_balance); | ||
| int new_idx = accountBalanceToClassIndex(account->balance()); |
There was a problem hiding this comment.
Now use something like updateAccountClass.
cases. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
Please address the magic number issues /wait |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
| /** | ||
| *@return the balance of the account. | ||
| */ | ||
| virtual uint64_t balance() const PURE; |
There was a problem hiding this comment.
I see no non-test uses of this API method. Consider removing from the interface.
| ASSERT(reset_handler_ != nullptr); | ||
| reset_handler_ = nullptr; | ||
|
|
||
| factory_->unregisterAccount(shared_this_, current_bucket_idx_); |
There was a problem hiding this comment.
current_bucket_idx_ does not contain the right value after unregistration, please add:
current_bucket_idx_ = -1;
| if (shared_this_ && new_class != current_bucket_idx_) { | ||
| factory_->updateAccountClass(shared_this_, current_bucket_idx_, new_class); | ||
| current_bucket_idx_ = new_class; | ||
| } |
There was a problem hiding this comment.
The previous 5 lines are repeated in the charge method below. Consider refactoring to an updateAccountClass() private method which you can call in the two places.
| * | ||
| * @param reason the reason for reseting the stream. | ||
| */ | ||
| virtual void resetDownstream(Http::StreamResetReason reason) PURE; |
There was a problem hiding this comment.
nit: reset reason could be omitted, resetting via this interface implies that the termination is related to overload.
| uint64_t shifted_balance = buffer_memory_allocated_ >> 20; // shift by 1MB. | ||
|
|
||
| if (shifted_balance == 0) { | ||
| return -1; // Not worth tracking anything < 1MB. |
There was a problem hiding this comment.
You're not allowing for a lot of configuration for this accounting. Will we need some configuration in the future?
Note that the recommended stream buffer limits for edge configurations are around 64kb. Tracking high memory connections at 1MB or larger leaves a fairly wide gap of ~16x before this mechanism can terminate expensive streams. It seems very unlikely that we'll have streams using >1MB given the 64kb edge config stream limit.
There was a problem hiding this comment.
We will need some configuration of this probably. Shall I do it in this PR or a follow up? Plumbing from the bootstrap to the watermark factory might require a good amount of changes.
I'm thinking that exposing the starting_bucket_size which is rounded to nearest power of two should be sufficient, as with 8 buckets the largest bucket will be 128x the smallest. WDYT?
TODO(kbaichoo): Either implement in this PR or follow up PR.
There was a problem hiding this comment.
I would consider requiring that the config be powers of two. I do think that the addition of config will force you to undo some of the micro optimizations in this component.
| // and shared_this_. | ||
| static BufferMemoryAccountSharedPtr createAccount(WatermarkBufferFactory* factory, | ||
| Http::StreamResetHandler* reset_handler); | ||
| ~BufferMemoryAccountImpl() override { ASSERT(buffer_memory_allocated_ == 0); } |
There was a problem hiding this comment.
ASSERT(reset_handler_ == nullptr); despite it being impossible for this to not be the case given use of shared_this_.
|
|
||
| void resetDownstream(Http::StreamResetReason reason) override { | ||
| if (reset_handler_ != nullptr) { | ||
| reset_handler_->resetStream(reason); |
There was a problem hiding this comment.
You may be making an assumption that reset_handler_->resetStream(reason); calls clearDownstream()
You may run into some problems in the watermark factory while iterating through accounts and calling resetDownstream if some implementation doesn't invoke clearDownstream() from resetStream. I'm fairly sure that some of the stream implementations like HTTP2 do not call clearDownstream() from resetStream since they use the deferred delete mechanism provided by the dispatcher. Another potential danger is you calling reset_handler_->resetStream(); on a stream that is waiting to be deleted via deferred deletion. If the stream doesn't correctly handle the case by ignoring the resetStream we may trigger a crash.
There was a problem hiding this comment.
Great point. Will have it such that resetStream(reason) always calls clearDownstream() to avoid situations where a stream might be reset for a non-OM situation, and is not yet destroyed, but Envoy Overload resets it again.
To avoid the deferred deletion issue whenever the stream is added to a zombie stream list, make sure that clearDownstream has been called.
TODO(kbaichoo): implement.
| // be called, but the downstream stream isn't going to terminate soon | ||
| // such as StreamDecoderFilterCallbacks::recreateStream(). | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); |
There was a problem hiding this comment.
Doing this from the destructor may be too late and result in calls to resetStream after deferred deletion started. Consider hooking into StreamImpl::destroy which is called before scheduling the deferred deletion.
There was a problem hiding this comment.
Great catch! Yes, this should happen in destroy s.t. we unlink the stream from the account at that point as we don't want it to reset the stream at that point.
| // be called, but the downstream stream isn't going to terminate soon | ||
| // such as StreamDecoderFilterCallbacks::recreateStream(). | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); |
There was a problem hiding this comment.
See comment in the H2 codec, it's not clear to me if this happens early enough.
There was a problem hiding this comment.
You are correct, the QUIC streams go into a deferred deletion mechanism that periodically is cleared by an Envoy QUIC alarm. Will instead call clearDownstream when the stream is added to the list of zombie streams that no data should be re-transmitted for it.
TODO(kbaichoo): implement
interface cleanups. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
by clearing downstream when writeSideCloses (the stream shouldn't have additional writes on it). Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
Anything else @antoniovicente ? Thanks |
|
|
||
| // Clear the downstream on the account since we're resetting the downstream. | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); |
There was a problem hiding this comment.
Consider doing this before calling the base class method for safety.
| // Clear the downstream on the account since we're resetting the downstream. | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); | ||
| } |
There was a problem hiding this comment.
clear before calling base class method may make sense. But isn't this covered by the call in ServerStreamImpl::destroy()?
There was a problem hiding this comment.
Now call clear before the base class method.
StreamImpl::resetStream can sometimes defer the reset (if end stream already but not end stream sent). By having this in the SeverStreamImpl::resetStream, we know the BufferFactory won't try to reset the stream again since clearing downstream unregister's the account.
| } | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); | ||
| } |
There was a problem hiding this comment.
Consider doing this before reset callbacks.
derived classes for safety. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
KBaichoo
left a comment
There was a problem hiding this comment.
Thanks for the review @antoniovicente
|
|
||
| // Clear the downstream on the account since we're resetting the downstream. | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); |
| } | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); | ||
| } |
| // Clear the downstream on the account since we're resetting the downstream. | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); | ||
| } |
There was a problem hiding this comment.
Now call clear before the base class method.
StreamImpl::resetStream can sometimes defer the reset (if end stream already but not end stream sent). By having this in the SeverStreamImpl::resetStream, we know the BufferFactory won't try to reset the stream again since clearing downstream unregister's the account.
antoniovicente
left a comment
There was a problem hiding this comment.
Thanks for the improvement. Off to senior-maintainers for final review.
/assign-from @envoyproxy/senior-maintainers
| // be called, but the downstream stream isn't going to terminate soon | ||
| // such as StreamDecoderFilterCallbacks::recreateStream(). | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); |
There was a problem hiding this comment.
Also move before base class call
|
@envoyproxy/senior-maintainers assignee is @htuch |
| * invoke to reset the stream. | ||
| * @return a BufferMemoryAccountSharedPtr of the newly created account. | ||
| */ | ||
| virtual BufferMemoryAccountSharedPtr createAccount(Http::StreamResetHandler* reset_handler) PURE; |
There was a problem hiding this comment.
Nit: StreamResetHandler& (unless optional?)
| } | ||
| } | ||
|
|
||
| // The number of memory classes the Account expects to exists. |
There was a problem hiding this comment.
Where is the memory class size scheme explained?
There was a problem hiding this comment.
Added additional comments to WatermarkBufferFactory and BufferMemoryAccountImpl::balanceToClassIndex
| downstream_request_account = std::make_shared<Buffer::BufferMemoryAccountImpl>(); | ||
| // Create account, wiring the stream to use it. | ||
| auto& buffer_factory = read_callbacks_->connection().dispatcher().getWatermarkFactory(); | ||
| downstream_request_account = buffer_factory.createAccount(&response_encoder.getStream()); |
There was a problem hiding this comment.
Very dumb question - why are we talking about a downstream request account but all the wiring is happening for the response encoder?
There was a problem hiding this comment.
The primary owner of the account right is the server-side(e.i. downstream) stream. Perhaps we should change "request" to "stream" in this variable name.
There's a desire to split the accounts used for the request and response directions, but have both of those accounts associated primarily with the downstream stream. But that change will happen in a future PR.
There was a problem hiding this comment.
I've changed it to downstream_stream_account as suggested.
document WatermarkBufferFactory tracking. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
KBaichoo
left a comment
There was a problem hiding this comment.
Thanks for the reviews @antoniovicente and @htuch.
| // be called, but the downstream stream isn't going to terminate soon | ||
| // such as StreamDecoderFilterCallbacks::recreateStream(). | ||
| if (buffer_memory_account_) { | ||
| buffer_memory_account_->clearDownstream(); |
| } | ||
| } | ||
|
|
||
| // The number of memory classes the Account expects to exists. |
There was a problem hiding this comment.
Added additional comments to WatermarkBufferFactory and BufferMemoryAccountImpl::balanceToClassIndex
| * invoke to reset the stream. | ||
| * @return a BufferMemoryAccountSharedPtr of the newly created account. | ||
| */ | ||
| virtual BufferMemoryAccountSharedPtr createAccount(Http::StreamResetHandler* reset_handler) PURE; |
| downstream_request_account = std::make_shared<Buffer::BufferMemoryAccountImpl>(); | ||
| // Create account, wiring the stream to use it. | ||
| auto& buffer_factory = read_callbacks_->connection().dispatcher().getWatermarkFactory(); | ||
| downstream_request_account = buffer_factory.createAccount(&response_encoder.getStream()); |
There was a problem hiding this comment.
I've changed it to downstream_stream_account as suggested.
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
verified examples kept failing, but merging in #17549 fixed it. think this is ready for merge, so I can put this follow up: KBaichoo#105 for review. |
…ry. (envoyproxy#17093) This PR tracks memory accounts using >1MB of allocated space, with feedback mechanisms based on credits and debits on accounts. It further creates the handle from which the BufferMemoryAccount can reset the stream, and has the WatermarkBufferFactory also produce the particular BufferMemoryAccountImpl used for tracking. Risk Level: Medium Testing: Unit and Integration test Docs Changes: NA Release Notes: NA -- not yet user facing Platform Specific Features: NA Runtime guard: Yes, envoy.test_only.per_stream_buffer_accounting from envoyproxy#16218 sufficient Related Issue envoyproxy#15791 Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo kbaichoo@google.com
Commit Message: Implement Tracking for BufferMemoryAccounts in Watermark Factory.
Additional Description: This PR tracks memory accounts using >1MB of allocated space, with feedback mechanisms based on credits and debits on accounts. It further creates the handle from which the BufferMemoryAccount can reset the stream, and has the
WatermarkBufferFactoryalso produce the particularBufferMemoryAccountImplused for tracking.Risk Level: Medium
Testing: Unit and Integration test
Docs Changes: NA
Release Notes: NA -- not yet user facing
Platform Specific Features: NA
Runtime guard: Yes, envoy.test_only.per_stream_buffer_accounting from #16218 sufficient
Related Issue #15791