Crate graph loading is slow and blocks the main loop

[matklad]

  350ms - GlobalState::handle_event
      350ms - GlobalState::handle_event/task
          350ms - GlobalState::switch_workspaces
              344ms - ProjectWorkspace::to_crate_graph
                  344ms - cargo_to_crate_graph
                        0ms - sysroot_to_crate_graph (1 calls)
                      344ms - ???
                0ms - GlobalState::process_changes (1 calls)

when opening https://github.com/near/nearcore

[stanciuadrian]

this one doesn't even load on Windows:

cargo check fails with

Caused by:
  failed to stat 'C:/Users/X/.cargo/git/checkouts/openethereum-91fec6732bb1206a/837f1e3/ethcore/res/ethereum/tests/LegacyTests/Constantinople/BlockchainTests/GeneralStateTests/stMemExpandingEIP150Calls/CallAndCallcodeConsumeMoreGasThenTransactionHasWithMemExpandingCalls_d0g0v0.json': The filename or extension is too long.
  ; class=Os (2); code=Exists (-4)

[lnicola]

After #8627:

  198ms - GlobalState::handle_event
      198ms - GlobalState::handle_event/task
          198ms - GlobalState::switch_workspaces
              179ms - ProjectWorkspace::to_crate_graph
                  179ms - cargo_to_crate_graph
                      165ms - GlobalState::load (2297 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
                       13ms - ???

  124ms - GlobalState::handle_event
      124ms - GlobalState::handle_event/task
          116ms - GlobalState::switch_workspaces
               92ms - ProjectWorkspace::to_crate_graph
                   92ms - cargo_to_crate_graph
                       17ms - GlobalState::load
                       36ms - GlobalState::load (2296 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
                       38ms - ???

  114ms - GlobalState::handle_event
      113ms - GlobalState::handle_event/task
           95ms - GlobalState::switch_workspaces
               73ms - ProjectWorkspace::to_crate_graph
                   73ms - cargo_to_crate_graph
                       33ms - GlobalState::load (2297 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
                       39ms - ???
                0ms - GlobalState::process_changes (1 calls)
                0ms - GlobalState::reload_flycheck (1 calls)
                3ms - RootDatabase::apply_change (1 calls)
               18ms - ???
           17ms - ???

[matklad]

Interesting! The fact that we use load in that function is a long-standing technical debt. This really should be done way more elegantly. Seems like a reason to finally clean that up!

…

On Thursday, 22 April 2021, Laurențiu Nicola ***@***.***> wrote: After #8627 <#8627>: 198ms - GlobalState::handle_event 198ms - GlobalState::handle_event/task 198ms - GlobalState::switch_workspaces 179ms - ProjectWorkspace::to_crate_graph 179ms - cargo_to_crate_graph 165ms - GlobalState::load (2297 calls) 0ms - sysroot_to_crate_graph (1 calls) 13ms - ??? 124ms - GlobalState::handle_event 124ms - GlobalState::handle_event/task 116ms - GlobalState::switch_workspaces 92ms - ProjectWorkspace::to_crate_graph 92ms - cargo_to_crate_graph 17ms - GlobalState::load 36ms - GlobalState::load (2296 calls) 0ms - sysroot_to_crate_graph (1 calls) 38ms - ??? 114ms - GlobalState::handle_event 113ms - GlobalState::handle_event/task 95ms - GlobalState::switch_workspaces 73ms - ProjectWorkspace::to_crate_graph 73ms - cargo_to_crate_graph 33ms - GlobalState::load (2297 calls) 0ms - sysroot_to_crate_graph (1 calls) 39ms - ??? 0ms - GlobalState::process_changes (1 calls) 0ms - GlobalState::reload_flycheck (1 calls) 3ms - RootDatabase::apply_change (1 calls) 18ms - ??? 17ms - ??? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3MZJ2L3TWBGUUFJ7CU3TKBTNZANCNFSM43MVY7KA> .

[lnicola]

Looks like the rest of the time is spent in add_dep (which does a DFS).

[lnicola]

   60ms - GlobalState::handle_event
       60ms - GlobalState::handle_event/task
           60ms - GlobalState::switch_workspaces
               41ms - ProjectWorkspace::to_crate_graph
                   41ms - cargo_to_crate_graph
                       22ms - GlobalState::load (2297 calls)
                       11ms - add_dep (19469 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
               15ms - GlobalState::process_changes
                    0ms - RootDatabase::apply_change (1 calls)
                    8ms - SourceRootConfig::partition (1 calls)
                0ms - GlobalState::reload_flycheck (1 calls)
                0ms - RootDatabase::apply_change (1 calls)
        0ms - GlobalState::process_changes (1 calls)

  240ms - GlobalState::handle_event
      240ms - GlobalState::handle_event/task
           33ms - GlobalState::switch_workspaces
               28ms - ProjectWorkspace::to_crate_graph
                   28ms - cargo_to_crate_graph
                       13ms - GlobalState::load (2297 calls)
                       10ms - add_dep (19469 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
                0ms - GlobalState::process_changes (1 calls)
                0ms - GlobalState::reload_flycheck (1 calls)
                1ms - RootDatabase::apply_change (1 calls)
          205ms - GlobalState::switch_workspaces
              198ms - ProjectWorkspace::to_crate_graph
                  198ms - cargo_to_crate_graph
                       14ms - GlobalState::load (2297 calls)
                      160ms - ProcMacroClient::by_dylib_path (178 calls)
                       14ms - add_dep (19469 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
                0ms - GlobalState::process_changes (1 calls)
                0ms - GlobalState::reload_flycheck (1 calls)
                0ms - RootDatabase::apply_change (1 calls)
        0ms - GlobalState::process_changes (1 calls)

   61ms - GlobalState::handle_event
       61ms - GlobalState::handle_event/task
           55ms - GlobalState::switch_workspaces
               46ms - ProjectWorkspace::to_crate_graph
                   46ms - cargo_to_crate_graph
                       16ms - GlobalState::load (2297 calls)
                        9ms - ProcMacroClient::by_dylib_path (178 calls)
                       10ms - add_dep (19469 calls)
                        0ms - sysroot_to_crate_graph (1 calls)
                0ms - GlobalState::process_changes (1 calls)
                0ms - GlobalState::reload_flycheck (1 calls)
                3ms - RootDatabase::apply_change (1 calls)
            3ms - GlobalState::switch_workspaces (1 calls)
        0ms - GlobalState::process_changes (1 calls)

[matklad]

Removing the broken window label. I think this doesn't affect UX negatively, and, with profile spans added, its clear whats going on. It's slow for two reasons:

• we load proc macros synchronously
• we load crate root files synchronously

#8646 tales the first issue.

The second one is interesting. The core issue is that we have an invariant that FileIds point to existing files. We also want to store FileId in the CrateGraph, because we don't want salsa input to know about file paths.

For this reason, at the moment we read the text of each lib.rs, during to_crate_graph, although the only thing we really need is to convert from path to FileId. There's a pretty simple work-around here -- instead of loading the true file, set it's contents to "" if it isn't already loaded. We'll overwrite it with real contents afterward anyway.

The proper solution would be to have a PerCrateGraph with paths, listen to events from VFS and convert it to create graph only when all crate root files are loaded.

While looking at it, I also realized that the current scheme is actually outright buggy! If we create a crate graph and then on of the crate roots get deleted, we don't re-build the crate graph. So it stores a FileId of a non-existing file, which might lead to a panic when we try to get this file's text out of VFS.

[matklad]

Marking as unactionable, the fix should fix the bug mentioned above, its not super clear how to actually do that.