-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
src: add Realm document in the src README.md #47932
src: add Realm document in the src README.md #47932
Conversation
@cjihrig @VoltrexKeyva Updated. Thank you for the suggestions! |
src/README.md
Outdated
It also provides [cleanup hooks][] and maintains a list of [`BaseObject`][] | ||
instances. | ||
different built-in modules that can be shared across different `Realm` | ||
instances, for example a libuv timer for `setTimeout()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should they actually share timers? I think the timer callbacks are tied to specific contexts too? Though I guess the event loop could be shared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would actually be very helpful to spell out what properties are being shared between different Realm
s for the same Environment
and which not. I've seen a bunch of PRs that move properties from the Environment
to the individual Realm
s, but it's not really clear where the line is and whether we're not essentially just introducing the ability to have multiple Environment
s similar to #47855.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out! I've added a list of examples that can be shared across realms with an Environment
.
It is still yet to be done to share the async hook info between the realms -- it is necessary to link the async continuation between realm boundaries for AsyncLocalStorage to propagate correctly. This is essential to allow JS object access between multiple execution "environments" on the same thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not really sure that this answers the question. How is the end state of Realm
going to be different from what Environment
is (or used to be before Realm
s were introduced)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference between the end state of the Realm
and the Environment
would be:
- An
Environment
is associated with anIsolate
and provides the per-isolate hooks and APIs, and per-thread states, for instance, inspector agents, profilers, and APIs likeRequestInterrupt()
andcan_call_into_js()
. - A
Realm
is associated with aContext
and consists of a global object and can be extended as a principal realm or a synthetic realm. EachEnvironment
has a single principal realm as its entry.
As realms share the Environment
instance, we don't have to create inspector IO threads, or profiler connections for each realm. The async local storage needs to be propagated across async boundaries in another Realm
of an Environment
too.
Additionally, a Realm
must be able to be repetitively created on the same Isolate
and weakly referenced to properly support the ECMAScript ShadowRealm
API.
As a conclusion, it is necessary to split the Realm
and Environment
to distinguish per-isolate states and per-context states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- An
Environment
is associated with anIsolate
and provides the per-isolate hooks and APIs,
You’re describing IsolateData
here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re describing
IsolateData
here, though.
Yes, it's true that IsolateData
is also associated with an Isolate
. However, as documented, IsolateData
acts like a string table and contains information (e.g. startup snapshot data) about the isolate. It has its distinct properties compared to an Environment
.
Compared to IsolateData
, Environment
at the end state contains the necessary handles (bootstrapped with the principal realm) to propagate events across realm boundaries, for instance, propagating async local storages and promise rejection events from ShadowRealm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current state we have is:
- There is IsolateData which is meant to hold per-isolate data
- There are Realms with are meant to hold per-context data
- There is Environment which has been a hotch-potch structure that people dump everything on since forever, so it contains both per-isolate data and per-context data (for the main context) in practice.
What we've been trying to do is to move per-isolate data in Environment to IsolateData and per-context data to Realm, it's happening but it's really not there yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax do you think your question has been answered?
d154353
to
a907870
Compare
@joyeecheung @addaleax would you mind taking a look at this again? Thank you! <3 |
Commit Queue failed- Loading data for nodejs/node/pull/47932 ✔ Done loading data for nodejs/node/pull/47932 ----------------------------------- PR info ------------------------------------ Title src: add Realm document in the src README.md (#47932) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch legendecas:shadow-realm/src-readme -> nodejs:main Labels c++, doc, needs-ci, realm Commits 3 - src: add Realm document in the src README.md - fixup! src: add Realm document in the src README.md - fixup! src: add Realm document in the src README.md Committers 1 - Chengzhong Wu PR-URL: https://github.com/nodejs/node/pull/47932 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Joyee Cheung ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47932 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Joyee Cheung -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 09 May 2023 09:39:26 GMT ✔ Approvals: 3 ✔ - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/47932#pullrequestreview-1418601961 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/47932#pullrequestreview-1449806356 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/47932#pullrequestreview-1446049565 ✔ Last GitHub CI successful ✘ No Jenkins CI runs detected -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/5134609431 |
Landed in d57dd70 |
PR-URL: #47932 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#47932 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #47932 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#47932 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#47932 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#47932 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Document the current
Environment
/Realm
definitions.