Skip to content

Commit

Permalink
Implement PackagePromiseMap in a way that avoids storing kj::Promises…
Browse files Browse the repository at this point in the history
… on the v8 isolate heap. Note that we can't just wrap the promise in an IoOwn because we don't yet have an IoContext when the promise is created
  • Loading branch information
garrettgu10 committed Sep 4, 2024
1 parent a13a503 commit b5dbcc6
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 2 deletions.
39 changes: 39 additions & 0 deletions src/workerd/api/pyodide/pyodide.c++
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,43 @@ bool hasPythonModules(capnp::List<server::config::Worker::Module>::Reader module
return false;
}

void PackagePromiseMap::insert(
kj::String path, kj::Promise<kj::Own<SmallPackagesTarReader>> promise) {
KJ_ASSERT(waitlists.find(kj::str(path)) == kj::none);
waitlists.insert(kj::str(path), CrossThreadWaitList());

promise
.then([this, p = kj::mv(path)](auto a) {
auto path = kj::str(p);
auto maybeWaitlist = waitlists.find(p);
KJ_IF_SOME(waitlist, maybeWaitlist) {
fetchedPackages.insert(kj::mv(path), kj::mv(a));
waitlist.fulfill();
} else {
JSG_FAIL_REQUIRE(Error, "Failed to get waitlist for package", path);
}
}).detach([](kj::Exception&& exception) {
JSG_FAIL_REQUIRE(Error, "Failed to get package", exception);
});
}

kj::Promise<kj::Own<SmallPackagesTarReader>> PackagePromiseMap::getPromise(kj::StringPtr path) {
auto maybeWaitlist = waitlists.find(path);

KJ_IF_SOME(waitlist, maybeWaitlist) {
co_await waitlist.addWaiter();
auto maybeEntry = fetchedPackages.findEntry(path);
KJ_IF_SOME(entryRef, maybeEntry) {
auto entry = fetchedPackages.release(entryRef);

co_return kj::mv(entry.value);
} else {
JSG_FAIL_REQUIRE(Error, "Failed to get package when trying to get promise", path);
}

} else {
JSG_FAIL_REQUIRE(Error, "Failed to get waitlist for package when trying to get promise", path);
}
}

} // namespace workerd::api::pyodide
24 changes: 22 additions & 2 deletions src/workerd/api/pyodide/pyodide.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include "workerd/util/wait-list.h"
#include <kj/array.h>
#include <kj/debug.h>
#include <kj/common.h>
Expand Down Expand Up @@ -187,12 +188,27 @@ struct MemorySnapshotResult {
JSG_STRUCT(snapshot, importedModulesList);
};

// Before a Pyodide isolate starts up, all its packages begin loading. This struct provides an
// interface for the Pyodide startup code to request promises for each package that was loaded.
class PackagePromiseMap {
// This implementation is complex because we're not allowed to store kj::Promises on an object
// stored on the V8 heap. Ideally, we'd like to simply store a map from package paths to Promises.
// Instead, we store a CrossThreadWaitList and get a new Promise from the waitlist as needed.
kj::HashMap<kj::String, kj::Own<SmallPackagesTarReader>> fetchedPackages;
kj::HashMap<kj::String, CrossThreadWaitList> waitlists;

public:
void insert(kj::String path, kj::Promise<kj::Own<SmallPackagesTarReader>> promise);

// Note it is explicitly not allowed to call getPromise more than once on the same package
// This is because allowing this causes ownership of the SmallPackagesTarReader to be muddied.
kj::Promise<kj::Own<SmallPackagesTarReader>> getPromise(kj::StringPtr path);
};

// A loaded bundle of artifacts for a particular script id. It can also contain V8 version and
// CPU architecture-specific artifacts. The logic for loading these is in getArtifacts.
class ArtifactBundler: public jsg::Object {
public:
typedef kj::HashMap<kj::String, IoOwn<kj::Promise<kj::Own<SmallPackagesTarReader>>>>
PackagePromiseMap;
kj::Maybe<MemorySnapshotResult> storedSnapshot;
kj::Own<PackagePromiseMap> loadedPackages;

Expand Down Expand Up @@ -251,6 +267,10 @@ class ArtifactBundler: public jsg::Object {
return false; // TODO(later): Remove this function once we regenerate the bundle.
}

kj::Promise<kj::Own<SmallPackagesTarReader>> getPackage(kj::StringPtr path) {
return loadedPackages->getPromise(path);
}

JSG_RESOURCE_TYPE(ArtifactBundler) {
JSG_METHOD(hasMemorySnapshot);
JSG_METHOD(getMemorySnapshotSize);
Expand Down

0 comments on commit b5dbcc6

Please sign in to comment.