[move-analyzer] Cache user program #20621

awelc · 2024-12-13T13:50:23Z

Description

This PR builds upon the compiler changes supporting partial compilation of of user code (skipping function bodies when compiling specific files designated by the user) to deliver the performance improvements described in the PR containing the compiler changes (reducing compilation time in the IDE for the Deepbook package from over 1s to less than 200ms).

In this PR, in addition to caching dependencies, we also cache user code, fully compile only modified files, and merge result of this compilation with the cached data

Test plan

All tests must pass. Also tested manually to verify correctness and performance improvements

vercel · 2024-12-13T13:50:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 17, 2024 9:58pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Dec 17, 2024 9:58pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Dec 17, 2024 9:58pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Dec 17, 2024 9:58pm

awelc · 2024-12-13T13:59:36Z

external-crates/move/crates/move-analyzer/src/symbols.rs

    /// Compiled user program
    program: CompiledProgram,
-    /// Source files


Somewhat unrelated, but I realized we don't need both

cgswords · 2024-12-13T20:31:14Z

external-crates/move/crates/move-analyzer/tests/ide_testsuite.rs

@@ -381,6 +381,7 @@ fn initial_symbols(
        pkg_deps.clone(),
        ide_files_root.clone(),
        project_path.as_path(),
+        None,
        LintLevel::None,
    )?;



Can we add a test with some precompiled info just for sanity checking?

We need a full compilation to compute all symbols and this is the first and only one that's done per test suite (as files do not change). What I did, though, is to add another partial compilation right after this one and use its results to run tests, but it lengthens the time to execute the test suite (not to unreasonable level, though)

cgswords · 2024-12-13T20:34:46Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+                            // if one of the paths for a package is not a Move file, have aonly one BTreeSet
+                            // None entry. This will be later passed to symbolicator as Some(Vec<PathBuf>) or
+                            // None, to indicate if incremental symbolication is possible or not.
+                            let root_dir = root_dir_opt.unwrap();


Small nit: possibly swap this for a let-else

Good idea! Thanks!

cgswords · 2024-12-13T23:53:47Z

external-crates/move/crates/move-analyzer/src/symbols.rs

@@ -1406,10 +1413,10 @@ impl SymbolicatorRunner {
                        }
                    };
                    if let Some(all_starting_paths) = all_starting_paths_opt {


I am really confused about the naming of the variables in this code here:

Why Some(root_dir) when that is a set?

What does all_starting_paths mean?

Sorry about that... The first one was totally my bad when changing the original code in the the pre-requisite PR (forgot to rename this following the change of what is passed in the mutex). The second one (all_starting_paths) was introduced there as well to distinguish from starting_path as we were previously only tracking a single (modified file).

I not changed them both to simply starting_paths

cgswords · 2024-12-13T23:55:15Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+                            // None entry. This will be later passed to symbolicator as Some(Vec<PathBuf>) or
+                            // None, to indicate if incremental symbolication is possible or not.
+                            let root_dir = root_dir_opt.unwrap();
+                            if let Some(mut modified_files) = pkgs_to_analyze.remove(&root_dir) {


Is there a reason to remove and insert vs simply using entry with the appropriate std::mem::replace?

No reason other than me failing to thing Rust-way sometimes... Rewritten!

cgswords · 2024-12-13T23:56:06Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+                            let root_dir = root_dir_opt.unwrap();
+                            if let Some(mut modified_files) = pkgs_to_analyze.remove(&root_dir) {
+                                // we already seen this package
+                                if modified_files.len() != 1 || modified_files.first().is_some() {


What is this testing for and why not just !modified_files.is_empty()?

It could be !modified_files.is_empty(). The intention was to represent a situation when modified_files has only one entry and this entry is None, but since an entry cannot be None if other entries exist, checking of non-emptiness is totally fine as well. In either case, with the simplifying edit, no longer needed

cgswords · 2024-12-13T23:56:57Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+                                        pkgs_to_analyze.insert(root_dir, modified_files);
+                                    } else {
+                                        // reset to a single None entry
+                                        pkgs_to_analyze.insert(root_dir,  BTreeSet::from_iter(vec![None]));


Can you explain this? If we have a bunch of modified move files and one non-move file, why not at least consider the move files?

On the second look, I overdid it as there is no way that there can be non-Move file paths in this function (though they can exist as input to get_symbols). I now simply collect the file paths

cgswords · 2024-12-13T23:57:18Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+                                    }
+                                } // else we already seen this package with a non-Move-file path so nothing to do
+                            } else {
+                                // first time we see this package


It seems like this logic could be combined with the above logic if we used entry

Logic changed so no longer needed

cgswords · 2024-12-13T23:58:32Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+                return mdef.loc.file_hash() == *fhash;
+            }
+        }
+        false


No strong preference, but could write as:

