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.

Sign up for GitHub

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

Jump to bottom

Remove circular dependency in code cache and snapshot building #31074

Closed
joyeecheung opened this issue Dec 23, 2019 · 0 comments
Closed

Remove circular dependency in code cache and snapshot building #31074

joyeecheung opened this issue Dec 23, 2019 · 0 comments
Assignees
@joyeecheung
Labels
build Issues and PRs related to build files or the CI.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Dec 23, 2019 •
edited

This issue tracks this TODO in node.gyp (which was originally for the code cache builder, but we also need something similar for the snapshot builder)

node/node.gyp

Lines 1176 to 1181 in db109e8

# TODO(joyeecheung): do not depend on node_lib,
# instead create a smaller static library node_lib_base that does
# just enough for node_native_module.cc and the cache builder to
# compile without compiling the generated code cache C++ file.
# So generate_code_cache -> mkcodecache -> node_lib_base,
# node_lib -> node_lib_base & generate_code_cache

Removing the current circular dependency in the two-step builds should make this building process less error-prone.

Previous refs:

#27431
#30647
@joyeecheung joyeecheung added the build Issues and PRs related to build files or the CI. label Dec 23, 2019
@joyeecheung joyeecheung self-assigned this Dec 23, 2019
@joyeecheung joyeecheung changed the title refactor code cache and snapshot building Remove circular dependency in code cache and snapshot building Dec 23, 2019
@joyeecheung joyeecheung mentioned this issue Feb 25, 2020
Closed
4 tasks
@joyeecheung joyeecheung mentioned this issue Jul 21, 2020
Closed
@joyeecheung joyeecheung mentioned this issue Jul 21, 2020
Closed
4 tasks
joyeecheung added a commit to joyeecheung/node that referenced this issue Jul 22, 2020
@joyeecheung
build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot
e4d50ab 
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See nodejs#31074.
gengjiawen pushed a commit that referenced this issue Jul 22, 2020
@joyeecheung @gengjiawen
build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot
f4f191b 
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See #31074.

PR-URL: #34454
Fixes: #34435
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
cjihrig pushed a commit that referenced this issue Jul 23, 2020
@joyeecheung @cjihrig
build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot
29309f6 
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See #31074.

PR-URL: #34454
Fixes: #34435
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@joyeecheung joyeecheung mentioned this issue May 9, 2022
Closed
joyeecheung added a commit that referenced this issue May 17, 2022
@joyeecheung
bootstrap: include code cache in the embedded snapshot
fbe1478 
Since V8 code cache encodes indices to the read-only space
it is safer to make sure that the code cache is generated in the
same heap used to generate the embdded snapshot. This patch
merges the code cache builder into the snapshot builder and
makes the code cache part of node::SnapshotData that is
deserialized into the native module loader during bootstrap.

PR-URL: #43023
Fixes: #31074
Refs: #35711
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
bengl pushed a commit that referenced this issue May 30, 2022
@joyeecheung @bengl
src: move context snapshot index to SnapshotData
82fb037 
Also added comments for the members of SnapshotData and renamed
blob to v8_snapshot_blob_data for clarity.

PR-URL: #43023
Fixes: #31074
Refs: #35711
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
bengl pushed a commit that referenced this issue May 30, 2022
@joyeecheung @bengl
bootstrap: include code cache in the embedded snapshot
85d81a7 
Since V8 code cache encodes indices to the read-only space
it is safer to make sure that the code cache is generated in the
same heap used to generate the embdded snapshot. This patch
merges the code cache builder into the snapshot builder and
makes the code cache part of node::SnapshotData that is
deserialized into the native module loader during bootstrap.

PR-URL: #43023
Fixes: #31074
Refs: #35711
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@joyeecheung joyeecheung mentioned this issue Jul 13, 2022
Closed
joyeecheung added a commit that referenced this issue Jul 21, 2022
@joyeecheung
src: merge NativeModuleEnv into NativeModuleLoader
7a92662 
Now that we include the code cache into the embedded snapshot,
there is no point in splitting an Environment-independent
NativeModuleLoader out of NativeModuleEnv. Merge the two
classes for simplicity.

PR-URL: #43824
Refs: #31074
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 26, 2022
@joyeecheung @danielleadams
src: merge NativeModuleEnv into NativeModuleLoader
9f9d00c 
Now that we include the code cache into the embedded snapshot,
there is no point in splitting an Environment-independent
NativeModuleLoader out of NativeModuleEnv. Merge the two
classes for simplicity.

PR-URL: #43824
Refs: #31074
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
@joyeecheung @Fyko
src: merge NativeModuleEnv into NativeModuleLoader
8989394 
Now that we include the code cache into the embedded snapshot,
there is no point in splitting an Environment-independent
NativeModuleLoader out of NativeModuleEnv. Merge the two
classes for simplicity.

PR-URL: nodejs#43824
Refs: nodejs#31074
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joyeecheung joyeecheung

Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bootstrap: include code cache in the embedded snapshot joyeecheung/node
1 participant
@joyeecheung