Add an option to newContext to disable memoization of global context … #2999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danlapid
force-pushed
the
dlapid/context_disable_memoize
branch
2 times, most recently
from
1afc837
to
8e026ba
Compare
jasnell
reviewed
Oct 24, 2024
| @@ -527,14 +533,15 @@ template <typename TypeWrapper, | |||
| const char* methodName, | |||
| typename Method, | |||
| Method method, | |||
| bool isContext> | |||
| bool isContext, | |||
| bool memoize> | |||
There was a problem hiding this comment.
Non-blocking nit... really not a big fan of bool args like this. Much prefer enums for this type of case. Won't block on it tho
danlapid
force-pushed
the
dlapid/context_disable_memoize
branch
from
8e026ba
to
d4d60d7
Compare
|
@jasnell is this good to go? |
dom96
reviewed
Oct 25, 2024
Comment on lines
+1463
to
+1470
| // | ||
| // When creating a new context the global context template will be memoized for future reuse in | ||
| // additional contexts. The global context can depend on `configuration`. For Python Isolate | ||
| // Pools we want to create a new context and initialize some code in it and later when a request | ||
| // arrives, edit the configuration and create a new context. To achieve this we create the first | ||
| // context with `memoize=false` to disable memoization of the global context created for the | ||
| // first context. Then we create a new context with `memoize=true` to enable memoization of the | ||
| // main context created after a request was received. |
There was a problem hiding this comment.
Probably a stupid question, but I wonder why we cannot overwrite the memoized context after it has been memoized?
There was a problem hiding this comment.
That would require the inverse of the same logic changing the code flow that existed before this change rather than adding a different code flow for our exceptional case.
jasnell
approved these changes
Oct 25, 2024
…template. For Python Isolate Pools we want to instantiate emscripten in a separate context on isolate creation (before we have compatibility flags) and later after receiving the compatibility flags create a new context. One blocker to this approach is all the templates that are instantiated in the first context are memoized. This PR allows us to create a new context without memoizing the global context templates. This feature is off by default and will be enabled in a future PR for python isolates only.
danlapid
force-pushed
the
dlapid/context_disable_memoize
branch
from
d4d60d7
to
57365ba
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…template.
For Python Isolate Pools we want to instantiate emscripten in a separate context on isolate creation (before we have compatibility flags) and later after receiving the compatibility flags create a new context.
One blocker to this approach is all the templates that are instantiated in the first context are memoized.
This PR allows us to create a new context without memoizing the global context templates.
This feature is off by default and will be enabled in a future PR for python isolates only.