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

sea: only assert snapshot main function for main threads #56120

Merged
merged 2 commits into from
Dec 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,8 @@ MaybeLocal<Value> StartExecution(Environment* env, StartExecutionCallback cb) {
CHECK(!env->isolate_data()->is_building_snapshot());

#ifndef DISABLE_SINGLE_EXECUTABLE_APPLICATION
if (sea::IsSingleExecutable()) {
// Snapshot in SEA is only loaded for the main thread.
if (sea::IsSingleExecutable() && env->is_main_thread()) {
Copy link
Member

@legendecas legendecas Dec 3, 2024

Choose a reason for hiding this comment

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

Can we add a check that env->snapshot_deserialize_main().IsEmpty() when it is not the main thread?

IIUC this relies on the trick that context->GetDataFromSnapshotOnce will only return the data once and the second time it will return empty, as the main thread and worker threads are using the same snapshot data. Edit: the worker's main context uses SnapshotData::kNodeBaseContextIndex which has no snapshot_deserialize_main. But I think it is still worth adding the check to ensure snapshot_deserialize_main is not set on worker threads, for the lines below:

node/src/node.cc

Lines 344 to 346 in fe12b01

if (!env->snapshot_deserialize_main().IsEmpty()) {
return env->RunSnapshotDeserializeMain();
}

Copy link
Member Author

@joyeecheung joyeecheung Dec 4, 2024

Choose a reason for hiding this comment

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

hmm yeah, we still haven't figured out how it should really work for workers - we'll likely need some restructuring of the snapshot blob/API. It currently goes like:

  • built-in default context with only stuff from V8
  • built-in vm context with the interceptor template and some fixups to the globals
  • built-in worker context which currently only runs through NewContext()
  • built-in/custom main context which runs through CreateEnvironment()

All these share one isolate context.

It's still possible to support custom worker context snapshot (in which case the main context is the "root" worker context) + populate more stuff in built-in worker context snapshot, but it will probably need some restructuring. For a fuller built-in worker context snapshot we'll need to run an additional CreateEnvironment() call on the same isolate but with different flags and do this

node/src/env.h

Line 568 in a243225

// TODO(joyeecheung): there should be a vector of env_info once we snapshot
, though I am not 100% sure if it works properly at the moment...can take a look as I look into the worker + snapshot combos a bit. For now a check to ensure snapshot_deserialize_main is not set on worker threads is fine.

sea::SeaResource sea = sea::FindSingleExecutableResource();
// The SEA preparation blob building process should already enforce this,
// this check is just here to guard against the unlikely case where
Expand All @@ -342,6 +343,9 @@ MaybeLocal<Value> StartExecution(Environment* env, StartExecutionCallback cb) {
// move the pre-execution part into a different file that can be
// reused when dealing with user-defined main functions.
if (!env->snapshot_deserialize_main().IsEmpty()) {
// Custom worker snapshot is not supported yet,
// so workers can't have deserialize main functions.
CHECK(env->is_main_thread());
return env->RunSnapshotDeserializeMain();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
'use strict';

require('../common');

const {
generateSEA,
skipIfSingleExecutableIsNotSupported,
} = require('../common/sea');

skipIfSingleExecutableIsNotSupported();

// This tests the snapshot support in single executable applications.

const tmpdir = require('../common/tmpdir');
const { writeFileSync, existsSync } = require('fs');
const {
spawnSyncAndAssert, spawnSyncAndExitWithoutError,
} = require('../common/child_process');
const assert = require('assert');

const configFile = tmpdir.resolve('sea-config.json');
const seaPrepBlob = tmpdir.resolve('sea-prep.blob');
const outputFile = tmpdir.resolve(process.platform === 'win32' ? 'sea.exe' : 'sea');

{
tmpdir.refresh();

// FIXME(joyeecheung): currently `worker_threads` cannot be loaded during the
// snapshot building process because internal/worker.js is accessing isMainThread at
// the top level (and there are maybe more code that access these at the top-level),
// and have to be loaded in the deserialized snapshot main function.
// Change these states to be accessed on-demand.
const code = `
const {
setDeserializeMainFunction,
} = require('v8').startupSnapshot;
setDeserializeMainFunction(() => {
const { Worker } = require('worker_threads');
new Worker("console.log('Hello from Worker')", { eval: true });
});
`;

writeFileSync(tmpdir.resolve('snapshot.js'), code, 'utf-8');
writeFileSync(configFile, `
{
"main": "snapshot.js",
"output": "sea-prep.blob",
"useSnapshot": true
}
`);

spawnSyncAndExitWithoutError(
process.execPath,
['--experimental-sea-config', 'sea-config.json'],
{
cwd: tmpdir.path,
env: {
NODE_DEBUG_NATIVE: 'SEA',
...process.env,
},
});

assert(existsSync(seaPrepBlob));

generateSEA(outputFile, process.execPath, seaPrepBlob);

spawnSyncAndAssert(
outputFile,
{
env: {
NODE_DEBUG_NATIVE: 'SEA',
...process.env,
}
},
{
trim: true,
stdout: 'Hello from Worker'
}
);
}
Loading