Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Workerd side of python isolate pool #2936

Open
wants to merge 1 commit into
base: dlapid/refactor_limitenforcer_workerobserver
Choose a base branch
from
Open

Workerd side of python isolate pool #2936

wants to merge 1 commit into from

Conversation

danlapid
Copy link
Contributor

No description provided.

@danlapid danlapid changed the title [WIP] python isolate pool [DRAFT] [WIP] python isolate pool Oct 16, 2024
hoodmane
hoodmane reviewed Oct 16, 2024
View reviewed changes
src/pyodide/BUILD.bazel Outdated Show resolved Hide resolved
@danlapid danlapid force-pushed the dlapid/python-pools2 branch 8 times, most recently from 7609106 to b3d8a00 Compare October 17, 2024 23:17
danlapid
danlapid commented Oct 17, 2024
View reviewed changes
src/pyodide/types/emscripten.d.ts
@@ -68,4 +68,8 @@ interface Module {
addRunDependency(x: string): void;
removeRunDependency(x: string): void;
noInitialRun: boolean;
setUnsafeEval(mod: typeof import('internal:unsafe-eval').default): void;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type imports must be here instead of an import statement because we use this file as an "ambient" type declaration.
See https://stackoverflow.com/questions/39040108/import-class-in-definition-file-d-ts

@danlapid danlapid force-pushed the dlapid/python-pools2 branch 2 times, most recently from 1deb2c2 to e4e608b Compare October 17, 2024 23:50
@danlapid danlapid force-pushed the dlapid/python-pools2 branch 8 times, most recently from bc1543a to 1a87fae Compare October 25, 2024 21:34
@danlapid danlapid force-pushed the dlapid/python-pools2 branch 2 times, most recently from 2e46939 to 05e7568 Compare October 30, 2024 23:36
@danlapid danlapid changed the base branch from main to dlapid/ownership_cleanups_for_pools October 30, 2024 23:38
danlapid
danlapid commented Oct 30, 2024
View reviewed changes
src/workerd/io/worker.h Outdated Show resolved Hide resolved
@danlapid danlapid requested review from a team as code owners November 8, 2024 00:06
@danlapid
Copy link
Contributor Author

danlapid commented Nov 8, 2024

Removing from draft stage as this is pretty much the end state of this PR.
Everything under src/workerd/jsg has been pulled into separate PRs so you can ignore that (it'll disappear when I rebase after those PRs are merged).
@hoodmane feel free to start reviewing Internal PR # 9006 is nearing a ready state and will be removed from draft soon as well so you can safely look at it to understand what the internal code is going to look like.

@danlapid danlapid changed the title [DRAFT] [WIP] python isolate pool Workerd side of python isolate pool Nov 8, 2024
@danlapid
Copy link
Contributor Author

danlapid commented Nov 8, 2024

@hoodmane this is a breaking change to the bundle, I'll need an explanation of how we can do that safely in a way that won't break backwards compatibility

hoodmane
hoodmane reviewed Nov 8, 2024
View reviewed changes
src/pyodide/BUILD.bazel Outdated Show resolved Hide resolved
src/pyodide/internal/python.ts Show resolved Hide resolved
@hoodmane
Copy link
Contributor

hoodmane commented Nov 8, 2024

this is a breaking change to the bundle, I'll need an explanation of how we can do that safely in a way that won't break backwards compatibility

You should be able to:

  1. release the new bundle as a patch version bump
  2. Update the patch version in compatibility-date.capnp in this PR

Then when we roll it out, the bundle patch version changes at the same time as the C++ code.

@danlapid danlapid force-pushed the dlapid/python-pools2 branch 2 times, most recently from 05ca004 to ef18feb Compare November 11, 2024 11:55
dom96
dom96 approved these changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, some small nits/questions, but otherwise LGTM.

src/pyodide/internal/pool/emscriptenSetup.ts Show resolved Hide resolved
src/workerd/api/pyodide/pyodide.c++
KJ_IF_SOME(module, emscriptenModule) {
return module.getHandle(js);
} else {
auto& runtime = KJ_ASSERT_NONNULL(workerd::Worker::Api::current().getEmscriptenRuntime());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The KJ_ASSERT_NONNULL below makes sense, but what about this one? Is it guaranteed for the return of this function to never be kj::none? If so, can its type be changed to not return a kj::Maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it can be guaranteed that if we have a python worker this HAS to be instantiated before we call getModule.
If for some reason someone called SetupEmscripten.getModule without an emscripten runtime this should definitely fail and there's now way to recover from it.
That being said, for any isolate that isn't a python isolate emscriptenRuntime will always be a kj::none and SetupEmscripten.getModule will simply never be called.

src/workerd/api/pyodide/setup-emscripten.c++
jsg::JsValue resolvePromise(jsg::Lock& js, jsg::JsValue prom) {
auto promise = KJ_ASSERT_NONNULL(prom.tryCast<jsg::JsPromise>());
if (promise.state() == jsg::PromiseState::PENDING) {
js.runMicrotasks();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can doing this block the thread for too long and prevent other isolates from doing work?

I guess we will run this in its own thread, so this won't be an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this logic would run in the global await that the loading of the pyodide bundle would block on. It's essentially the exact same logic just moved.
This means that we aren't blocking for any more time than we previously did but simply blocking at stage that's a bit earlier.

For workerd that doesn't matter at all.
For our internal use case it will maintain the original behaviour for any isolate that isn't part of the pool (yeah those still exist).
If the isolate is part of the pool then this will run in a separate thread and actually improve the current status.

src/workerd/server/workerd-api.c++ Outdated Show resolved Hide resolved
@danlapid danlapid force-pushed the dlapid/python-pools2 branch 8 times, most recently from b7fe651 to cf2324b Compare November 14, 2024 13:50
@danlapid danlapid changed the base branch from main to dlapid/refactor_limitenforcer_workerobserver November 14, 2024 14:12
@hoodmane
Copy link
Contributor

Changes to typescript look good to me.

@danlapid danlapid force-pushed the dlapid/refactor_limitenforcer_workerobserver branch 4 times, most recently from 9349b16 to f5bc217 Compare November 14, 2024 19:41
@danlapid
Instantiate Emscripten Runtime for python workers earlier.
317e535 
Move ownership of metrics and limitEnforcer to the api type.

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.

Add updateConfiguration function to jsg Isolates

This can be used to update the given configuration at runtime. Note that
while some jsg structs are lazily using the configuration, others can
use it at construction and will have the original configuration value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoodmane hoodmane hoodmane left review comments

@dom96 dom96 dom96 approved these changes

@erikcorry erikcorry Awaiting requested review from erikcorry erikcorry is a code owner automatically assigned from cloudflare/workers-runtime-1

Assignees
No one assigned
Labels
needs-internal-pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danlapid @hoodmane @dom96