internal: Support `Pointee` trait #14693

HKalbasi · 2023-04-30T19:02:42Z

fix #13992

HKalbasi · 2023-04-30T19:06:23Z

This PR upgrades chalk which uses syn2. Is it a problem?

Veykril · 2023-04-30T19:18:12Z

I'd like to wait until we can move all our dependencies over to versions using syn 2 so we don't build syn 1 and syn 2

lnicola · 2023-05-01T10:42:51Z

That mean we won't be able to get any juicy chalk fixes until we switch, not that it's really active these days.

I have a draft PR for salsa at salsa-rs/salsa#442, but it still needs some work.

Veykril · 2023-05-01T11:07:16Z

fwiw, we can't really use upstream salsa as is. There are some commits on master that the latest release doesn't have. And those commits cause a considerable slow down in rust-analyzer.

So we'd need to branch a release off with those commits omitted (and ideally with the commit that updates parking_lot included since thats currently duped for us due to salsa as well)

HKalbasi · 2023-05-01T12:12:40Z

Now that we don't publish crates, isn't possible to use git dependency to our forks? Either for salsa to upgrade syn, or for chalk to downgrade syn.

lnicola · 2023-05-01T12:57:58Z

I see, you mean that slot change. I tried to run the completion benchmark again and got:

# master
running 1 test
workspace loading: 9.07s
initial: 1.24s
change: 48.98µs
cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable.
   91ms - unqualified path completion
       21ms - CompletionContext::new
           10ms - parse_query @ FileId(45)
            3ms - CompletionContext::analyze (1 calls)
            5ms - CompletionContext::expand (1 calls)
            0   - Semantics::analyze_impl (1 calls)
            0   - crate_def_map:wait (1 calls)
       38ms - import_on_the_fly @ sel
           22ms - import_assets::search_for_imports
               22ms - import_assets::search_for
                   22ms - import_assets::path_applicable_imports
                       21ms - items_with_name @ Name: sel, crate: Exclude, assoc items: Some("hir"), limit: Some(40)
                           21ms - find_items
                               21ms - crate_symbols
                                    0   - crate_def_map:wait (1 calls)
                                   21ms - module_symbols (11 calls)
                                0   - query_external_importables (1 calls)
                                0   - symbol_index::Query::search (1 calls)
                        0   - find_path_prefixed (45 calls)
                        0   - get_name_definition (40 calls)
                    0   - Semantics::analyze_impl (1 calls)
                    0   - import_assets::scope_definitions (1 calls)
            0   - attrs_query (7 calls)
            0   - crate_def_map:wait (3 calls)
            0   - item::Builder::build (35 calls)
           15ms - render_resolution (35 calls)
       13ms - complete_expr_path
           13ms - CompletionContext::process_all_names
                0   - attrs_query (27 calls)
                0   - crate_def_map:wait (344 calls)
                0   - item::Builder::build (303 calls)
               12ms - render_resolution (303 calls)
            0   - item::Builder::build (13 calls)
       18ms - iterate_method_candidates
            0   - PerNs::filter_visibility (24 calls)
            0   - crate_def_map:wait (384 calls)
            0   - deref_by_trait (7 calls)
            0   - generic_params_query (209 calls)
            0   - item::Builder::build (134 calls)
           13ms - render_method (134 calls)
            0   - resolve_obligations_as_possible (17 calls)
            0   - trait_solve::wait (37 calls)
        0   - crate_def_map:wait (2 calls)
        0   - deref_by_trait (1 calls)
        0   - item::Builder::build (17 calls)
        0   - resolve_obligations_as_possible (2 calls)
change: 62.59µs
cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable.
   37ms - dot completion
       23ms - CompletionContext::new
           11ms - parse_query @ FileId(45)
            5ms - CompletionContext::analyze (1 calls)
            5ms - CompletionContext::expand (1 calls)
            0   - Semantics::analyze_impl (1 calls)
            0   - crate_def_map:wait (1 calls)
        9ms - import_on_the_fly_method @ 
            9ms - import_assets::search_for_imports
                9ms - import_assets::search_for
                    9ms - import_assets::trait_applicable_items
                        0   - applicable_inherent_traits (1 calls)
                        0   - env_traits (1 calls)
                        3ms - get_name_definition (40 calls)
                        3ms - items_with_name (1 calls)
                        3ms - iterate_method_candidates (1 calls)
                    0   - Semantics::analyze_impl (1 calls)
                    0   - import_assets::scope_definitions (1 calls)
        0   - crate_def_map:wait (2 calls)
        0   - deref_by_trait (1 calls)
        0   - item::Builder::build (11 calls)
        4ms - iterate_method_candidates (1 calls)
        0   - lang_item_query (2 calls)
        0   - resolve_obligations_as_possible (2 calls)
test integrated_benchmarks::integrated_completion_benchmark ... ok

# -git salsa
running 1 test
workspace loading: 7.44s
initial: 1.31s
change: 51.48µs
cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable.
   95ms - unqualified path completion
       23ms - CompletionContext::new
           10ms - parse_query @ FileId(49)
            5ms - CompletionContext::analyze (1 calls)
            5ms - CompletionContext::expand (1 calls)
            0   - Semantics::analyze_impl (1 calls)
            0   - crate_def_map:wait (1 calls)
       41ms - import_on_the_fly @ sel
           22ms - import_assets::search_for_imports
               22ms - import_assets::search_for
                   22ms - import_assets::path_applicable_imports
                       22ms - items_with_name @ Name: sel, crate: Exclude, assoc items: Some("hir"), limit: Some(40)
                           22ms - find_items
                               21ms - crate_symbols
                                    0   - crate_def_map:wait (1 calls)
                                   21ms - module_symbols (11 calls)
                                0   - query_external_importables (1 calls)
                                0   - symbol_index::Query::search (1 calls)
                        0   - find_path_prefixed (45 calls)
                        0   - get_name_definition (40 calls)
                    0   - Semantics::analyze_impl (1 calls)
                    0   - import_assets::scope_definitions (1 calls)
            6ms - render_resolution
                6ms - render_fn
                    1ms - body_with_source_map_query (1 calls)
                    0   - crate_def_map:wait (5 calls)
                    0   - generic_params_query (1 calls)
                    4ms - infer:wait (1 calls)
                    0   - lang_item_query (1 calls)
            0   - attrs_query (7 calls)
            0   - crate_def_map:wait (3 calls)
            0   - item::Builder::build (35 calls)
           11ms - render_resolution (34 calls)
       12ms - complete_expr_path
           12ms - CompletionContext::process_all_names
                0   - attrs_query (27 calls)
                0   - crate_def_map:wait (344 calls)
                0   - item::Builder::build (303 calls)
               11ms - render_resolution (303 calls)
            0   - item::Builder::build (13 calls)
       18ms - iterate_method_candidates
            0   - PerNs::filter_visibility (24 calls)
            0   - crate_def_map:wait (384 calls)
            0   - deref_by_trait (7 calls)
            0   - generic_params_query (210 calls)
            0   - item::Builder::build (134 calls)
           12ms - render_method (134 calls)
            0   - resolve_obligations_as_possible (17 calls)
            0   - trait_solve::wait (37 calls)
            5ms - ???
        0   - crate_def_map:wait (2 calls)
        0   - deref_by_trait (1 calls)
        0   - item::Builder::build (17 calls)
        0   - resolve_obligations_as_possible (2 calls)
change: 47.79µs
cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable.
   39ms - dot completion
       27ms - CompletionContext::new
           10ms - parse_query @ FileId(49)
            9ms - CompletionContext::analyze
                9ms - Semantics::analyze_impl
                    9ms - infer:wait @ name
                        9ms - infer_query
                            0   - crate_def_map:wait (9 calls)
                            0   - deref_by_trait (13 calls)
                            3ms - resolve_obligations_as_possible (74 calls)
                            0   - trait_solve::wait (4 calls)
                            5ms - ???
                    0   - SourceBinder::to_module_def (1 calls)
                    0   - body_with_source_map_query (1 calls)
                    0   - crate_def_map:wait (1 calls)
            5ms - CompletionContext::expand (1 calls)
            0   - Semantics::analyze_impl (1 calls)
            0   - crate_def_map:wait (1 calls)
        7ms - import_on_the_fly_method @ 
            7ms - import_assets::search_for_imports
                7ms - import_assets::search_for
                    7ms - import_assets::trait_applicable_items
                        0   - applicable_inherent_traits (1 calls)
                        0   - env_traits (1 calls)
                        0   - get_name_definition (40 calls)
                        3ms - items_with_name (1 calls)
                        3ms - iterate_method_candidates (1 calls)
                    0   - Semantics::analyze_impl (1 calls)
                    0   - import_assets::scope_definitions (1 calls)
        0   - crate_def_map:wait (2 calls)
        0   - deref_by_trait (1 calls)
        0   - item::Builder::build (11 calls)
        4ms - iterate_method_candidates (1 calls)
        0   - lang_item_query (2 calls)
        0   - resolve_obligations_as_possible (2 calls)

