Skip to content

Commit

Permalink
Move ownership of metrics and limitEnforcer to the api type.
Browse files Browse the repository at this point in the history
Currently ownership is shared even though the Isolate class encapsulates
the api class.
Moving complete ownership to the underlying api class allows the isolate
class to be constructed in a different scope to the api class.
This is useful for preinitialization of the api class before a request
has come in.
  • Loading branch information
Dan Lapid authored and danlapid committed Oct 30, 2024
1 parent 3dc10ba commit e347c69
Show file tree
Hide file tree
Showing 7 changed files with 79 additions and 44 deletions.
26 changes: 11 additions & 15 deletions src/workerd/io/worker.c++
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ struct Worker::Isolate::Impl {
metrics.gcEpilogue();
}

// Call limitEnforcer->exitJs(), and also schedule to call limitEnforcer->reportMetrics()
// Call limitEnforcer.exitJs(), and also schedule to call limitEnforcer.reportMetrics()
// later. Returns true if condemned. We take a mutable reference to it to make sure the caller
// believes it has exclusive access.
bool checkInWithLimitEnforcer(Worker::Isolate& isolate);
Expand Down Expand Up @@ -703,8 +703,6 @@ struct Worker::Isolate::Impl {
actorCacheLru(limitEnforcer.getActorCacheLruOptions()) {
jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) {
auto lock = api.lock(stackScope);
limitEnforcer.customizeIsolate(lock->v8Isolate);

if (inspectorPolicy != InspectorPolicy::DISALLOW) {
// We just created our isolate, so we don't need to use Isolate::Impl::Lock.
KJ_ASSERT(!isMultiTenantProcess(), "inspector is not safe in multi-tenant processes");
Expand Down Expand Up @@ -1002,21 +1000,19 @@ const HeapSnapshotDeleter HeapSnapshotDeleter::INSTANCE;
} // namespace

Worker::Isolate::Isolate(kj::Own<Api> apiParam,
kj::Own<IsolateObserver>&& metricsParam,
kj::StringPtr id,
kj::Own<IsolateLimitEnforcer> limitEnforcerParam,
InspectorPolicy inspectorPolicy,
ConsoleMode consoleMode)
: id(kj::str(id)),
limitEnforcer(kj::mv(limitEnforcerParam)),
api(kj::mv(apiParam)),
limitEnforcer(api->getLimitEnforcer()),
consoleMode(consoleMode),
featureFlagsForFl(makeCompatJson(decompileCompatibilityFlagsForFl(api->getFeatureFlags()))),
metrics(kj::mv(metricsParam)),
impl(kj::heap<Impl>(*api, *metrics, *limitEnforcer, inspectorPolicy)),
metrics(api->getMetrics()),
impl(kj::heap<Impl>(*api, metrics, limitEnforcer, inspectorPolicy)),
weakIsolateRef(WeakIsolateRef::wrap(this)),
traceAsyncContextKey(kj::refcounted<jsg::AsyncContextFrame::StorageKey>()) {
metrics->created();
metrics.created();
// We just created our isolate, so we don't need to use Isolate::Impl::Lock (nor an async lock).
jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) {
auto lock = api->lock(stackScope);
Expand Down Expand Up @@ -1272,7 +1268,7 @@ Worker::Script::Script(kj::Own<const Isolate> isolateParam,
modular(source.is<ModulesSource>()),
impl(kj::heap<Impl>()) {
this->isPython = false;
auto parseMetrics = isolate->metrics->parse(startType);
auto parseMetrics = isolate->metrics.parse(startType);
// TODO(perf): It could make sense to take an async lock when constructing a script if we
// co-locate multiple scripts in the same isolate. As of this writing, we do not, except in
// previews, where it doesn't matter. If we ever do co-locate multiple scripts in the same
Expand Down Expand Up @@ -1406,13 +1402,13 @@ kj::Own<const Worker::Isolate::WeakIsolateRef> Worker::Isolate::getWeakRef() con
}

Worker::Isolate::~Isolate() noexcept(false) {
metrics->teardownStarted();
metrics.teardownStarted();

// Update the isolate stats one last time to make sure we're accurate for cleanup in
// `evicted()`.
limitEnforcer->reportMetrics(*metrics);
limitEnforcer.reportMetrics(metrics);

metrics->evicted();
metrics.evicted();
weakIsolateRef->invalidate();

// Make sure to destroy things under lock. This lock should never be contended since the isolate
Expand All @@ -1421,7 +1417,7 @@ Worker::Isolate::~Isolate() noexcept(false) {
// worker destruction queue.
jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) {
Isolate::Impl::Lock recordedLock(*this, Worker::Lock::TakeSynchronously(kj::none), stackScope);
metrics->teardownLockAcquired();
metrics.teardownLockAcquired();
auto inspector = kj::mv(impl->inspector);
auto dropTraceAsyncContextKey = kj::mv(traceAsyncContextKey);
});
Expand Down Expand Up @@ -3793,7 +3789,7 @@ kj::Own<const Worker::Script> Worker::Isolate::newScript(kj::StringPtr scriptId,
}

void Worker::Isolate::completedRequest() const {
limitEnforcer->completedRequest(id);
limitEnforcer.completedRequest(id);
}

bool Worker::Isolate::isInspectorEnabled() const {
Expand Down
29 changes: 22 additions & 7 deletions src/workerd/io/worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,7 @@ class Worker::Isolate: public kj::AtomicRefcounted {
// session has one isolate which may load many iterations of the script (this allows the
// inspector session to stay open across them).
explicit Isolate(kj::Own<Api> api,
kj::Own<IsolateObserver>&& metrics,
kj::StringPtr id,
kj::Own<IsolateLimitEnforcer> limitEnforcer,
InspectorPolicy inspectorPolicy,
ConsoleMode consoleMode = ConsoleMode::INSPECTOR_ONLY);

Expand All @@ -284,8 +282,12 @@ class Worker::Isolate: public kj::AtomicRefcounted {
// Get the current Worker::Isolate from the current jsg::Lock
static const Isolate& from(jsg::Lock& js);

inline IsolateObserver& getMetrics() {
return metrics;
}

inline const IsolateObserver& getMetrics() const {
return *metrics;
return metrics;
}

inline kj::StringPtr getId() const {
Expand All @@ -299,8 +301,16 @@ class Worker::Isolate: public kj::AtomicRefcounted {
bool logNewScript = false,
kj::Maybe<ValidationErrorReporter&> errorReporter = kj::none) const;

inline IsolateLimitEnforcer& getLimitEnforcer() {
return limitEnforcer;
}

inline const IsolateLimitEnforcer& getLimitEnforcer() const {
return *limitEnforcer;
return limitEnforcer;
}

inline Api& getApi() {
return *api;
}

inline const Api& getApi() const {
Expand Down Expand Up @@ -390,16 +400,16 @@ class Worker::Isolate: public kj::AtomicRefcounted {
kj::Maybe<kj::Own<IsolateObserver::LockTiming>> lockTiming) const;

kj::String id;
kj::Own<IsolateLimitEnforcer> limitEnforcer;
kj::Own<Api> api;
IsolateLimitEnforcer& limitEnforcer;
ConsoleMode consoleMode;

// If non-null, a serialized JSON object with a single "flags" property, which is a list of
// compatibility enable-flags that are relevant to FL.
kj::Maybe<kj::String> featureFlagsForFl;

kj::Own<IsolateObserver> metrics;
TeardownFinishedGuard<IsolateObserver&> teardownGuard{*metrics};
IsolateObserver& metrics;
TeardownFinishedGuard<IsolateObserver&> teardownGuard{metrics};

struct Impl;
kj::Own<Impl> impl;
Expand Down Expand Up @@ -517,6 +527,11 @@ class Worker::Api {
virtual jsg::JsObject wrapExecutionContext(
jsg::Lock& lock, jsg::Ref<api::ExecutionContext> ref) const = 0;

virtual IsolateLimitEnforcer& getLimitEnforcer() = 0;
virtual const IsolateLimitEnforcer& getLimitEnforcer() const = 0;
virtual IsolateObserver& getMetrics() = 0;
virtual const IsolateObserver& getMetrics() const = 0;

// Set the module fallback service callback, if any.
using ModuleFallbackCallback = kj::Maybe<kj::OneOf<kj::String, jsg::ModuleRegistry::ModuleInfo>>(
jsg::Lock& js,
Expand Down
8 changes: 4 additions & 4 deletions src/workerd/server/server.c++
Original file line number Diff line number Diff line change
Expand Up @@ -2940,15 +2940,15 @@ kj::Own<Server::Service> Server::makeWorker(kj::StringPtr name,
*observer, conf, featureFlags.asReader(), pythonConfig);
}

auto api = kj::heap<WorkerdApi>(globalContext->v8System, featureFlags.asReader(), *limitEnforcer,
kj::atomicAddRef(*observer), *memoryCacheProvider, pythonConfig, kj::mv(newModuleRegistry));
auto api =
kj::heap<WorkerdApi>(globalContext->v8System, featureFlags.asReader(), kj::mv(limitEnforcer),
kj::mv(observer), *memoryCacheProvider, pythonConfig, kj::mv(newModuleRegistry));
auto inspectorPolicy = Worker::Isolate::InspectorPolicy::DISALLOW;
if (inspectorOverride != kj::none) {
// For workerd, if the inspector is enabled, it is always fully trusted.
inspectorPolicy = Worker::Isolate::InspectorPolicy::ALLOW_FULLY_TRUSTED;
}
auto isolate = kj::atomicRefcounted<Worker::Isolate>(kj::mv(api), kj::mv(observer), name,
kj::mv(limitEnforcer), inspectorPolicy,
auto isolate = kj::atomicRefcounted<Worker::Isolate>(kj::mv(api), name, inspectorPolicy,
conf.isServiceWorkerScript() ? Worker::ConsoleMode::INSPECTOR_ONLY : consoleMode);

// If we are using the inspector, we need to register the Worker::Isolate
Expand Down
41 changes: 33 additions & 8 deletions src/workerd/server/workerd-api.c++
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ static const PythonConfig defaultConfig{
struct WorkerdApi::Impl final {
kj::Own<CompatibilityFlags::Reader> features;
kj::Maybe<kj::Own<jsg::modules::ModuleRegistry>> maybeOwnedModuleRegistry;
kj::Own<IsolateLimitEnforcer> limitEnforcer;
kj::Own<IsolateObserver> observer;
JsgWorkerdIsolate jsgIsolate;
api::MemoryCacheProvider& memoryCacheProvider;
const PythonConfig& pythonConfig;
Expand All @@ -157,17 +159,24 @@ struct WorkerdApi::Impl final {

Impl(jsg::V8System& v8System,
CompatibilityFlags::Reader featuresParam,
IsolateLimitEnforcer& limitEnforcer,
kj::Own<jsg::IsolateObserver> observer,
kj::Own<IsolateLimitEnforcer> limitEnforcerParam,
kj::Own<IsolateObserver> observerParam,
api::MemoryCacheProvider& memoryCacheProvider,
const PythonConfig& pythonConfig = defaultConfig,
kj::Maybe<kj::Own<jsg::modules::ModuleRegistry>> newModuleRegistry = kj::none)
: features(capnp::clone(featuresParam)),
maybeOwnedModuleRegistry(kj::mv(newModuleRegistry)),
jsgIsolate(
v8System, Configuration(*this), kj::mv(observer), limitEnforcer.getCreateParams()),
limitEnforcer(kj::mv(limitEnforcerParam)),
observer(kj::atomicAddRef(*observerParam)),
jsgIsolate(v8System,
Configuration(*this),
kj::mv(observerParam),
limitEnforcer->getCreateParams()),
memoryCacheProvider(memoryCacheProvider),
pythonConfig(pythonConfig) {}
pythonConfig(pythonConfig) {
jsgIsolate.runInLockScope(
[&](JsgWorkerdIsolate::Lock& lock) { limitEnforcer->customizeIsolate(lock.v8Isolate); });
}

static v8::Local<v8::String> compileTextGlobal(
JsgWorkerdIsolate::Lock& lock, capnp::Text::Reader reader) {
Expand Down Expand Up @@ -209,14 +218,14 @@ struct WorkerdApi::Impl final {

WorkerdApi::WorkerdApi(jsg::V8System& v8System,
CompatibilityFlags::Reader features,
IsolateLimitEnforcer& limitEnforcer,
kj::Own<jsg::IsolateObserver> observer,
kj::Own<IsolateLimitEnforcer> limitEnforcer,
kj::Own<IsolateObserver> observer,
api::MemoryCacheProvider& memoryCacheProvider,
const PythonConfig& pythonConfig,
kj::Maybe<kj::Own<jsg::modules::ModuleRegistry>> newModuleRegistry)
: impl(kj::heap<Impl>(v8System,
features,
limitEnforcer,
kj::mv(limitEnforcer),
kj::mv(observer),
memoryCacheProvider,
pythonConfig,
Expand Down Expand Up @@ -266,6 +275,22 @@ jsg::JsObject WorkerdApi::wrapExecutionContext(
kj::downcast<JsgWorkerdIsolate::Lock>(lock).wrap(lock.v8Context(), kj::mv(ref)));
}

IsolateLimitEnforcer& WorkerdApi::getLimitEnforcer() {
return *impl->limitEnforcer;
}

const IsolateLimitEnforcer& WorkerdApi::getLimitEnforcer() const {
return *impl->limitEnforcer;
}

IsolateObserver& WorkerdApi::getMetrics() {
return *impl->observer;
}

const IsolateObserver& WorkerdApi::getMetrics() const {
return *impl->observer;
}

Worker::Script::Source WorkerdApi::extractSource(kj::StringPtr name,
config::Worker::Reader conf,
Worker::ValidationErrorReporter& errorReporter,
Expand Down
8 changes: 6 additions & 2 deletions src/workerd/server/workerd-api.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ class WorkerdApi final: public Worker::Api {
public:
WorkerdApi(jsg::V8System& v8System,
CompatibilityFlags::Reader features,
IsolateLimitEnforcer& limitEnforcer,
kj::Own<jsg::IsolateObserver> observer,
kj::Own<IsolateLimitEnforcer> limitEnforcer,
kj::Own<IsolateObserver> observer,
api::MemoryCacheProvider& memoryCacheProvider,
const PythonConfig& pythonConfig,
kj::Maybe<kj::Own<jsg::modules::ModuleRegistry>> newModuleRegistry);
Expand All @@ -55,6 +55,10 @@ class WorkerdApi final: public Worker::Api {
jsg::Lock& lock) const override;
jsg::JsObject wrapExecutionContext(
jsg::Lock& lock, jsg::Ref<api::ExecutionContext> ref) const override;
IsolateLimitEnforcer& getLimitEnforcer() override;
const IsolateLimitEnforcer& getLimitEnforcer() const override;
IsolateObserver& getMetrics() override;
const IsolateObserver& getMetrics() const override;

static Worker::Script::Source extractSource(kj::StringPtr name,
config::Worker::Reader conf,
Expand Down
10 changes: 3 additions & 7 deletions src/workerd/tests/test-fixture.c++
Original file line number Diff line number Diff line change
Expand Up @@ -319,21 +319,17 @@ TestFixture::TestFixture(SetupParams&& params)
httpOverCapnpFactory,
byteStreamFactory,
false),
isolateLimitEnforcer(kj::heap<MockIsolateLimitEnforcer>()),
errorReporter(kj::heap<MockErrorReporter>()),
memoryCacheProvider(kj::heap<api::MemoryCacheProvider>(*timer)),
api(kj::heap<server::WorkerdApi>(testV8System,
params.featureFlags.orDefault(CompatibilityFlags::Reader()),
*isolateLimitEnforcer,
kj::heap<MockIsolateLimitEnforcer>(),
kj::atomicRefcounted<IsolateObserver>(),
*memoryCacheProvider,
defaultPythonConfig,
kj::none)),
workerIsolate(kj::atomicRefcounted<Worker::Isolate>(kj::mv(api),
kj::atomicRefcounted<IsolateObserver>(),
scriptId,
kj::mv(isolateLimitEnforcer),
Worker::Isolate::InspectorPolicy::DISALLOW)),
workerIsolate(kj::atomicRefcounted<Worker::Isolate>(
kj::mv(api), scriptId, Worker::Isolate::InspectorPolicy::DISALLOW)),
workerScript(kj::atomicRefcounted<Worker::Script>(kj::atomicAddRef(*workerIsolate),
scriptId,
server::WorkerdApi::extractSource(mainModuleName,
Expand Down
1 change: 0 additions & 1 deletion src/workerd/tests/test-fixture.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ struct TestFixture {
ThreadContext::HeaderIdBundle threadContextHeaderBundle;
capnp::HttpOverCapnpFactory httpOverCapnpFactory;
ThreadContext threadContext;
kj::Own<IsolateLimitEnforcer> isolateLimitEnforcer;
kj::Own<Worker::ValidationErrorReporter> errorReporter;
kj::Own<api::MemoryCacheProvider> memoryCacheProvider;
kj::Own<Worker::Api> api;
Expand Down

0 comments on commit e347c69

Please sign in to comment.