file_hashes.get(fpath) .and_then(|fhash| match &pkg_def.def { P::Definition::Module(mdef) => Some((mdef, fhash)), _ => None, }) .map(|(mdef, fhash)| mdef.loc.file_hash() == *fhash) .unwrap_or(false) ```

I know it's more Rust-y but with extra transformation via match and map not sure if it's more readable. If you don't have a strong preference, I think I'd leave it as is

Actually, since I decided to follow the other suggestion and rewrite the other similar function, let's rewrite this one as well for consistency

cgswords · 2024-12-13T23:59:31Z

external-crates/move/crates/move-analyzer/src/symbols.rs

+        if let Some(fhash) = file_hashes.get(fpath) {
+            return mdef.loc.file_hash() == *fhash;
+        }
+        false


Suggested change

if let Some(fhash) = file_hashes.get(fpath) {

return mdef.loc.file_hash() == *fhash;

}

false

file_hashes.get(fpath)

.map(|fhash| mdef.loc.file_hash() == *fhash)

.unwrap_or(false)

Thanks! An arguably even more Rust-y one would be:

file_hashes .get(fpath) .map_or(false, |fhash| mdef.loc.file_hash() == *fhash)

cgswords

LGTM modulo the code in new for SymbolicatorRunner, which is opaque and very hard to follow. There are several bits that do unexpected things. Refactoring that massive constructor into smaller functions that do clear things would help quite a bit, and make the code that much easier to maintain going forward.

awelc · 2024-12-16T13:33:54Z

LGTM modulo the code in new for SymbolicatorRunner, which is opaque and very hard to follow. There are several bits that do unexpected things. Refactoring that massive constructor into smaller functions that do clear things would help quite a bit, and make the code that much easier to maintain going forward.

Moved aggregation of package paths into a separate function which was the part that seemed to make the most sense to refactor

awelc requested a review from cgswords December 13, 2024 13:50

awelc self-assigned this Dec 13, 2024

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 13, 2024 13:50 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 13, 2024 13:51 View deployment

awelc requested a review from dariorussi December 13, 2024 13:58

awelc commented Dec 13, 2024

View reviewed changes

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 13, 2024 14:18 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 13, 2024 14:19 View deployment

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 13, 2024 16:40 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 13, 2024 16:42 View deployment

awelc enabled auto-merge (squash) December 13, 2024 16:46

cgswords reviewed Dec 13, 2024

View reviewed changes

cgswords approved these changes Dec 14, 2024

View reviewed changes

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 16, 2024 14:27 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 16, 2024 14:28 View deployment

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 16, 2024 14:48 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 16, 2024 14:49 View deployment

awelc force-pushed the aw/ide-user-code-cache branch from ffeedb8 to 95d47ba Compare December 16, 2024 15:57

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 16, 2024 15:57 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 16, 2024 15:59 View deployment

awelc force-pushed the aw/ide-user-code-cache branch from 95d47ba to 330f240 Compare December 16, 2024 21:04

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 16, 2024 21:04 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 16, 2024 21:06 View deployment

awelc force-pushed the aw/ide-user-code-cache branch from 330f240 to 02a3129 Compare December 17, 2024 08:51

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 17, 2024 08:51 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 17, 2024 08:53 View deployment

awelc added 7 commits December 17, 2024 22:56

[move-analyzer] Cache user program

741f7d2

Cosmetic changes

ae02184

Fixed collection of files to analyze

8f5c4dc

Made clippy happy

167b9a6

Addressed review comments

2a81753

Addressed more review comments

fba1bf6

Made clippy happy

40ac4e2

awelc force-pushed the aw/ide-user-code-cache branch from 02a3129 to 40ac4e2 Compare December 17, 2024 21:56

awelc temporarily deployed to sui-typescript-aws-kms-test-env December 17, 2024 21:56 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 17, 2024 21:58 View deployment

awelc merged commit ac03618 into main Dec 18, 2024
52 checks passed

awelc deleted the aw/ide-user-code-cache branch December 18, 2024 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-analyzer] Cache user program #20621

[move-analyzer] Cache user program #20621

awelc commented Dec 13, 2024

vercel bot commented Dec 13, 2024 •

edited

Loading

awelc Dec 13, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024 •

edited

Loading

cgswords Dec 13, 2024

awelc Dec 16, 2024 •

edited

Loading

cgswords Dec 13, 2024

awelc Dec 16, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024

awelc Dec 16, 2024

cgswords Dec 13, 2024

awelc Dec 16, 2024

cgswords left a comment

awelc commented Dec 16, 2024

[move-analyzer] Cache user program #20621

[move-analyzer] Cache user program #20621

Conversation

awelc commented Dec 13, 2024

Description

Test plan

vercel bot commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgswords left a comment

Choose a reason for hiding this comment

awelc commented Dec 16, 2024

vercel bot commented Dec 13, 2024 •

edited

Loading

awelc Dec 16, 2024 •

edited

Loading

awelc Dec 16, 2024 •

edited

Loading