So it's still slightly slower, but not that bad.

Veykril · 2023-05-01T13:02:31Z

Huh, it was slower by a lot when I tested this back then, but I guess we can try and see.

lnicola · 2023-05-01T13:10:02Z

Yeah, but then Niko made some changes and brought it within 10% of the original, according to that Zulip topic.

bors · 2023-05-18T15:31:21Z

☔ The latest upstream changes (presumably #14843) made this pull request unmergeable. Please resolve the merge conflicts.

HKalbasi · 2023-06-16T13:40:34Z

I tried to see the compile time effect of this branch using cargo build --timings and it seems the syn2 is not on the critical path. The current critical path is quote > syn1 > salsa-macros > salsa > base-db > hir-expand > hir-def > hir-ty and the maximum path including syn2 is quote > syn2 > chalk-derive > chalk-ir > chalk-solve > chalk-recursive > hir-ty which has ~4 seconds slack time. And the reported time by cargo on my machine is 1m 15s this branch vs 1m 13s on master. The situation is probably worse in CI which has less cores than my machine, but it shouldn't be that bad as the syn takes 12s to compile so I guess a ~6 delay? Let's see.

Is there any other problem in mixing syns other than compile time?

HKalbasi · 2023-06-16T14:11:55Z

The compile test time on ubuntu in CI is 10m 4s on this branch. On #15003 it is 11m 31, and on #14990 it is 9m 44s so I guess it is too unstable to say anything based on that.

lnicola · 2023-06-16T14:14:53Z

I hope we could eat the syn 2 slowdown for now, because there's some great goodies coming up in chalk.

Veykril · 2023-06-16T14:16:38Z

I agree, let's just upgrade chalk given all the recent changes that landed there. I didn't realize that syn and syn2 will probably build in parallel, since almost everything is blocked on syn building, so the slow down won't be too big after all.

HKalbasi · 2023-06-16T14:34:03Z

@bors r+

bors · 2023-06-16T14:34:05Z

📌 Commit 527dfed has been approved by HKalbasi

It is now in the queue for this repository.

bors · 2023-06-16T14:34:14Z

⌛ Testing commit 527dfed with merge 0cad484...

bors · 2023-06-16T14:51:07Z

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 0cad484 to master...

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2023

HKalbasi force-pushed the pointee-trait branch 2 times, most recently from d533079 to f34892e Compare April 30, 2023 20:34

lnicola mentioned this pull request May 1, 2023

internal: Bump salsa #14700

Closed

HKalbasi added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2023

Support Pointee trait

527dfed

HKalbasi force-pushed the pointee-trait branch from f34892e to 527dfed Compare June 16, 2023 13:41

bors merged commit 0cad484 into rust-lang:master Jun 16, 2023
10 checks passed

lnicola changed the title ~~Support Pointee trait~~ internal: Support Pointee trait Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Support `Pointee` trait #14693

internal: Support `Pointee` trait #14693

HKalbasi commented Apr 30, 2023

HKalbasi commented Apr 30, 2023

Veykril commented Apr 30, 2023

lnicola commented May 1, 2023

Veykril commented May 1, 2023 •

edited

HKalbasi commented May 1, 2023

lnicola commented May 1, 2023

Veykril commented May 1, 2023 •

edited

lnicola commented May 1, 2023 •

edited

bors commented May 18, 2023

HKalbasi commented Jun 16, 2023

HKalbasi commented Jun 16, 2023

lnicola commented Jun 16, 2023

Veykril commented Jun 16, 2023 •

edited

HKalbasi commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

internal: Support Pointee trait #14693

internal: Support Pointee trait #14693

Conversation

HKalbasi commented Apr 30, 2023

HKalbasi commented Apr 30, 2023

Veykril commented Apr 30, 2023

lnicola commented May 1, 2023

Veykril commented May 1, 2023 • edited

HKalbasi commented May 1, 2023

lnicola commented May 1, 2023

Veykril commented May 1, 2023 • edited

lnicola commented May 1, 2023 • edited

bors commented May 18, 2023

HKalbasi commented Jun 16, 2023

HKalbasi commented Jun 16, 2023

lnicola commented Jun 16, 2023

Veykril commented Jun 16, 2023 • edited

HKalbasi commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

bors commented Jun 16, 2023

internal: Support `Pointee` trait #14693

internal: Support `Pointee` trait #14693

Veykril commented May 1, 2023 •

edited

Veykril commented May 1, 2023 •

edited

lnicola commented May 1, 2023 •

edited

Veykril commented Jun 16, 2023 •

edited