From 2b5086fe877ef40ff62bff1171439b09c754848d Mon Sep 17 00:00:00 2001 From: Lily He Date: Tue, 31 Aug 2021 17:36:50 +0000 Subject: [PATCH] finagle-core: Offload early by default with OffloadFilter Problem: The toggle `com.twitter.finagle.OffloadEarly`, which moves the position of OffloadFilter to the top of the StackClient and the bottom of StackServer has been enabled for 6 months. Solution: Remove the toggle. The default behavior is to offload work to the offload pool early. JIRA Issues: CSL-11286 Differential Revision: https://phabricator.twitter.biz/D733526 --- CHANGELOG.rst | 4 +++ .../configs/com.twitter.finagle.core.json | 5 --- .../twitter/finagle/client/StackClient.scala | 35 ++++++------------- .../com/twitter/finagle/offloadEarly.scala | 11 ------ .../twitter/finagle/server/StackServer.scala | 18 ++-------- .../twitter/finagle/OffloadEarlyTest.scala | 20 +++-------- 6 files changed, 21 insertions(+), 72 deletions(-) delete mode 100644 finagle-core/src/main/scala/com/twitter/finagle/offloadEarly.scala diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e9c4bc95a3..4143dcf63c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,10 @@ Runtime Behavior Changes * finagle: Bump version of Jackson to 2.11.4. ``PHAB_ID=D727879`` +* finagle-core: OffloadFilter hands off work from Netty I/O thread to the offload CPU thread pool + right after we enter the Finagle stack by default. Previously this could be enabled via a toggle. + The `com.twitter.finagle.OffloadEarly` toggle has been removed. ``PHAB_ID=D733526`` + 21.8.0 (No 21.7.0 Release) -------------------------- diff --git a/finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json b/finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json index 1d51e577f3..d01eabf96e 100755 --- a/finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json +++ b/finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json @@ -1,10 +1,5 @@ { "toggles": [ - { - "id": "com.twitter.finagle.OffloadEarly", - "description": "Hand off work to the Offload pool right after we enter the Finagle stack", - "fraction": 0.0 - }, { "id": "com.twitter.finagle.context.MarshalledContextLookupId", "description": "Case insenstive broadcast context key lookups", diff --git a/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala b/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala index 0ee245af32..9ebeea2270 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala @@ -130,14 +130,7 @@ object StackClient { * @see [[com.twitter.finagle.filter.ExceptionSourceFilter]] * @see [[com.twitter.finagle.client.LatencyCompensation]] */ - def endpointStack[Req, Rep]: Stack[ServiceFactory[Req, Rep]] = - // temporary standard behaviour for those calling this API from outside - // of StackClient.scala - endpointStack(false) - - private def endpointStack[Req, Rep]( - shouldOffloadEarly: Boolean - ): Stack[ServiceFactory[Req, Rep]] = { + def endpointStack[Req, Rep]: Stack[ServiceFactory[Req, Rep]] = { // Ensure that we have performed global initialization. com.twitter.finagle.Init() @@ -146,17 +139,14 @@ object StackClient { */ val stk = new StackBuilder[ServiceFactory[Req, Rep]](nilStack[Req, Rep]) - if (shouldOffloadEarly) { - - /** - * `OffloadFilter` shifts future continuations (callbacks and - * transformations) off of IO threads into a configured `FuturePool`. - * This module is intentionally placed at the top of the stack - * such that execution context shifts as client's response leaves - * the stack and enters the application code. - */ - stk.push(OffloadFilter.client) - } + /** + * `OffloadFilter` shifts future continuations (callbacks and + * transformations) off of IO threads into a configured `FuturePool`. + * This module is intentionally placed at the top of the stack + * such that execution context shifts as client's response leaves + * the stack and enters the application code. + */ + stk.push(OffloadFilter.client) /** * `prepConn` is the bottom of the stack by definition. This position represents @@ -322,9 +312,7 @@ object StackClient { * as "module A is pushed after module B". */ - val shouldOffloadEarly = offloadEarly() || com.twitter.finagle.offload.auto() - - val stk = new StackBuilder(endpointStack[Req, Rep](shouldOffloadEarly)) + val stk = new StackBuilder(endpointStack[Req, Rep]) /* * These modules balance requests across cluster endpoints and @@ -488,9 +476,6 @@ object StackClient { stk.push(Failure.module) stk.push(ClientTracingFilter.module) stk.push(ForwardAnnotation.module) - if (!shouldOffloadEarly) { - stk.push(OffloadFilter.client) - } stk.push(RegistryEntryLifecycle.module) stk.push(ClientExceptionTracingFilter.module()) stk.push(TraceInitializerFilter.clientModule) diff --git a/finagle-core/src/main/scala/com/twitter/finagle/offloadEarly.scala b/finagle-core/src/main/scala/com/twitter/finagle/offloadEarly.scala deleted file mode 100644 index a317cd1012..0000000000 --- a/finagle-core/src/main/scala/com/twitter/finagle/offloadEarly.scala +++ /dev/null @@ -1,11 +0,0 @@ -package com.twitter.finagle - -import com.twitter.finagle.server.ServerInfo - -/** - * A toggle that determines whether or not offload happens early in the stack. - */ -private object offloadEarly { - private val toggle = CoreToggles("com.twitter.finagle.OffloadEarly") - def apply(): Boolean = toggle(ServerInfo().id.hashCode) -} diff --git a/finagle-core/src/main/scala/com/twitter/finagle/server/StackServer.scala b/finagle-core/src/main/scala/com/twitter/finagle/server/StackServer.scala index 89b5647600..50f1170eed 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/server/StackServer.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/server/StackServer.scala @@ -112,15 +112,6 @@ object StackServer { val stk = new StackBuilder[ServiceFactory[Req, Rep]](stack.nilStack[Req, Rep]) - val shouldOffloadEarly = offloadEarly() || com.twitter.finagle.offload.auto() - - if (!shouldOffloadEarly) { - // This module is placed at the bottom of the stack and shifts Future execution context - // from IO threads into a configured FuturePool right before user-defined Service.apply is - // being called. - stk.push(OffloadFilter.server) - } - stk.push(ServerTracingFilter.module) // this goes near the bottom of the stack so it is close to where Service.apply happens. @@ -165,12 +156,9 @@ object StackServer { // forks the execution if the current scheduler supports forking stk.push(ForkingSchedulerFilter.server) - - if (shouldOffloadEarly) { - // This module is placed at the top of the stack and shifts Future execution context - // from IO threads into a configured FuturePool right after Netty. - stk.push(OffloadFilter.server) - } + // This module is placed at the top of the stack and shifts Future execution context + // from IO threads into a configured FuturePool right after Netty. + stk.push(OffloadFilter.server) // The TraceInitializerFilter must be pushed after most other modules so that // any Tracing produced by those modules is enclosed in the appropriate diff --git a/finagle-core/src/test/scala/com/twitter/finagle/OffloadEarlyTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/OffloadEarlyTest.scala index a20d2fd010..0b13849446 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/OffloadEarlyTest.scala +++ b/finagle-core/src/test/scala/com/twitter/finagle/OffloadEarlyTest.scala @@ -19,23 +19,11 @@ class OffloadEarlyTest extends AnyFunSuite { assert(total - offloadAt < offloadAt) } - test("offload early is off by default in clients") { - offloadAtTheBottom(StackClient.newStack[Unit, Unit]) + test("offload early is on by default in clients") { + offloadAtTheTop(StackClient.newStack[Unit, Unit]) } - test("offload early is off by default in servers") { - offloadAtTheTop(StackServer.newStack[Unit, Unit]) - } - - test("a toggle moves offload filter earlier in clients") { - com.twitter.finagle.toggle.flag.overrides.let("com.twitter.finagle.OffloadEarly", 1.0) { - offloadAtTheTop(StackClient.newStack[Unit, Unit]) - } - } - - test("a toggle moves offload filter earlier in servers") { - com.twitter.finagle.toggle.flag.overrides.let("com.twitter.finagle.OffloadEarly", 1.0) { - offloadAtTheBottom(StackServer.newStack[Unit, Unit]) - } + test("offload early is on by default in servers") { + offloadAtTheBottom(StackServer.newStack[Unit, Unit]) } }