Improve parallelism for 'moon test'

[lgxbslgx]

Currently, when using the command moon test, moon will create 16 threads by using the tokio library. But these 16 threads will never be used after creating. And these 16 threads become more strange when combining with the argument --no-parallel. It is a misuse of the library tokio.

This patch, which want to fix the issue, contains the following features:

• When the --no-parallel is not provided, run the tests parallelly
  • the thread count is equal to the logic cores (not the constant 16)
  • spawn the test tasks to the tokio runtime scheduler (use the threads tokio created to run the test)
• When the --no-parallel is provided, run the tests sequentially
  • don't create new thread
  • run the test one by one in current thread

Thanks for your review.

Related Issues

• Related issues: #____

Type of Pull Request

• Bug fix
• New feature
  • I have already discussed this feature with the maintainers.
• Refactor
• Documentation
• Other (please describe): Improve performance

Does this PR change existing behavior?

• Yes (please describe the changes below)
• No

Does this PR introduce new dependencies?

• No
• Yes (please check binary size changes)

Checklist:

• Tests added/updated for bug fixes or new features
• Compatible with Windows/Linux/macOS

[peter-jerry-ye-code-review]

From the provided git diff output, I observe several potential issues and suggestions for improvements in the code:

1. Unnecessary Borrowing of References:

   -        &moonc_opt,
   -        &moonbuild_opt,
   +        moonc_opt,
   +        moonbuild_opt,

   In Rust, passing variables by reference when they are not mutated within the function can be unnecessary. Here, moonc_opt and moonbuild_opt are passed by value, which is more idiomatic if they are not mutated inside the function. This change simplifies the function signature and reduces overhead.

2. Arc::new Usage for Concurrency:

   +    let moonc_opt = Arc::new(moonc_opt);
   +    let moonbuild_opt = Arc::new(moonbuild_opt);
   +    let module = Arc::new(module);

   This change suggests that moonc_opt, moonbuild_opt, and module are being shared across multiple threads. Using Arc::new (Atomic Reference Counting) ensures that these variables are safely shared across threads. This is crucial for concurrency safety in Rust.

3. Order of Initialization and Usage:

   -    let backend_hint = moonbuild_opt
   -        .test_opt
   -        .as_ref()
   -        .and_then(|opt| opt.display_backend_hint.as_ref())
   -        .map(|_| format!(" [{}]", moonc_opt.build_opt.target_backend.to_backend_ext()))
   -        .unwrap_or_default();

   The original code computes backend_hint before using it, but in the revised version, it seems to be moved to a later part of the function. Ensure that backend_hint is still computed and used appropriately, especially if it affects the output or logic flow of the function.

4. Tokio Runtime Configuration:

   +        let runtime = tokio::runtime::Builder::new_multi_thread()
   +            .enable_all()
   +            .build()?;

   The addition of a Tokio runtime builder suggests that the code is being optimized for concurrent execution. Ensure that the number of worker threads (worker_threads(16)) is appropriate for the target system's capabilities and the expected workload. This configuration can significantly impact performance.

5. Sequential vs Parallel Execution:

   +        runtime.block_on(async {
   +            let mut res_handlers = vec![];
   +            for handler in handlers {
   +                // Submit tasks to the scheduler
   +                res_handlers.push(runtime.spawn(handler));
   +            }
   +            futures::future::join_all(res_handlers)
   +                .await
   +                .into_iter()
   +                .map(|res| res.unwrap())
   +                .collect()
   +        })

   The revised code for handling concurrent tasks uses runtime.spawn to submit tasks to the Tokio scheduler. This approach is more efficient for parallel execution compared to the original sequential handling. Ensure that the error handling (res.unwrap()) is appropriate for your application, as unwrapping can lead to panic in case of errors.

These observations focus on efficiency, safety, and idiomatic Rust practices. Adjustments should be made based on the specific requirements and constraints of your application.

[lgxbslgx]

I compare the run time of the main branch and this patch in the standard library core by using the command time moon test simply. Unfortunately, their run time are almost the same. My local env is 2 cores & 4 theads. Could you help me test the performance in a machine with multi cores (if you have)? Thanks.

[Young-Flash]

why change the param type from &MoonbuildOpt to MoonbuildOpt and use Arc::clone() below?

[lgxbslgx]

MoonbuildOpt(and MooncOpt and ModuleDB) need to be shared in multiple threads, so Arc is needed. And the Arc::new want to move the value instead of getting a reference, so I change the parameter.

[Young-Flash]

do we need another res_handlers and spawn here? IMO the previous runtime.block_on(futures::future::join_all(handlers)) is easy to understand

[lgxbslgx]

When using runtime.block_on(futures::future::join_all(handlers)), the threads created by tokio are never used. Because the tasks are never spawned/submitted to the scheduler, all of them are run in the current thread. You can debug the code or/and print the thread id in method entry.rs::execute_test to verify it.

[Young-Flash]

Thanks!