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

[DRAFT] [WIP] python isolate pool #2936

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[DRAFT] [WIP] 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
] + glob(
[
"internal/*.ts",
"internal/topLevelEntropy/*.ts",
# The pool directory is only needed by typescript, it shouldn't be used at runtime.
"internal/pool/*.ts",
# "internal/pool/*.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should run tsc --noEmit on this directory to make sure that our types are correct. Probably alongside the esbuild command, since esbuild by itself ignores types.

@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 force-pushed the dlapid/ownership_cleanups_for_pools branch 2 times, most recently from 6dd801a to a9fd4fc Compare October 31, 2024 00:11
Base automatically changed from dlapid/ownership_cleanups_for_pools to main October 31, 2024 14:40
@danlapid danlapid mentioned this pull request Nov 6, 2024
Open
@danlapid danlapid force-pushed the dlapid/python-pools2 branch 2 times, most recently from af0eecf to aa23635 Compare November 6, 2024 13:05
@danlapid danlapid mentioned this pull request Nov 6, 2024
Merged
@danlapid danlapid force-pushed the dlapid/python-pools2 branch 7 times, most recently from e661e30 to 264d94b Compare November 7, 2024 12:22
danlapid
danlapid commented Nov 7, 2024
View reviewed changes
src/workerd/server/workerd-api.c++
@@ -128,6 +129,92 @@ static const PythonConfig defaultConfig{
};
} // namespace

namespace {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for code review:
No changes to anything in this namespace scope, this is only a move from below because I need these functions earlier in the file

@danlapid
Instantiate Emscripten Runtime for python workers earlier.
712733a 
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.
@danlapid danlapid marked this pull request as ready for review November 8, 2024 00:06
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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

@dom96 dom96 Awaiting requested review from dom96

At least 1 approving review is required to merge this pull request.

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

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