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.

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

Move ownership of metrics and limitEnforcer to the api type. #3034

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

danlapid
Copy link
Collaborator

@danlapid danlapid commented Oct 30, 2024

Enable const variants of some const-safe jsg functions.
Move ownership of metrics and limitEnforcer to the api type.

This PR has no real function and should not change anything of itself.
Instead it's just another part of the work to enable python isolate pools.
This PR does some cleanup that will later enable us to create an isolate without the data that only becomes available once we have a request.

There is also a corresponding internal PR 9105

@danlapid danlapid requested review from a team as code owners October 30, 2024 23:12
@danlapid danlapid requested review from mikea and a-robinson October 30, 2024 23:12
Copy link

github-actions bot commented Oct 30, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@danlapid danlapid force-pushed the dlapid/ownership_cleanups_for_pools branch 2 times, most recently from ee2b913 to e347c69 Compare October 30, 2024 23:14
@danlapid danlapid requested review from dom96 and hoodmane October 30, 2024 23:16
@danlapid danlapid force-pushed the dlapid/ownership_cleanups_for_pools branch from e347c69 to 6dd801a Compare October 31, 2024 00:09
Dan Lapid added 2 commits October 31, 2024 00:11
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.
@danlapid danlapid force-pushed the dlapid/ownership_cleanups_for_pools branch from 6dd801a to a9fd4fc Compare October 31, 2024 00:11
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! Ownership changes look good. One tiny change seems to be to when we call customizeIsolate, but that likely isn't problematic. So LGTM!

Comment on lines +177 to +178
jsgIsolate.runInLockScope(
[&](JsgWorkerdIsolate::Lock& lock) { limitEnforcer->customizeIsolate(lock.v8Isolate); });
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the previous version of the code this is run under jsg::runInV8Stack, I assume doing this here under runInLockScope is equivalent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's a utility function that translates to the same thing :)

@danlapid
Copy link
Collaborator Author

Nice! Ownership changes look good. One tiny change seems to be to when we call customizeIsolate, but that likely isn't problematic. So LGTM!

Yep you're right, that change is indeed intended, we're going to be using this isolate before we have an Isolate class wrapping it, we need it to have a limit enforcer tied to it. I would argue the previous location was just incorrect but since the constructors were always called one after the other in quick succession it never mattered.

@danlapid danlapid merged commit 43c5ca0 into main Oct 31, 2024
13 checks passed
@danlapid danlapid deleted the dlapid/ownership_cleanups_for_pools branch October 31, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants