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: export InitializeV8Platform function #31217

Closed
wants to merge 1 commit into from

Conversation

jc-lab
Copy link

@jc-lab jc-lab commented Jan 6, 2020

Export InitializeV8Platform to allow use node as a shared library.

For example:
https://github.com/jc-lab/node-app


  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

export InitializeV8Platform to allow use node as a shared library.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 6, 2020
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

However, I would very much encourage using CreatePlatform()/FreePlatform() rather than using the global instance.

@addaleax addaleax added the embedding Issues and PRs related to embedding Node.js in another project. label Jan 6, 2020
@jc-lab
Copy link
Author

jc-lab commented Jan 6, 2020

@addaleax Thanks for comment.

I used CreatePlatform. However, the call to GetMainThreadMultiIsolatePlatform (Call from NewIsolate) used by worker_threads fails.

Initialization of per_process::v8_platform seems to only happen on InitializeV8Platform.

node/src/api/environment.cc

Lines 353 to 355 in 2bdeb88

MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform() {
return per_process::v8_platform.Platform();
}

<=
inline void Initialize(int thread_pool_size) {
tracing_agent_ = std::make_unique<tracing::Agent>();
node::tracing::TraceEventHelper::SetAgent(tracing_agent_.get());
node::tracing::TracingController* controller =
tracing_agent_->GetTracingController();
trace_state_observer_ =
std::make_unique<NodeTraceStateObserver>(controller);
controller->AddTraceStateObserver(trace_state_observer_.get());
tracing_file_writer_ = tracing_agent_->DefaultHandle();
// Only start the tracing agent if we enabled any tracing categories.
if (!per_process::cli_options->trace_event_categories.empty()) {
StartTracingAgent();
}
// Tracing must be initialized before platform threads are created.
platform_ = new NodePlatform(thread_pool_size, controller);
v8::V8::InitializePlatform(platform_);
}

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 6, 2020

Export InitializeV8Platform to allow use node as a shared library.

You already can with node::CreatePlatform() because conceptually all InitializeV8Platform() does is this:

auto platform = node::CreatePlatform(thread_pool_size, some_stock_tracing_controller);
v8::V8::InitializePlatform(platform);
return platform;

I don't see a good reason to export it. Can you elaborate on what it enables that you can't do with node::CreatePlatform()?

edit: comments just crossed.

@jc-lab
Copy link
Author

jc-lab commented Jan 6, 2020

@bnoordhuis See the comments above.

@bnoordhuis
Copy link
Member

Initialization of per_process::v8_platform seems to only happen on InitializeV8Platform.

Right, so what is probably missing is an API to call V8Platform::Initialize() with a custom MultiIsolatePlatform.

(I won't block this PR but it seems like the wrong approach.)

@addaleax
Copy link
Member

addaleax commented Jan 6, 2020

I used CreatePlatform. However, the call to GetMainThreadMultiIsolatePlatform (Call from NewIsolate) used by worker_threads fails.

@jc-lab That sounds an awful lot like the bug that was fixed by 6db45bf … what version of Node.js are you using?

Initialization of per_process::v8_platform seems to only happen on InitializeV8Platform.

Right, so what is probably missing is an API to call V8Platform::Initialize() with a custom MultiIsolatePlatform.

I’d prefer moving away from the global V8Platform object altogether, rather than exposing even more hooks into it.

@jc-lab
Copy link
Author

jc-lab commented Jan 6, 2020

@addaleax Oops...
I didn't know it was fixed. I used node version 12.4.
Thanks for letting me know.

@jc-lab
Copy link
Author

jc-lab commented Jan 7, 2020

I have a question about node::CreateEnvironment.

node/src/api/environment.cc

Lines 301 to 322 in 2551a21

Environment* CreateEnvironment(IsolateData* isolate_data,
Local<Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv) {
Isolate* isolate = context->GetIsolate();
HandleScope handle_scope(isolate);
Context::Scope context_scope(context);
// TODO(addaleax): This is a much better place for parsing per-Environment
// options than the global parse call.
std::vector<std::string> args(argv, argv + argc);
std::vector<std::string> exec_args(exec_argv, exec_argv + exec_argc);
// TODO(addaleax): Provide more sensible flags, in an embedder-accessible way.
Environment* env = new Environment(
isolate_data,
context,
args,
exec_args,
static_cast<Environment::Flags>(Environment::kIsMainThread |
Environment::kOwnsProcessState |
Environment::kOwnsInspector));

->

node/src/env.cc

Lines 333 to 337 in 2551a21

if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);
TracingController* tracing_controller = writer->GetTracingController();
tracing_controller->AddTraceStateObserver(trace_state_observer_.get());
}

->
inline tracing::AgentWriterHandle* GetTracingAgentWriter() {
return &tracing_file_writer_;
}

Here, V8Platform :: Initialize must be called for tracing_file_writer_ to be initialized when NODE_USE_V8_PLATFORM is 1.
How can I solve this?

@codecov-io
Copy link

codecov-io commented Jan 7, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@95871a1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #31217   +/-   ##
=========================================
  Coverage          ?   97.32%           
=========================================
  Files             ?      192           
  Lines             ?    64162           
  Branches          ?        0           
=========================================
  Hits              ?    62444           
  Misses            ?     1718           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95871a1...8807294. Read the comment docs.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jan 7, 2020
This API method was introduced in commit 90ae4bd ("src: add
InitializeV8Platform function") from July 2018 but wasn't properly
exported and therefore not usable on Windows or with shared library
builds.

The motivation from the commit log is mainly about making it easier
to wire up the cctests and there are better ways to do that.

Refs: nodejs#31217
@addaleax
Copy link
Member

addaleax commented Jan 9, 2020

@jc-lab Are you okay with us closing this in favour of #31245?

@jc-lab
Copy link
Author

jc-lab commented Jan 9, 2020

@addaleax Yes, I close this PR.
I tested it with #31245 commit. Have are a few more things to fix, but #31245 work well in limited situations.

@jc-lab jc-lab closed this Jan 9, 2020
addaleax pushed a commit that referenced this pull request Jan 9, 2020
This requires minor changes to src/env.cc to deal with
`node::tracing::AgentWriterHandle::GetTracingController()` now possibly
returning a nullptr, because the cctest doesn't set one.

It seems plausible to me that embedders won't set one either so that
seems like an okay change to make. It avoids embedders having to track
down nullptr segfaults.

PR-URL: #31245
Refs: #31217
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 9, 2020
This API method was introduced in commit 90ae4bd ("src: add
InitializeV8Platform function") from July 2018 but wasn't properly
exported and therefore not usable on Windows or with shared library
builds.

The motivation from the commit log is mainly about making it easier
to wire up the cctests and there are better ways to do that.

Refs: #31217

PR-URL: #31245
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
This requires minor changes to src/env.cc to deal with
`node::tracing::AgentWriterHandle::GetTracingController()` now possibly
returning a nullptr, because the cctest doesn't set one.

It seems plausible to me that embedders won't set one either so that
seems like an okay change to make. It avoids embedders having to track
down nullptr segfaults.

PR-URL: #31245
Refs: #31217
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
This API method was introduced in commit 90ae4bd ("src: add
InitializeV8Platform function") from July 2018 but wasn't properly
exported and therefore not usable on Windows or with shared library
builds.

The motivation from the commit log is mainly about making it easier
to wire up the cctests and there are better ways to do that.

Refs: #31217

PR-URL: #31245
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 14, 2020
This requires minor changes to src/env.cc to deal with
`node::tracing::AgentWriterHandle::GetTracingController()` now possibly
returning a nullptr, because the cctest doesn't set one.

It seems plausible to me that embedders won't set one either so that
seems like an okay change to make. It avoids embedders having to track
down nullptr segfaults.

PR-URL: #31245
Refs: #31217
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 14, 2020
This API method was introduced in commit 90ae4bd ("src: add
InitializeV8Platform function") from July 2018 but wasn't properly
exported and therefore not usable on Windows or with shared library
builds.

The motivation from the commit log is mainly about making it easier
to wire up the cctests and there are better ways to do that.

Refs: #31217

PR-URL: #31245
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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++. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants