[v20.x] backport unflagging of require(esm), part 1 (of 4?) #56730
+137,025
−1,494
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
|
Review requested:
|
nodejs-github-bot
added
lib / src
|
cc @nodejs/releasers |
aduh95
added
semver-minor
Let's wait for #56727 to land as currently the CI is not passing on the staging branch |
marco-ippolito
force-pushed
the
v20.x-staging
branch
from
416c658 to
cc7d1a7
Compare
Original commit message:
Reland "[cache] Don't compare host defined options if the script was deserialized"
This is a reland of commit b9cfb8b7cfdbf195c3baf87735865948dfa5907e
Original change's description:
> [cache] Don't compare host defined options if the script was deserialized
>
> We don't serialize host defined options (see
> CodeSerializer::SerializeObjectImpl()).
>
> Change-Id: Icab9fe910a5ec93b5eacc4869aba75034ad8b447
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4805329
> Reviewed-by: Camillo Bruni <cbruni@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Tao Pan <tao.pan@intel.com>
> Cr-Commit-Position: refs/heads/main@{#90698}
Change-Id: I7ea5e9355056104ebd25b385aba63c1233d42260
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4998581
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Tao Pan <tao.pan@intel.com>
Cr-Commit-Position: refs/heads/main@{#90711}
Refs: v8/v8@7b677a5
Original commit message:
[compiler] support isolate compilation cache in CompileFunction()
Previously there was no isolate compilation cache support for
scripts compiled Script::CompileFunction() with wrapped arguments.
This patch adds support for that.
Refs: nodejs#35375
Change-Id: Id1849961ecd1282eb2dac95829157d167a3aa9a1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4962094
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#91681}
Refs: v8/v8@f4b3f6e
Original commit message:
[compiler] reset script details in functions deserialized from code cache
During the serialization of the code cache, V8 would wipe out the
host-defined options, so after a script id deserialized from the
code cache, the host-defined options need to be reset on the script
using what's provided by the embedder when doing the deserializing
compilation, otherwise the HostImportModuleDynamically callbacks
can't get the data it needs to implement dynamic import().
Change-Id: I33cc6a5e43b6469d3527242e083f7ae6d8ed0c6a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5401780
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#93323}
Refs: v8/v8@cd10ad7
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Original commit message:
[snapshot] Check if a cached data has wrapped arguments
Fixes that ScriptCompiler::CreateCodeCacheForFunction aborts on
a deserialized shared function info from a cached data accepted
with ScriptCompiler::CompileFunction. If the wrapped argument list
does not match, the cached data should be rejected.
Refs: nodejs#56366
Change-Id: I3f0376216791d866fac8eed1ce88dfa05e919b48
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6140933
Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#97942}
Refs: v8/v8@96ee9bb
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#50322 Backport-PR-URL: nodejs#56590 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: nodejs#50322 Backport-PR-URL: nodejs#56590 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: nodejs#50322 Backport-PR-URL: nodejs#56590 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: nodejs#52093 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com>
Otherwise re-entering V8 doesn't work as expected after exceptions were thrown. Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5050065 Co-Authored-By: Toon Verwaest <verwaest@chromium.org> Co-Authored-By: deepak1556 <hop2deep@gmail.com> PR-URL: nodejs#51362 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This patch implements automatic on-disk code caching that can be enabled
via an environment variable NODE_COMPILE_CACHE.
When set, whenever Node.js compiles a CommonJS or a ECMAScript Module,
it will use on-disk [V8 code cache][] persisted in the specified
directory to speed up the compilation. This may slow down the first
load of a module graph, but subsequent loads of the same module graph
may get a significant speedup if the contents of the modules do not
change. Locally, this speeds up loading of
test/fixtures/snapshot/typescript.js from ~130ms to ~80ms.
To clean up the generated code cache, simply remove the directory.
It will be recreated the next time the same directory is used for
`NODE_COMPILE_CACHE`.
Compilation cache generated by one version of Node.js may not be used
by a different version of Node.js. Cache generated by different versions
of Node.js will be stored separately if the same directory is used
to persist the cache, so they can co-exist.
Caveat: currently when using this with V8 JavaScript code coverage, the
coverage being collected by V8 may be less precise in functions that are
deserialized from the code cache. It's recommended to turn this off when
running tests to generate precise coverage.
Implementation details:
There is one cache file per module on disk. The directory layout
is:
- Compile cache directory (from NODE_COMPILE_CACHE)
- 8b23c8fe: CRC32 hash of CachedDataVersionTag + NODE_VERESION
- 2ea3424d:
- 10860e5a: CRC32 hash of filename + module type
- 431e9adc: ...
- ...
Inside the cache file, there is a header followed by the actual
cache content:
```
[uint32_t] code size
[uint32_t] code hash
[uint32_t] cache size
[uint32_t] cache hash
... compile cache content ...
```
When reading the cache file, we'll also check if the code size
and code hash match the code that the module loader is loading
and whether the cache size and cache hash match the file content
read. If they don't match, or if V8 rejects the cache passed,
we'll ignore the mismatch cache, and regenerate the cache after
compilation succeeds and rewrite it to disk.
PR-URL: nodejs#52535
Refs: nodejs#47472
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Instead of using an async function wrapper, just try compiling code with unknown module format as SourceTextModule when it cannot be compiled as CJS and the error message indicates that it's worth a retry. If it can be parsed as SourceTextModule then it's considered ESM. Also, move shouldRetryAsESM() to C++ completely so that we can reuse it in the CJS module loader for require(esm). Drive-by: move methods that don't belong to ContextifyContext out as static methods and move GetHostDefinedOptions to ModuleWrap. PR-URL: nodejs#52413 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
It might be worth designing a policy for the compilation cache. For now, just skip the cache when policy is enabled. PR-URL: nodejs#52577 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Avoid repetitively calling into JS callback from C++ in `ModuleWrap::Link`. This removes the convoluted callback style of the internal `ModuleWrap` link step. PR-URL: nodejs#52058 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This patch: 1. Adds ESM syntax detection to compileFunctionForCJSLoader() for --experimental-detect-module and allow it to emit the warning for how to load ESM when it's used to parse ESM as CJS but detection is not enabled. 2. Moves the ESM detection of --experimental-detect-module for the entrypoint from executeUserEntryPoint() into Module.prototype._compile() and handle it directly in the CJS loader so that the errors thrown during compilation *and execution* during the loading of the entrypoint does not need to be bubbled all the way up. If the entrypoint doesn't parse as CJS, and detection is enabled, the CJS loader will re-load the entrypoint as ESM on the spot asynchronously using runEntryPointWithESMLoader() and cascadedLoader.import(). This is fine for the entrypoint because unlike require(ESM) we don't the namespace of the entrypoint synchronously, and can just ignore the returned value. In this case process.mainModule is reset to undefined as they are not available for ESM entrypoints. 3. Supports --experimental-detect-module for require(esm). PR-URL: nodejs#52047 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
So that if there are circular dependencies in the synchronous module graph, they could be resolved using the cached jobs. In case linking fails and the error gets caught, reset the cache right after linking. If it succeeds, the caller will cache it again. Otherwise the error bubbles up to users, and since we unset the cache for the unlinkable module the next attempt would still fail. PR-URL: nodejs#52868 Fixes: nodejs#52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs#53050 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Unifies the CJS and ESM source map cache map with SourceMapCacheMap and allows the CJS cache entries to be queried more efficiently with a source url without iteration on an IterableWeakMap. Add a test to verify that the CJS source map cache entry can be reclaimed. PR-URL: nodejs#51711 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#52213 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#52658 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me>
joyeecheung
force-pushed
the
backport-require-esm-20
branch
from
a51b482 to
5d03fce
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.
This is what I have so far by following the worklist in #52697 (comment) - not sure how we are going to manage the backport merges and whether this strategy is the best, but just opening a PR for now to leave some traces and get some discussion before I get too deep in the rabbit hole.
Notable things in this backport:
lib/internal/modules/,src/node_contextify.ccandsrc/module_wrap.cc, I am fairly certain all the relevant changes must have touched at least one of these), and picking all the commits that are affecting code paths ofrequire(esm), no matter they are directly done forrequire(esm)or not, to reduce conflicts and avoid introducing disconnected bits that might cause bugs. The commits that are not crossing paths withrequire(esm)under my assessment and the ones that obviously should not land (e.g. mass refactoring, semver-major) are omitted.module.dependencySpecifiers()(which is semver-major) and added a internal polyfill forFromV8Arrayin util.h since the new V8 API isn't on v20.x.internalRequire()which is still a thing on v20.x since policy is still there.require(esm)without also unflagging detect-module can be risky because the code are very intertwined, not just source-level but also logic-level.