perf(file-validator): improve the performance of file_validator #1078
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR significantly improves the performance of the file validator in the SJMCL Minecraft launcher by implementing concurrent processing with limited parallelism. The changes replace unbounded concurrent execution (join_all) with controlled concurrency using stream processing, reducing execution time from 14 seconds to under 3 seconds in testing.
Key changes:
- Introduced concurrency control with
CONCURRENT_HASH_CHECKSconstant (16 concurrent operations) - Refactored file validation logic into reusable helper functions
- Used
spawn_blockingfor CPU-intensive hash validation tasks - Applied concurrency limits to native library extraction
Comments suppressed due to low confidence (1)
src-tauri/src/launch/helpers/file_validator.rs:1
- The error handling loop can be simplified using iterator methods. Consider using
results.into_iter().collect::<Result<Vec<_>, _>>()?;before the loop to handle all errors at once.
use crate::error::SJMCLResult;
| let native_str = if let Some(native_fn) = Some(&get_natives_string) { | ||
| library.natives.as_ref().and_then(native_fn) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
The conditional assignment is unnecessarily complex. Since native_fn is always Some(&get_natives_string), this can be simplified to directly call library.natives.as_ref().and_then(&get_natives_string).
UNIkeEN
left a comment
UNIkeEN
left a comment
There was a problem hiding this comment.
有几个函数没有改但是调换了位置，有什么原因吗；
没有特别原因的话能不能换回去， diff 好长 上下翻来翻去（xD）
| use url::Url; | ||
| use zip::ZipArchive; | ||
|
|
||
| const CONCURRENT_HASH_CHECKS: usize = 16; |
There was a problem hiding this comment.
hhhh，换回去不太可能，原来的函数顺序太乱了，不过现在这个感觉也不太好，我想再大刀阔斧地改一下） |
|
目前的函数顺序： 数据结构定义 |
| let cpu_count = sys.cpus().len(); | ||
| (cpu_count * 3).max(8).min(32) | ||
| }) | ||
| } |
There was a problem hiding this comment.
我还是感觉这个函数很奇怪，为什么不用
std::thread::available_parallelism().unwrap().into()来获取并发数，而使用自定义的规则
There was a problem hiding this comment.
对于IO密集的任务，这样更好吧（？
如果这个有依据的话，麻烦下封装至utils里面
| Ok(None) | ||
| } | ||
|
|
||
| async fn validate_files_concurrently<T, F, Fut>( |
There was a problem hiding this comment.
原来 validate_file_with_hash 不是内嵌在这里面而是需要在 processor 里啊，我感觉如果并发数的逻辑改成 std 后，这个函数不是很有必要复用（至少它看起来就更像一个通用并发 utils helper 了）
|
我还是觉得需要用std而不是自定义规则，如果采纳并最终形成合理自定义规则的话，应该需要做成utils 具体 cc @ToolmanP 审查与决定 另外，此 PR 修改了此前的一些代码风格问题（比如使用get数组下标替代先判断长度做if-else），👍 |
UNIkeEN
added
the
🔜 Review Priority
| std::thread::sleep(sysinfo::MINIMUM_CPU_UPDATE_INTERVAL); | ||
| sys.refresh_cpu_usage(); | ||
| let cpu_count = sys.cpus().len(); | ||
| (cpu_count * 3).max(8).min(32) |
There was a problem hiding this comment.
单纯就是对于这种部分 IO 阻塞，然后 CPU 哈希计算（就是这种实际情况）的场景下的一个比较好的数值吧，这你一定要说依据好像也没什么依据啊）
这应该已经很不魔法数字了吧），如果真要依据那估计就得用 rayon 了（乐，这个我是改不动
| let cpu_count = sys.cpus().len(); | ||
| (cpu_count * 3).max(8).min(32) | ||
| }) | ||
| } |
There was a problem hiding this comment.
对于IO密集的任务，这样更好吧（？
如果这个有依据的话，麻烦下封装至utils里面
| F: Fn(T, bool) -> Fut + Send + Sync + Clone + 'static, | ||
| Fut: std::future::Future<Output = SJMCLResult<Option<PTaskParam>>> + Send, | ||
| { | ||
| let max_concurrent = get_concurrent_limit(3.0); |
4 participants
Checklist
This PR is a ..
Related Issues
Description
性能提升
实测在 1.21.1 NeoForge 的相同环境下从 14s 提高到了不到 3s。
源代码使用
join_all同时启动所有文件校验任务，导致大量 IO 操作，线程阻塞。使用限制并发读取带来了大量的性能优化。其次，使用 spawn_blocking 将任务移动到单独的线程池，这也是这类 CPU 密集的工作更好的地方。
解压 natives 库也做了并发限制。
其他地方，提取了一些重复的逻辑，做了一些小代码更改。
Additional Context