Skip to content

Commit abdcb4a

Browse files
Catherine Gasniermeta-codesync[bot]
authored andcommitted
Give Glean derive methods a dedicated HIGH-priority thread pool
Summary: High-level goal: run multiple indexing jobs on the same Glean write server. Right now, when two indexing jobs run concurrently, the first job that reaches the derive stage will have its derive requests waiting in thrift queues behind many write requests, making the sandcastle jobs possibly retry and issuing duplicate derive requests. To fix that, the goal of this diff is to assign higher priority to derive requests. To that end: - Tag `deriveStored`/`deriveStoredV2` with `thrift.Priority{level = thrift.RpcPriority.HIGH}` in `glean.thrift`. - Size the HIGH pool to 10 threads on the Glean server instead of the default 2. Sizing the pool needs a way to configure the priority thread manager on an hsthrift `CppServer`. This adds a first-class `cpuPriorityPoolSizes` field to hsthrift's `ServerOptions`: a list of per-priority pool-size overrides keyed off the canonical `Thrift.Protocol.RpcOptions.Types.Priority` enum. Priorities not listed keep the fbthrift default. The overrides are marshalled to `c_create_cpp_server`, which applies them via `setThreadManagerPoolSizes`. To avoid duplicating fbthrift's default pool sizing, `PriorityThreadManager` now exposes `defaultThreadCounts(normalThreadsCount)` as the single source of those defaults; `newPriorityThreadManager(size_t)` delegates to it, and hsthrift starts from it and applies only the requested overrides. Notes: - Priority pools are a `CppServer`-only feature, so the Glean override is compiled out of the HTTP (OSS) build. - Existing `ServerOptions` consumers are unaffected: the new field defaults to `[]` (fbthrift's normal sizing) and they build options via `defaultOptions { ... }`. Reviewed By: bochko Differential Revision: D114073952 fbshipit-source-id: a0e6fca4b8fcb828bdd1bdbd999e1958ec51873c
1 parent b18f4a2 commit abdcb4a

4 files changed

Lines changed: 47 additions & 1 deletion

File tree

server/Thrift/Server/CppServer.hs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,14 +102,25 @@ withBackgroundServer' handler postProcess ServerOptions{..} action =
102102
priorities =
103103
fromIntegral . fromThriftEnum . methodPriority <$> Map.elems infos
104104

105+
-- CPU worker pool size overrides, split into parallel arrays of priority
106+
-- ordinal and size for the C++ side. Priorities not listed keep the
107+
-- fbthrift default.
108+
poolOverridePriorities =
109+
fromIntegral . fromThriftEnum . fst <$> cpuPriorityPoolSizes
110+
poolOverrideSizes = fromIntegral . snd <$> cpuPriorityPoolSizes
111+
numPoolOverrides = fromIntegral (length cpuPriorityPoolSizes)
112+
105113
-- Creates a PServer to run `act` on
106114
withCServer cb act =
107115
useTextsAsCStringLens names $ \names_ptr names_sizes names_len ->
108116
withArray priorities $ \priorities_ptr ->
109-
withArray oneways $ \oneways_pts -> do
117+
withArray oneways $ \oneways_pts ->
118+
withArray poolOverridePriorities $ \pool_prios_ptr ->
119+
withArray poolOverrideSizes $ \pool_sizes_ptr -> do
110120
let
111121
alloc =
112122
let create = create_cpp_server cb factoryFn cPort cNumWorkers
123+
pool_prios_ptr pool_sizes_ptr numPoolOverrides
113124
priorities_ptr oneways_pts
114125
names_ptr names_sizes names_len
115126
in
@@ -150,6 +161,9 @@ foreign import ccall safe "c_create_cpp_server"
150161
-> FactoryFunction
151162
-> CInt -- ^ port
152163
-> CInt -- ^ workers, or 0 to use the default
164+
-> Ptr CInt -- ^ pool size override priorities
165+
-> Ptr CSize -- ^ pool size override sizes
166+
-> CSize -- ^ number of pool size overrides
153167
-> Ptr CInt -- ^ method priorities array
154168
-> Ptr Bool -- ^ one way methods
155169
-> Ptr CString -- ^ method names array

server/Thrift/Server/Types.hs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import Control.Exception hiding (handle)
1919
import Data.Text (Text)
2020
import Foreign.Ptr
2121

22+
import Thrift.Protocol.RpcOptions.Types (Priority)
23+
2224
-- -----------------------------------------------------------------------------
2325
-- Factory function
2426

@@ -47,6 +49,11 @@ data ServerOptions = ServerOptions
4749
, customFactoryFn :: Maybe FactoryFunction
4850
-- ^ whether a custom factory should be used
4951
, customModifyFn :: Maybe ModifyFunction
52+
, cpuPriorityPoolSizes :: [(Priority, Int)]
53+
-- ^ CPU worker pool size overrides, per Thrift RPC priority. Priorities
54+
-- not listed keep the fbthrift default (see
55+
-- @PriorityThreadManager::defaultThreadCounts@): the number of CPU cores
56+
-- for NORMAL and an implementation-defined number for the rest.
5057
}
5158

