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

Add Concurrency. #52

Merged
merged 4 commits into from
Nov 28, 2019
Merged

Add Concurrency. #52

merged 4 commits into from
Nov 28, 2019

Conversation

JeremyTCD
Copy link
Member

  • Multi-threading using workers.
  • Multi-processing using a pool of INodeJSServices.

@DaniilSokolyuk
Copy link
Contributor

DaniilSokolyuk commented Nov 23, 2019

@JeremyTCD
It seems to me that clients can implement multithreading themselves? There can be a huge number of different options, like:

  • queue size
  • worker max usages
  • different strategies to reduce or increase the size of the pool

For multi-processing - cool, less overhead compared to a cluster

@JeremyTCD
Copy link
Member Author

JeremyTCD commented Nov 23, 2019

If OutOfProcessNodeJSServiceOptions.Parallelization is Parallelization.None (default), it'll work the same as right now. Clients can use that mode if they want to handle threading. I suspect not every user will be familiar enough with node to handle threading on their own though.

For multi-processing - cool, less overhead compared to a cluster

Yeah agreed, tried cluster it's really slow. Requests go to a master then get sent by IPC to workers, so 2x latency.

I think this implementation may be faster than basic worker threads. We'll see.

- Separated into latency, concurrency and real workload categories.
@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #52 into master will decrease coverage by 0.55%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   97.36%   96.81%   -0.56%     
==========================================
  Files          17       18       +1     
  Lines         494      533      +39     
==========================================
+ Hits          481      516      +35     
- Misses         13       17       +4
Impacted Files Coverage Δ
src/NodeJS/NodeJSServiceCollectionExtensions.cs 100% <100%> (ø) ⬆️
...s/OutOfProcess/OutOfProcessNodeJSServiceOptions.cs 100% <100%> (ø) ⬆️
...ntations/OutOfProcess/OutOfProcessNodeJSService.cs 97.93% <100%> (-0.03%) ⬇️
...tations/OutOfProcess/Http/HttpNodeJSPoolService.cs 85.18% <85.18%> (ø)

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 2a05059...daf3fcb. Read the comment docs.

@JeremyTCD JeremyTCD force-pushed the Parallelization branch 4 times, most recently from 670aa95 to 18a3550 Compare November 28, 2019 15:06
@JeremyTCD JeremyTCD merged commit 446af1c into master Nov 28, 2019
@JeremyTCD JeremyTCD deleted the Parallelization branch November 28, 2019 15:20
@JeremyTCD
Copy link
Member Author

Published 5.1.0 with out-of-the-box multi-process concurrency.

Decided not to implement multi-threading using workers:

  • Tricky passing stuff to and from threads, without major changes to HttpServer.ts, can't pass everything via transferlist. Can't pass streams either.
  • Not all Node.js modules are available within workers.
  • Build tools aren't mature yet. Webpack's worker-loader can't inline worker threads.

API allows for adding it in future though.

@JeremyTCD JeremyTCD changed the title Add Parallelization. Add Concurrency. Nov 30, 2019
@JeremyTCD JeremyTCD linked an issue Apr 2, 2020 that may be closed by this pull request
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.

Pooling support for high request rates?
2 participants