-
-
Notifications
You must be signed in to change notification settings - Fork 112
refactor: kind of everything #421
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
base: main
Are you sure you want to change the base?
Conversation
- Optimize Loc::new() to eliminate string allocations during byte-to-char conversion - Replace HashMap with IndexMap for better cache performance in models - Use SmallVec for commonly small collections (ranges, statements, declarations) - Flatten cache structure with combined keys for better memory layout - Optimize merge operations to use HashSet for O(1) lookup instead of dedup_by - Add helper functions for efficient data structure conversions - Use more efficient parallel processing patterns Co-authored-by: MuntasirSZN <[email protected]>
- Update miri_tests.rs to use constructors instead of direct struct initialization - Replace Vec::new() with appropriate SmallVec constructors in tests - Fix capacity assertions to match SmallVec minimums - Ensure all tests compile and run correctly with new optimized data structures Co-authored-by: MuntasirSZN <[email protected]>
…omprehensive configuration Co-authored-by: MuntasirSZN <[email protected]>
Co-authored-by: MuntasirSZN <[email protected]>
…timizations Co-authored-by: MuntasirSZN <[email protected]>
chore: another clippy fix Update docs/cache-configuration.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Update src/bin/core/cache.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Update src/cache.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> remove allow unused
Co-authored-by: MuntasirSZN <[email protected]>
…used code Co-authored-by: MuntasirSZN <[email protected]>
… checks Co-authored-by: MuntasirSZN <[email protected]>
Co-authored-by: MuntasirSZN <[email protected]>
Co-authored-by: MuntasirSZN <[email protected]>
…opying code Co-authored-by: MuntasirSZN <[email protected]>
Co-authored-by: MuntasirSZN <[email protected]>
Co-authored-by: MuntasirSZN <[email protected]>
Co-authored-by: MuntasirSZN <[email protected]>
|
Also pls set up codecov, manual code coverage is error prone |
|
Also codspeed to continuous benches |
|
@MuntasirSZN There are a lot of changes, and you didn't provide enough description of the motivation. I felt most optimization is not needed, and it makes reading the code difficult. |
I disagree. I myself cannot run rustowl, just because it just is tooooo much resource hungry. Everyone doesnt run beefy machines. Also about code, it isnt that hard, just some optimizations that make it faster, and they are known to many people (caching). |
|
Yes, RustOwl needs many resources. But your change resolves the situation significantly? I didn't measured, but if you can, please describe and show the data. I feel the change sacrifices readability. |
Ok i will. I am busy for some days so it will take some time |
|
| Metric | Value |
|---|---|
| User time (wall clock time) | 67.681s |
| System time | 12.357s |
this branch
Time (mean ± σ): 25.153 s ± 2.306 s [User: 59.941 s, System: 11.364 s]
Range (min … max): 22.177 s … 30.916 s 10 runs
| Metric | Value |
|---|---|
| User time (wall clock time) | 59.941s |
| System time | 11.364s |
Summary
- This branch is about 25.8% faster than
mainin cold build scenario.
Warm build (aka second run with cache)
- ~/.rustowl dir exists, no download needed
- Cache exists, only incremental check needed
command: hyperfine "rm -rf ./target/debug/rustowl check ./perf-tests/dummy-package"
main branch
Time (mean ± σ): 10.729 s ± 0.661 s [User: 1.229 s, System: 0.692 s]
Range (min … max): 10.112 s … 11.943 s 10 runs
| Metric | Value |
|---|---|
| User time (wall clock time) | 1.229s |
| System time | 0.692s |
this branch
Time (mean ± σ): 1.341 s ± 0.072 s [User: 0.878 s, System: 0.265 s]
Range (min … max): 1.227 s … 1.444 s 10 runs
| Metric | Value |
|---|---|
| User time (wall clock time) | 1.341s |
| System time | 0.265s |
Summary
- This branch is about 87.5% faster than
mainin warm build scenario.
|
Okay |
|
@cordx56 can you do a announcement on github discussions and discord with [at]everyone to test this as much as possible? After merge of course, to minimize as much bugs as possible. Currently its 99% complete, the xtask refactor will be another pr. Pls review completely. |
cordx56
left a comment
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.
reviewing
| body.var_debug_info.len(), | ||
| foldhash::quality::RandomState::default(), | ||
| ); | ||
| for debug in &body.var_debug_info { |
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 didn't you use iterator idiom? I feel this loop idiom is too redundant.
|
|
||
| let mut result = EcoVec::with_capacity(basic_blocks.len()); | ||
|
|
||
| for (_bb, bb_data) in basic_blocks.iter_enumerated() { |
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.
Same as above.
src/utils.rs
Outdated
| /// `rustc` byte positions behave as if `\r` bytes do not exist in the source. | ||
| /// `Loc` is a *logical character index* where `\r` is ignored too. | ||
| /// | ||
| /// `Loc::new(source, byte_pos, offset)` previously scanned `source` on every |
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 thought the comment about previous implementation is needless.
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.
same. please left any comments if you resolve the conversation without fixing the code.
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 removed comment? @cordx56
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.
Do you want to remove last two lines? (though rgey are just context)
|
@MuntasirSZN checks failed. please fix |
cordx56
left a comment
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.
Please read my comment carefully.
|
|
||
| impl Analyzer { | ||
| pub async fn new(path: impl AsRef<Path>, rustc_threads: usize) -> Result<Self, ()> { | ||
| pub async fn new(path: impl AsRef<Path>, rustc_threads: usize) -> Result<Self> { |
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 you remove second type argument?
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.
There arent any errors expected?
src/toolchain.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn test_recursive_read_dir_non_existent() { |
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 you resolve this comment without any comments?
| } | ||
|
|
||
| #[test] | ||
| fn test_file_operations() { |
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 you resolved this without editing or commenting?
| } | ||
|
|
||
| #[test] | ||
| fn test_shell_error_message_patterns() { |
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.
same
src/utils.rs
Outdated
| /// `rustc` byte positions behave as if `\r` bytes do not exist in the source. | ||
| /// `Loc` is a *logical character index* where `\r` is ignored too. | ||
| /// | ||
| /// `Loc::new(source, byte_pos, offset)` previously scanned `source` on every |
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.
same. please left any comments if you resolve the conversation without fixing the code.
src/lsp/analyze.rs
Outdated
| pub async fn new(path: impl AsRef<Path>, rustc_threads: usize) -> Result<Self> { | ||
| let path = path.as_ref().to_path_buf(); | ||
|
|
||
| // `cargo metadata` may invoke `rustc` for target information. It must use a real |
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.
false. setup_cargo_command does not use rustc. rustc version may differ from rustowlc version.
|
Some comments are wrong. Please check carefully before writing docs and codes. |
|
@cordx56 sorry before i say anything, but as a maintainer you can do some edits too instead of just asking me. I am saying this because i just changed houses and i am busy as heck. Also you have much more knowledge about this then me (expected). So pls solve any issues as you see put, in any case if you want my opinion write a review comment. Sorry again...... |
- Update to use reqwest v0.13 (we now use default features as some are default) - Remove rustls (aws_lc is now used by default) - No more rustls native roots, we use rustls platform verifier (better) which uses aws_lc and others according to platform (not openssl though)
No description provided.