5259
-- | Takes the `onewayFunctions'` from your thrift Service instance
@@ -56,6 +63,7 @@ defaultOptions = ServerOptions
5663
, numWorkerThreads = Nothing
5764
, customFactoryFn = Nothing
5865
, customModifyFn = Nothing
66+
, cpuPriorityPoolSizes = []
5967
}
6068

6169
-- -----------------------------------------------------------------------------

server/cpp/CppServer.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include <folly/Memory.h>
1515
#include <glog/logging.h>
1616

17+
#include <thrift/lib/cpp/concurrency/ThreadManager.h>
1718
#include <thrift/lib/cpp2/server/ThriftServer.h>
1819

1920
#include "cpp/Destructible.h"
@@ -138,6 +139,9 @@ CreateCppServerResult* c_create_cpp_server(
138139
apache::thrift::TFactory factoryFn,
139140
int desiredPort,
140141
int workers,
142+
const int* poolSizeOverridePriorities,
143+
const size_t* poolSizeOverrideSizes,
144+
size_t numPoolSizeOverrides,
141145
const apache::thrift::concurrency::PRIORITY* methodPriorities,
142146
const bool* methodOneways,
143147
const char** methodNames,
@@ -173,6 +177,25 @@ CreateCppServerResult* c_create_cpp_server(
173177
cppServer->setNumCPUWorkerThreads(workers);
174178
}
175179

180+
if (numPoolSizeOverrides > 0) {
181+
// Start from PriorityThreadManager's own defaults and override only the
182+
// priorities the caller asked for, so we never duplicate or drift from
183+
// the default pool sizing.
184+
auto poolSizes = apache::thrift::concurrency::PriorityThreadManager::
185+
defaultThreadCounts(cppServer->getNumCPUWorkerThreads());
186+
for (size_t i = 0; i < numPoolSizeOverrides; i++) {
187+
const int priority = poolSizeOverridePriorities[i];
188+
if (priority >= 0 &&
189+
priority <
190+
static_cast<int>(apache::thrift::concurrency::N_PRIORITIES)) {
191+
poolSizes[priority] = poolSizeOverrideSizes[i];
192+
}
193+
}
194+
cppServer->setThreadManagerType(
195+
apache::thrift::ThriftServer::ThreadManagerType::PRIORITY);
196+
cppServer->setThreadManagerPoolSizes(poolSizes);
197+
}
198+
176199
return new CreateCppServerResult(HsLeft, std::move(cppServer));
177200
} catch (const std::exception& e) {
178201
auto exStr = e.what();

server/test/ServerTest.hs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ portAlreadyBoundTest pname protId =
132132
, numWorkerThreads = Just 2
133133
, customFactoryFn = Nothing
134134
, customModifyFn = Nothing
135+
, cpuPriorityPoolSizes = []
135136
}
136137
headerConfig :: HeaderConfig t
137138
headerConfig = mkHeaderConfig port protId

0 commit comments

Comments
 (0)