Skip to content

Commit 7761c62

Browse files
Send setParserPaused straight to the end of the codec filter chain
Summary: Finishes the part of D120517427 that was dropped. Picks up the `setParserPaused` half of D122722345 by willwang1; the `getTransportDirection` half already landed in D120517427 through the cached codec traits. `HTTPSession::processReadData` calls `codec_.setParserPaused(false)` before every `onIngress`, and the call used to walk every filter in the chain, each one just forwarding `call_->setParserPaused(paused)`. Now `HTTPCodecFilterChain::setParserPaused` calls the codec at the end of the chain directly. To keep that correct, filters no longer take part in `setParserPaused`: - `HTTPCodecFilter` is now a class on top of `GenericFilter` with an empty `final` `setParserPaused`. No filter can override it, whatever it derives from, and a filter called directly ignores it. - `HTTPCodecFilterChain::add` and `addFilters` require `std::derived_from<Filter, HTTPCodecFilter>`, so a raw `GenericFilter` cannot get into the chain. `foreach` hands out `HTTPCodecFilter*`, so existing `FilterIteratorFn` callers are unchanged. - `chainEnd_` changes only in `setDestination`, so the chain always reaches the current codec. No filter in fbsource overrides `setParserPaused`, and nothing outside the chain calls it on a filter. Perf (from D122722345): Strobelight puts stacks through `PassThroughHTTPCodecFilter::setParserPaused` at about $148K a year fleet-wide, about 96% of it the forwarding itself. On proxygen/olb_internet, `HTTPCodecFilterChain::setParserPaused` is about 0.165% of CPU. Reviewed By: hanidamlaj Differential Revision: D123003021 fbshipit-source-id: ed2aeafd7c22d4ad4ba35cd469d5f7ede217dc8b
1 parent e580c8a commit 7761c62

3 files changed

Lines changed: 31 additions & 16 deletions

File tree

‎proxygen/lib/http/codec/HTTPCodecFilter.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,6 @@ bool PassThroughHTTPCodecFilter::isBusy() const {
147147
return call_->isBusy();
148148
}
149149

150-
void PassThroughHTTPCodecFilter::setParserPaused(bool paused) {
151-
call_->setParserPaused(paused);
152-
}
153-
154150
bool PassThroughHTTPCodecFilter::isParserPaused() const {
155151
return call_->isParserPaused();
156152
}

‎proxygen/lib/http/codec/HTTPCodecFilter.h‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,26 @@
88

99
#pragma once
1010

11+
#include <concepts>
12+
#include <memory>
1113
#include <proxygen/lib/http/codec/HTTPCodec.h>
1214
#include <proxygen/lib/utils/FilterChain.h>
1315

1416
namespace proxygen {
1517

16-
using HTTPCodecFilter = GenericFilter<HTTPCodec,
17-
HTTPCodec::Callback,
18-
&HTTPCodec::setCallback,
19-
true>;
18+
class HTTPCodecFilter
19+
: public GenericFilter<HTTPCodec,
20+
HTTPCodec::Callback,
21+
&HTTPCodec::setCallback,
22+
true> {
23+
public:
24+
using GenericFilter::GenericFilter;
25+
26+
// Filters are not allowed to change this: HTTPCodecFilterChain sends it
27+
// straight to the codec at the end of the chain.
28+
void setParserPaused(bool /* paused */) final {
29+
}
30+
};
2031

2132
/**
2233
* An implementation of HTTPCodecFilter that passes through all calls. This is
@@ -107,8 +118,6 @@ class PassThroughHTTPCodecFilter : public HTTPCodecFilter {
107118

108119
[[nodiscard]] bool isBusy() const override;
109120

110-
void setParserPaused(bool paused) override;
111-
112121
[[nodiscard]] bool isParserPaused() const override;
113122

114123
size_t onIngress(const folly::IOBuf& buf) override;
@@ -252,7 +261,7 @@ class HTTPCodecFilterChain {
252261
}
253262

254263
void setParserPaused(bool paused) {
255-
chain_->setParserPaused(paused);
264+
chain_.getChainEndPtr()->setParserPaused(paused);
256265
}
257266

258267
[[nodiscard]] const std::string& getUserAgent() const {
@@ -434,19 +443,20 @@ class HTTPCodecFilterChain {
434443
chain_.setCallback(callback);
435444
}
436445

437-
template <typename Filter, typename... Args>
446+
template <std::derived_from<HTTPCodecFilter> Filter, typename... Args>
438447
void add(Args&&... args) {
439448
chain_.add<Filter>(std::forward<Args>(args)...);
440449
}
441450

442-
template <typename... Filters>
443-
void addFilters(Filters&&... filters) {
444-
chain_.addFilters(std::forward<Filters>(filters)...);
451+
template <std::derived_from<HTTPCodecFilter>... Filters>
452+
void addFilters(std::unique_ptr<Filters>... filters) {
453+
chain_.addFilters(std::move(filters)...);
445454
}
446455

447456
template <typename Fn>
448457
void foreach (Fn&& fn) {
449-
chain_.foreach (std::forward<Fn>(fn));
458+
chain_.foreach (
459+
[&fn](auto* filter) { fn(static_cast<HTTPCodecFilter*>(filter)); });
450460
}
451461

452462
std::unique_ptr<HTTPCodec> setDestination(std::unique_ptr<HTTPCodec> dest) {

‎proxygen/lib/http/codec/test/FilterTests.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,15 @@ TEST_F(HTTPChecksTest, RecvTraceBody) {
265265
callbackStart_->onHeadersComplete(0, std::move(msg));
266266
}
267267

268+
TEST_F(HTTPChecksTest, SetParserPausedReachesChainEnd) {
269+
auto* newCodec = new MockHTTPCodec();
270+
auto oldCodec = chain_.setDestination(std::unique_ptr<HTTPCodec>(newCodec));
271+
EXPECT_CALL(*codec_, setParserPaused(_)).Times(0);
272+
EXPECT_CALL(*newCodec, setParserPaused(true));
273+
274+
chain_.setParserPaused(true);
275+
}
276+
268277
TEST_F(DebugFilterTest, NoError) {
269278
chain_.onIngress(*makeIOBuf("foo"));
270279
chain_.onIngressEOF();

0 commit comments

Comments
 (0)