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

src: add Realm document in the src README.md #47932

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

legendecas
Copy link
Member

Document the current Environment/Realm definitions.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 9, 2023
@legendecas legendecas added doc Issues and PRs related to the documentations. realm Issues and PRs related to the ShadowRealm API and node::Realm labels May 9, 2023
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

@cjihrig @VoltrexKeyva Updated. Thank you for the suggestions!

src/README.md Outdated Show resolved Hide resolved
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()`.
Copy link
Member

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.

Copy link
Member

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 Realms for the same Environment and which not. I've seen a bunch of PRs that move properties from the Environment to the individual Realms, but it's not really clear where the line is and whether we're not essentially just introducing the ability to have multiple Environments similar to #47855.

Copy link
Member Author

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.

Copy link
Member

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 Realms were introduced)?

Copy link
Member Author

@legendecas legendecas May 18, 2023

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:

  1. An Environment is associated with an Isolate and provides the per-isolate hooks and APIs, and per-thread states, for instance, inspector agents, profilers, and APIs like RequestInterrupt() and can_call_into_js().
  2. A Realm is associated with a Context and consists of a global object and can be extended as a principal realm or a synthetic realm. Each Environment 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.

Copy link
Member

Choose a reason for hiding this comment

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

  1. An Environment is associated with an Isolate and provides the per-isolate hooks and APIs,

You’re describing IsolateData here, though.

Copy link
Member Author

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.

Copy link
Member

@joyeecheung joyeecheung May 26, 2023

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:

  1. There is IsolateData which is meant to hold per-isolate data
  2. There are Realms with are meant to hold per-context data
  3. 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.

Copy link
Member Author

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?

src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
joyeecheung

This comment was marked as off-topic.

@legendecas legendecas force-pushed the shadow-realm/src-readme branch from d154353 to a907870 Compare May 16, 2023 12:01
@legendecas
Copy link
Member Author

@joyeecheung @addaleax would you mind taking a look at this again? Thank you! <3

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label May 31, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

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/.ncu
https://github.com/nodejs/node/actions/runs/5134609431

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot nodejs-github-bot merged commit d57dd70 into nodejs:main Jun 1, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in d57dd70

@legendecas legendecas deleted the shadow-realm/src-readme branch June 2, 2023 10:02
targos pushed a commit that referenced this pull request Jun 4, 2023
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>
@targos targos mentioned this pull request Jun 4, 2023
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
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>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. realm Issues and PRs related to the ShadowRealm API and node::Realm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants