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: Use worker threads in render #939

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

hegelocampus
Copy link

@hegelocampus hegelocampus commented Jan 22, 2020

This isn't quite in a working state but I think it's at least 75% of the way there right now.

I've hit a bit of a snag with using the js.dart package and I'm pretty sure I'm doing something very wrong in lib/src/node/worker_threads.dart but I can't quite figure out exactly what.
The present issue is that essentially that nothing is actually being imported by node. My first approach was very similar to what is suggested in the api docs, but nothing was defined. So then I tried to implement the same JS require interop that I found being done in test/node_api/intercept_stdout.dart, but I'm not sure if my translation of that implementation makes sense either.

If someone could point me in the right direction as to how to use the js.dart package to import something that is not globally accessible, I would really appreciate it because I feel like I've my lost my footing and don't really know how one would/should go about importing worker_threads for use in Dart.

Todo:

  • Fix lib/src/node/worker_threads.dart
  • Clean up worker thread implementation in lib/src/node.js
  • Check that all of tests that are no longer relevant have been removed
  • Do performance tests to see benefits attained through the use of a worker thread
  • Possibly more...

Implements #868

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

I'm sorry you have to face the nightmare that is Dart's JS interop... it's confusing, incomplete, and very poorly documented. I'll try to walk you through the necessary steps.

First, you don't need to worry about manually simulating a require()—there's an easier way. In the JS compilation tooling, we add a few requires to the beginning of the JS output file. You can add a worker_thread require there as well, and then in the JS interop worker_thread will just be a pre-defined name.

The reason your existing code isn't working is because, when you call require() through JS interop, the object being returned isn't globally available. As far as Dart's concerned, it's just some object somewhere in the program, and it doesn't know that it matches the worker_threads module name that you declared for the rest of the library.

@ronkorving
Copy link

ronkorving commented Apr 7, 2020

Hi, very interested party here :) Am very happy with this undertaking. Is this still going places?

@hegelocampus hegelocampus force-pushed the use-worker-threads-to-implement-render branch from 1cf0faa to 4d6caaa Compare April 7, 2020 17:52
@hegelocampus
Copy link
Author

Hi, @ronkorving, I had a major change in the amount of free time I have and I definitely don't have time to complete this so it's all yours if you want it. I pushed all of my local changes to this branch and you can feel free to use those as a starting place. Good luck and godspeed.

@ronkorving
Copy link

@hegelocampus First off, I appreciate your work so far. But I need to be very clear about my own situation... I am not in a situation right now or anytime soon where I can make this kind of contribution for a host of reasons. I hope the SASS community can help out, as this seems to be very important work (especially since fibers now officially printed a warning of obsolescence).

@nex3 nex3 marked this pull request as draft February 6, 2023 23:28
@nex3 nex3 force-pushed the main branch 2 times, most recently from 9cff4ca to 6c59213 Compare July 21, 2023 01:57
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.

3 participants