internal: Don't store call-site text offsets in hygiene info #8746
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
Contributor
|
After this PR, we could remove |
Contributor
|
Threading db everywhere always good. LGTM, but maybe make db a public filed of LowerCtx and don’t pass the two around together? |
Contributor
|
r=me |
Contributor
Author
|
bors r=matklad |
Contributor
Contributor
|
Seems like we still recompute item trees for macros for maybe some other reason? λ env RUN_SLOW_BENCHES=1 cargo test --release --package rust-analyzer --lib -- integrated_benchmarks::integrated_completion_benchmark --exact --nocapture
Finished release [optimized] target(s) in 0.04s
Running unittests (target/release/deps/rust_analyzer-e3854b1ff4c04920)
running 1 test
workspace loading: 322.96ms
initial: 6.66s
change: 30.74µs
cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable.
22ms - SourceBinder::to_module_def
22ms - crate_def_map:wait
15ms - item_tree_query (195 calls)
0ms - parse_macro_expansion (2 calls)
7ms - ???
17ms - descend_into_macros
17ms - Semantics::analyze_impl
13ms - infer:wait @ name
13ms - infer_query
13ms - deref
13ms - deref_by_trait
13ms - trait_solve::wait
4ms - impl_data_query (260 calls)
8ms - ???
0ms - crate_def_map:wait (36 calls)
0ms - deref (5 calls)
0ms - resolve_obligations_as_possible (3 calls)
0ms - trait_solve::wait (5 calls)
0ms - SourceBinder::to_module_def (1 calls)
0ms - body_with_source_map_query (1 calls)
0ms - crate_def_map:wait (2 calls)
0ms - impl_data_query (1 calls)
3ms - parse_macro_expansion (151 calls)
33ms - import_on_the_fly @ sel
23ms - import_assets::search_for_imports
23ms - import_assets::search_for
23ms - import_assets::path_applicable_imports
19ms - items_with_name @ Name: sel, crate: Exclude, assoc items: Some("hir"), limit: Some(40)
19ms - find_items
15ms - search_dependencies @ Query { query: "sel", lowercased: "sel", name_only: true, assoc_items_only: false, search_mode: Fuzzy, case_sensitive: false, limit: 40, exclude_import_kinds: {AssociatedItem} }
14ms - import_map_query (18 calls)
0ms - crate_def_map:wait (1 calls)
2ms - find_path_prefixed (43 calls)
1ms - get_name_definition (40 calls)
0ms - import_assets::scope_definitions (1 calls)
0ms - crate_def_map:wait (1 calls)
9ms - render_resolution (39 calls)
unqualified path completion: 121.02ms
change: 41.34µs
cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable.
20ms - SourceBinder::to_module_def
20ms - crate_def_map:wait
11ms - item_tree_query (152 calls)
1ms - parse_macro_expansion (45 calls)
7ms - ???
16ms - descend_into_macros
16ms - Semantics::analyze_impl
12ms - infer:wait @ name
12ms - infer_query
12ms - deref
12ms - deref_by_trait
12ms - trait_solve::wait
4ms - impl_data_query (259 calls)
8ms - ???
0ms - crate_def_map:wait (35 calls)
0ms - deref (6 calls)
0ms - resolve_obligations_as_possible (3 calls)
0ms - trait_solve::wait (5 calls)
0ms - SourceBinder::to_module_def (1 calls)
0ms - body_with_source_map_query (1 calls)
0ms - crate_def_map:wait (2 calls)
0ms - impl_data_query (1 calls)
2ms - parse_macro_expansion (130 calls)
7ms - import_on_the_fly @
7ms - import_assets::search_for_imports
7ms - import_assets::search_for
7ms - import_assets::trait_applicable_items
0ms - crate_def_map:wait (105 calls)
0ms - deref (2 calls)
0ms - find_path_prefixed (6 calls)
0ms - generic_params_query (69 calls)
1ms - get_name_definition (40 calls)
4ms - items_with_name (1 calls)
0ms - trait_solve::wait (51 calls)
0ms - import_assets::scope_definitions (1 calls)
0ms - Semantics::analyze_impl (1 calls)
dot completion: 65.09ms
test integrated_benchmarks::integrated_completion_benchmark ... ok |
Veykril
changed the title
bors bot
added a commit
that referenced
this pull request
May 9, 2021
8776: fix: fix unnecessary recomputations due to macros r=jonas-schievink a=jonas-schievink This computes a macro's fragment kind eagerly (when the calling file is still available in parsed form) and stores it in the `MacroCallLoc`. This means that during expansion we no longer have to reparse the file containing the macro call, avoiding the unnecessary salsa dependencies (#8746 (comment)). Marking as draft until I manage to find a test for this problem, since for some reason `typing_inside_a_function_should_not_invalidate_expansions` does not catch this (which might indicate that I misunderstand the problem). I've manually confirmed that this fixes the issue described in #8746 (comment): ``` 7ms - parse_query @ FileId(179) 12ms - SourceBinder::to_module_def 12ms - crate_def_map:wait 5ms - item_tree_query (1 calls) 7ms - ??? ``` Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This threads a lot more database references around in order to avoid storing a bare
TextOffsetin the hygiene info. ThisTextOffsetmade hygiene info andItemTrees more volatile than they should be, leading to excessive recomputation ofItemTrees.The incremental test added in #8721 is now passing with these changes.
closes #8721