Conversation
The buffer middleware is parameterized on the inner service's repsonse future, which "leaks" the inner service's type to the outer components (which slows down compilations). This changes the signature to instead be parameterized by the response type, boxing the inner service's response future to avoid exposing its type signature.
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
commented
Jun 29, 2020
Comment on lines
-241
to
-243
| // Prevent the cache's lock from being acquired in poll_ready, ensuring this happens | ||
| // in the response future. This prevents buffers from holding the cache's lock. | ||
| .push_oneshot() |
Contributor
Author
There was a problem hiding this comment.
the oneshots here were only necessary due to implementation details of Cache (acquiring the lock in poll_ready), so they are now removeable.
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
commented
Jun 29, 2020
| .push(metrics.stack.layer(stack_labels("profile"))), | ||
| ), | ||
| ) | ||
| .spawn_buffer(buffer_capacity) |
Contributor
Author
There was a problem hiding this comment.
i wasn't sure if we wanted to put an idle timeout here or not. i think we only want that at the request level?
Member
There was a problem hiding this comment.
i think that's right -- because the idle timeout is part of cache eviction. i'll give this a closer look though.
Signed-off-by: Eliza Weisman <[email protected]>
This way, we don't traverse the whole cache in the fast path. Signed-off-by: Eliza Weisman <[email protected]>
…y into eliza/buffer-cache
olix0r
approved these changes
Jul 1, 2020
kleimkuhler
approved these changes
Jul 1, 2020
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jul 2, 2020
This release increases the default buffer size to match the proxy's in-flight request limit. This reduces contention in overload--especially high-concurrency--situations, substantially reducing tail latency. --- * update test-support clients and servers to be natively async (linkerd/linkerd2-proxy#580) * Print build diagnostics in docker (linkerd/linkerd2-proxy#583) * update test controllers to std::future/Tonic; remove threads (linkerd/linkerd2-proxy#585) * buffer: Box the inner service's reponse future (linkerd/linkerd2-proxy#586) * Eliminate Bind & Listen traits (linkerd/linkerd2-proxy#584) * cache: replace Lock with Buffer (linkerd/linkerd2-proxy#587)
cpretzer
pushed a commit
to linkerd/linkerd2
that referenced
this pull request
Jul 2, 2020
This release increases the default buffer size to match the proxy's in-flight request limit. This reduces contention in overload--especially high-concurrency--situations, substantially reducing tail latency. --- * update test-support clients and servers to be natively async (linkerd/linkerd2-proxy#580) * Print build diagnostics in docker (linkerd/linkerd2-proxy#583) * update test controllers to std::future/Tonic; remove threads (linkerd/linkerd2-proxy#585) * buffer: Box the inner service's reponse future (linkerd/linkerd2-proxy#586) * Eliminate Bind & Listen traits (linkerd/linkerd2-proxy#584) * cache: replace Lock with Buffer (linkerd/linkerd2-proxy#587)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The use of a buffer hides some of the type complexity of the inner
service (all of the type complexity, after #586). However, Linkerd
currently only uses buffers at the
Servicelevel, not at theMakeServicelevel. SinceMakeService/NewServices tend to begeneric over both inner
MakeServicetypes and over the producedServices (as well as potentially two future types), they probably havemuch longer concrete types than
Services in most cases.As @olix0r suggested, we can replace the
Lockthat's currently used toensure exclusive access to
Caches (which are at theMakeService)swith
Buffers. TheLockis currently essentially doing somethingquite similar to adding a
Bufferanyway. Introducing buffers aroundall caches erases the inner type for everything layered around the
cache, which should make overall type length much shorter.
This seems to have a fairly noticeable impact on build time and memory
use (see linkerd/linkerd2#4676). On my machine, running
make dockeron#586 gets SIGKILLed (presumably by the OOM killer) after 7m53s. After
this change,
make dockercompletes successfully after 1m44s.Also, the
linkerd2-lockcrate can be removed, as it was used only byCache.To work around increased tail latencies when the buffer fills up, this
branch also sets the default buffer capacity to be equal to the
default concurrency limit, rather than 10. This is fine, since the buffer
capacity isn't actually what enforces a bound on proxy memory use.
The spawned tasks waiting on a full buffer are still sitting on the Tokio
executor, and it's actually the in flight limit that stops us from accepting
any more requests when we have too many in flight.
Depends on #586.
Fixes linkerd/linkerd2#4676.
Signed-off-by: Eliza Weisman [email protected]