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

[WIP] Add WebWorker API #1261

Closed
wants to merge 12 commits into from
Closed

Conversation

bartlomieju
Copy link
Member

Ref #1222

ATM I am getting this error during build:

Undefined symbols for architecture x86_64:
  "v8::Shell::WorkerTerminate(v8::FunctionCallbackInfo<v8::Value> const&)", referenced from:
      deno::CreateGlobalTemplate(v8::Isolate*) in libdeno.a(binding.o)
  "v8::Shell::WorkerGetMessage(v8::FunctionCallbackInfo<v8::Value> const&)", referenced from:
      deno::CreateGlobalTemplate(v8::Isolate*) in libdeno.a(binding.o)
  "v8::Shell::WorkerPostMessage(v8::FunctionCallbackInfo<v8::Value> const&)", referenced from:
      deno::CreateGlobalTemplate(v8::Isolate*) in libdeno.a(binding.o)
  "v8::Shell::WorkerNew(v8::FunctionCallbackInfo<v8::Value> const&)", referenced from:
      deno::CreateGlobalTemplate(v8::Isolate*) in libdeno.a(binding.o)

@ry
Copy link
Member

ry commented Dec 1, 2018

Interesting!

v8::Shell functions have to be copied over. Start a new file called libdeno/worker.cc and add them. You can grep for the locations in third_party/v8

> grep -R WorkerTerminate third_party/v8/
third_party/v8//src/d8.cc:void Shell::WorkerTerminate(const v8::FunctionCallbackInfo<v8::Value>& args) {
third_party/v8//src/d8.cc:      FunctionTemplate::New(isolate, WorkerTerminate, Local<Value>(),
third_party/v8//src/d8.h:  static void WorkerTerminate(const v8::FunctionCallbackInfo<v8::Value>& args);

@bartlomieju
Copy link
Member Author

I started with WorkerNew. It required copying a lot of code from v8/src/d8.cc and I can't get rid of those two errors:

In file included from ../../libdeno/binding.cc:13:
../../libdeno/worker.cc:108:42: error: incomplete type 'v8::internal::String' named in nested name specifier
  if (i::FLAG_use_external_strings && i::String::IsAscii(chars, size)) {
                                      ~~~^~~~~~~~
../../third_party/v8/src/globals.h:528:7: note: forward declaration of 'v8::internal::String'
class String;
      ^
In file included from ../../libdeno/binding.cc:13:
../../libdeno/worker.cc:187:14: error: no matching member function for call to 'push_back'
    workers_.push_back(worker);
    ~~~~~~~~~^~~~~~~~~
../../third_party/llvm-build/Release+Asserts/include/c++/v1/vector:707:36: note: candidate function not viable: no known conversion from 'v8::Worker *' to 'const std::__1::__vector_base<deno::Worker *, std::__1::allocator<deno::Worker *> >::value_type' (aka 'deno::Worker *const') for 1st argument
    _LIBCPP_INLINE_VISIBILITY void push_back(const_reference __x);
                                   ^
../../third_party/llvm-build/Release+Asserts/include/c++/v1/vector:710:36: note: candidate function not viable: no known conversion from 'v8::Worker *' to 'std::__1::vector<deno::Worker *, std::__1::allocator<deno::Worker *> >::value_type' (aka 'deno::Worker *') for 1st argument
    _LIBCPP_INLINE_VISIBILITY void push_back(value_type&& __x);

Is that the right direction @ry?

@ry
Copy link
Member

ry commented Dec 1, 2018

Ya - you will have to make some heavy modifications to get it to work. I will give some more detailed comments later

Be sure to update build.gn with the new source file

@bartlomieju
Copy link
Member Author

Awesome!

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Dec 2, 2018

Just curious (sorry for not really following the discussion about workers recently in Node), are we going to have separate event loops for each worker thread, or are we going to share a single event loop across all worker threads?

Also I believe we also need to implement messaging together with Workers

@kitsonk
Copy link
Contributor

kitsonk commented Dec 2, 2018

It should match the specification IMO, which is a seperate event loop.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

This is obviously a larger project - and I'm glad you're starting it - so far so good. Maybe add tests/worker.js with an extremely simple worker test - and see how far you can get towards running it. Feel free to ping me on gitter or email for help.

@@ -30,12 +30,13 @@ Deno* deno_new(deno_buf snapshot, deno_buf shared, deno_recv_cb cb) {
v8::Locker locker(isolate);
v8::Isolate::Scope isolate_scope(isolate);
{
v8::Local<v8::ObjectTemplate> global_template =
deno::CreateGlobalTemplate(isolate);
Copy link
Member

Choose a reason for hiding this comment

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

This works. I would consider using our existing deno::InitializeContext for setting up the global variables... However using a ObjectTemplate is probably a better option - and how D8 is doing it - so don't worry about this nit for now.

libdeno/worker.cc Outdated Show resolved Hide resolved
libdeno/worker.cc Show resolved Hide resolved
libdeno/worker.cc Outdated Show resolved Hide resolved
libdeno/worker.cc Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

bartlomieju commented Dec 4, 2018

@ry I've started with your feedback.

I put some placeholder methods (like Worker::ExecuteInThread) to get compiler happy, but that resulted in:

$ ./tools/test.py
[...]
[----------] 17 tests from LibDenoTest
[ RUN      ] LibDenoTest.InitializesCorrectly

#
# Fatal error in v8::HandleScope::CreateHandle()
# Cannot create a handle without a HandleScope
#

Digging further!

@ry
Copy link
Member

ry commented Dec 4, 2018

@bartlomieju In V8's binding API, basically every function needs to declare a HandleScope for storing local variables. I suggest reading https://v8.dev/docs/embed
For debugging such errors, if a line number isn't printed, you should use a debugger like lldb or gdb. I often debug deno like this:

lldb -- target/debug/deno tests/worker.js
> run
> bt
> up
> up
> l

@ry
Copy link
Member

ry commented Dec 4, 2018

@kevinkassimo Sorry didn't see your message until now. I'd like the different workers to share the same Tokio runtime.

@bartlomieju
Copy link
Member Author

Thanks for tips @ry great read.

There is some progress now, actual worker instantiation is called (run deno tests/test_worker.ts) but some error when reading worker file emerges. I'll get to that tomorrow and keep going.

@ry
Copy link
Member

ry commented Dec 12, 2018

I've started an alternative implementation in rust here #1331

@bartlomieju
Copy link
Member Author

Closing in favor of #1331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants