-
Notifications
You must be signed in to change notification settings - Fork 3
feat(hermes): Add thread pool for wasm modules parallel execution #488
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
Conversation
f22890e
to
cdc7c8c
Compare
d7bd604
to
2ee63a6
Compare
b422017
to
9ff8a43
Compare
17ee679
to
f64deab
Compare
hermes/bin/tests/integration/components/sleep_component/src/lib.rs
Outdated
Show resolved
Hide resolved
Now blocked by issue. |
* feat(hermes): add unit test for parallel execution, rewrite integration test to use db * chore(hermes): remove env var, add comment describing the reason why we don't need it * fix(hermes): add busy_handler that always retries SQLITE_BUSY * fix(hermes): updated sqlite_busy retry delay * feat(hermes): add WAL mode * chore(hermes): link issue to TODO comment --------- Co-authored-by: Steven Johnson <stevenj@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I left two nits.
hermes/bin/tests/integration/tests/serial/parallel_module_execution.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
use rayon::ThreadPoolBuilder; | ||
|
||
/// Global counter of currently running tasks. | ||
static TASK_COUNTER: AtomicUsize = AtomicUsize::new(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it a usize? What relationship does it have to pointers or memory? Is it used for indexing?
Unless its got something to do with memory (an index or pointer math) we should pick an appropriately sized variable type so its 100% consistent regardless of platform.
Given that its actually counting concurrently running threads, it probably never needs to be bigger than u16.
static TASK_COUNTER: AtomicUsize = AtomicUsize::new(0); | ||
|
||
/// Synchronization primitives for waiting until all tasks finish. | ||
static TASK_WAIT: (Mutex<()>, Condvar) = (Mutex::new(()), Condvar::new()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel if the aim here is to build a countdown latch, then we should have a countdown latch type which does that.
Seems like this is the sort of thing we would use in other places.
It would also be easier to test, in isolation.
would also make the intent of it here more obvious.
Description
This PR adds thread pool, which makes possible parallel execution of wasm modules. Also it provides test, which checks that execution runs in parallel.
Related Issue(s)
Closes #81
Description of Changes
rayon
thread pool)tests/integration/components/sleep_component
tests/integration/tests/serial/parallel_module_execution.rs
which can be managed viacargo test
hermes/bin/tests/integration/tests/serial/utils/
to be more generic in usageBreaking Changes
Describe any breaking changes and the impact.
Please confirm the following checks