Skip to content
This repository was archived by the owner on Nov 4, 2021. It is now read-only.

Run piscina worker in the main thread if concurrency at 1 - #449

Closed
mariusandra wants to merge 1 commit into
masterfrom
singlethreaded
Closed

Run piscina worker in the main thread if concurrency at 1#449
mariusandra wants to merge 1 commit into
masterfrom
singlethreaded

Conversation

@mariusandra

Copy link
Copy Markdown
Collaborator

Changes

  • WIP. Just an experiment in search of feedback (and of someone to push further)
  • Instead of spawning a piscina worker pool, if concurrency is set to 1, just run everything in the main thread
  • Currently exposes a fake piscina interface, which is not feature complete (no TASKS_PER_WORKER support)
  • Actually makes everything work on a M1 macbook!
  • This could be used to speed up and simplify tests in the future.

Where should we go with this, if anywhere?

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@neilkakkar

Copy link
Copy Markdown
Contributor

Interesting, I was thinking of something similar for the "install/test" pool - since that one would have a single worker.

Not quite sure if it's a great thing, since it means we'll be blocking on tasks in the main thread? (ain't a big deal when we're awaiting the responses anyway, but is a big deal otherwise)


Regarding the implementation: curious if the isWorkerThread setting has implications for resource management / thread killing by Piscina?

@mariusandra

Copy link
Copy Markdown
Collaborator Author

I think in the real world, even with just one thread, we still want the piscina task pool. If for nothing else then to never block the main thread. That separation is precisely what you want with the install/test pool.

The setting in this PR should probably be configured with a special SINGLE_THREADED=true env... and will be mostly useful to simplify tests or in case we have no other way to get it working on Apple Silicon (unlikely).

@Twixes

Twixes commented Jun 1, 2021

Copy link
Copy Markdown
Contributor

I don't quite see why we'd want this, besides having a temporary hack to get the plugin server working on ARM. Tests should generally run the same code as production, shouldn't they?

@mariusandra

Copy link
Copy Markdown
Collaborator Author

We could use this on the lowest heroku tiers as well to save some redis connections...

@mariusandra

Copy link
Copy Markdown
Collaborator Author

I got the plugin server working on the M1, so this could effectively be closed. Having a completely separate code path for cheap heroku users doesn't sound wise.

@mariusandra mariusandra closed this Jun 4, 2021
@mariusandra
mariusandra deleted the singlethreaded branch June 4, 2021 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants