src: stop copying code cache #47144
Merged
src: stop copying code cache #47144
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
c++
github-actions
bot
removed
the
request-ci
|
For 2) I think we can just document in node.h that the snapshot data must outlive the isolate/Environment. That's already the case internally. |
anonrig
approved these changes
Mar 18, 2023
I added a line about this to the documentation of |
github-actions
bot
removed
the
request-ci
17 tasks
legendecas
approved these changes
Mar 20, 2023
addaleax
reviewed
Mar 20, 2023
src/node.h
Outdated
Comment on lines
507
to
508
| // The snapshot *must* be kept alive during the execution of the environment / | ||
| // isolate that it is derived from. |
There was a problem hiding this comment.
Suggested change
| // The snapshot *must* be kept alive during the execution of the environment / | |
| // isolate that it is derived from. | |
| // The snapshot *must* be kept alive during the execution of the Isolate | |
| // that derives from it. |
or
Suggested change
| // The snapshot *must* be kept alive during the execution of the environment / | |
| // isolate that it is derived from. | |
| // The snapshot *must* be kept alive during the execution of the Isolate | |
| // that was created using it. |
(this header refers to Isolate/Environment as capitalized names to distinguish them from the regular English-language words; Environment instances do not outlive their Isolate instances; and the Isolate derives from the snapshot in this scenario, not the other way around, since this is about the case in which the snapshot is being consumed, not when it is being built)
19 tasks
19 tasks
The code cache is quite large - around 1.3 MiB. Change the code to use
non-owning buffers to avoid copying it. For starting up an otherwise
empty main isolate, this saves around 1.3 MiB of unique set size memory
(9.9 MiB -> 8.6 MiB) and 1.1ms elapsed time (22.9 ms -> 21.8 ms).
Copying the code cache is unnecessary since:
1. for the builtin snapshot, the code cache data has static lifetime.
2. for non-builtin snapshots, we create copies of the code cache data in
`SnapshotDeserializer::ReadVector`. These copies are owned by the
`Environment` (through `IsolateData` -> `SnapshotData`), so they
won't be deallocated.
3. a worker thread can copy a parent's isolate's code cache, but in that
case we still know that the parent isolate will outlive the worker
isolate.
(Admittedly point (2) feels a little fragile from a lifetime
perspective, and I would be happy to restrict this optimization to the
builtin snapshot.)
```console
$ perf stat -r 100 -e ... ./node -e 0
Performance counter stats for './node -e 0' (100 runs):
21.78 msec task-clock
2760 page-faults
113161604 instructions
18437648 branches
423230 branch-misses
853093 cache-references
41474 cache-misses
0.0225473 +- 0.0000504 seconds time elapsed ( +- 0.22% )
$ perf stat -r 100 -e ... ./node-main -e 0
Performance counter stats for './node-main -e 0' (100 runs):
22.91 msec task-clock
3102 page-faults
114890673 instructions
18751329 branches
428909 branch-misses
895721 cache-references
45202 cache-misses
0.0233760 +- 0.0000741 seconds time elapsed ( +- 0.32% )
```
kvakil
force-pushed
the
doNotCopyCodeCache
branch
from
f4833ff to
eb99a86
Compare
github-actions
bot
removed
the
request-ci
|
I reworded the comment to @addaleax's suggestion. Test failures look like obvious flakes. PTAL. |
This was referenced Apr 12, 2023
joyeecheung
approved these changes
May 2, 2023
anonrig
approved these changes
May 2, 2023
This was referenced May 3, 2023
kvakil
added
the
author ready
22 tasks
debadree25
added
commit-queue-squash
nodejs-github-bot
removed
the
commit-queue
|
Landed in 1e6e5a9 |
targos
pushed a commit
that referenced
this pull request
May 12, 2023
The code cache is quite large - around 1.3 MiB. Change the code to use
non-owning buffers to avoid copying it. For starting up an otherwise
empty main isolate, this saves around 1.3 MiB of unique set size memory
(9.9 MiB -> 8.6 MiB) and 1.1ms elapsed time (22.9 ms -> 21.8 ms).
Copying the code cache is unnecessary since:
1. for the builtin snapshot, the code cache data has static lifetime.
2. for non-builtin snapshots, we create copies of the code cache data in
`SnapshotDeserializer::ReadVector`. These copies are owned by the
`Environment` (through `IsolateData` -> `SnapshotData`), so they
won't be deallocated.
3. a worker thread can copy a parent's isolate's code cache, but in that
case we still know that the parent isolate will outlive the worker
isolate.
(Admittedly point (2) feels a little fragile from a lifetime
perspective, and I would be happy to restrict this optimization to the
builtin snapshot.)
```console
$ perf stat -r 100 -e ... ./node -e 0
Performance counter stats for './node -e 0' (100 runs):
21.78 msec task-clock
2760 page-faults
113161604 instructions
18437648 branches
423230 branch-misses
853093 cache-references
41474 cache-misses
0.0225473 +- 0.0000504 seconds time elapsed ( +- 0.22% )
$ perf stat -r 100 -e ... ./node-main -e 0
Performance counter stats for './node-main -e 0' (100 runs):
22.91 msec task-clock
3102 page-faults
114890673 instructions
18751329 branches
428909 branch-misses
895721 cache-references
45202 cache-misses
0.0233760 +- 0.0000741 seconds time elapsed ( +- 0.32% )
```
PR-URL: #47144
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
danielleadams
added
the
backport-requested-v18.x
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.
The code cache is quite large - around 1.3 MiB. Change the code to use non-owning buffers to avoid copying it. For starting up an otherwise empty main isolate, this saves around 1.3 MiB of unique set size memory (9.9 MiB -> 8.6 MiB) and 1.1ms elapsed time (22.9 ms -> 21.8 ms).
Copying the code cache is unnecessary since:
SnapshotDeserializer::ReadVector. These copies are owned by theEnvironment(throughIsolateData->SnapshotData), so they won't be deallocated.(Admittedly point (2) feels a little fragile from a lifetime perspective, and I would be happy to restrict this optimization to the builtin snapshot.)