feat: Add public semantic token modifier for public items
#9031
Conversation
|
Can you also add the modifier and a short description to the list here? https://github.com/rust-analyzer/rust-analyzer/blob/5b4589f4747f242c038f19f5f2b51b2e03f93252/crates/ide/src/syntax_highlighting.rs#L122-L143 |
|
CI tests are complaining about non-linearity here(all three platforms) but I don't see where this change would be non-linear? 🤔 |
Done!
Any ideas how to fix this? Would it be worth rerunning to make sure it wasn’t a fluke? Then again, it isn’t good to have tests that fail on a whim. |
|
Looks linear enough to me. |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
Looks rather quadratic here:
diff --git a/crates/ide/src/syntax_highlighting/tests.rs b/crates/ide/src/syntax_highlighting/tests.rs
index bb68181fd..4edda0501 100644
--- a/crates/ide/src/syntax_highlighting/tests.rs
+++ b/crates/ide/src/syntax_highlighting/tests.rs
@@ -327,32 +327,33 @@ fn benchmark_syntax_highlighting_long_struct() {
#[test]
fn syntax_highlighting_not_quadratic() {
- if skip_slow_tests() {
- return;
- }
+ // if skip_slow_tests() {
+ // return;
+ // }
let mut al = AssertLinear::default();
- while al.next_round() {
- for i in 6..=10 {
- let n = 1 << i;
+ // while al.next_round() {
+ for i in 1..=45 {
+ let n = i * 100;
- let fixture = bench_fixture::big_struct_n(n);
- let (analysis, file_id) = fixture::file(&fixture);
+ let fixture = bench_fixture::big_struct_n(n);
+ let (analysis, file_id) = fixture::file(&fixture);
- let time = Instant::now();
+ let time = Instant::now();
- let hash = analysis
- .highlight(file_id)
- .unwrap()
- .iter()
- .filter(|it| it.highlight.tag == HlTag::Symbol(SymbolKind::Struct))
- .count();
- assert!(hash > n as usize);
+ let hash = analysis
+ .highlight(file_id)
+ .unwrap()
+ .iter()
+ .filter(|it| it.highlight.tag == HlTag::Symbol(SymbolKind::Struct))
+ .count();
+ assert!(hash > n as usize);
+ println!("P! {:>4?} {:?}", n, time.elapsed().as_millis());
- let elapsed = time.elapsed();
- al.sample(n as f64, elapsed.as_millis() as f64);
- }
+ let elapsed = time.elapsed();
+ // al.sample(n as f64, elapsed.as_millis() as f64);
}
+ // }
}
#[test]
|
|
Thanks for making the effort to create that graph! Would you have any idea what about this change makes highlighting quadratic? |
|
Huh, looks like we are also quadratic on master... |
|
Marking as broken window:
Not sure if I'll be able to look into this myself today though |
matklad
added
the
Broken Window
|
I suppose this one Caller hierarchy leads to |
This story begins in rust-lang#8384, where we added a smart test for our syntax highting, which run the algorithm on synthetic files of varying length in order to guesstimate if the complexity is O(N^2) or O(N)-ish. The test turned out to be pretty effective, and flagged rust-lang#9031 as a change that makes syntax highlighting accidentally quadratic. There was much rejoicing, for the time being. Then, lnicola asked an ominous question[1]: "Are we sure that the time is linear right now?" Of course it turned out that our sophisticated non-linearity detector *was* broken, and that our syntax highlighting *was* quadratic. Investigating that, many brave hearts dug deeper and deeper into the guts of rust-analyzer, only to get lost in a maze of traits delegating to traits delegating to macros. Eventually, matklad managed to peel off all layers of abstraction one by one, until almost nothing was left. In fact, the issue was discovered in the very foundation of the rust-analyzer -- in the syntax trees. Worse, it was not a new problem, but rather a well-know, well-understood and event (almost) well-fixed (!) performance bug. The problem lies within `SyntaxNodePtr` type -- a light-weight "address" of a node in a syntax tree [3]. Such pointers are used by rust-analyzer all other the place to record relationships between IR nodes and the original syntax. Internally, the pointer to a syntax node is represented by node's range. To "dereference" the pointer, you traverse the syntax tree from the root, looking for the node with the right range. The inner loop of this search is finding a node's child whose range contains the specified range. This inner loop was implemented by naive linear search over all the children. For wide trees, dereferencing a single `SyntaxNodePtr` was linear. The problem with wide trees though is that they contain a lot of nodes! And dereferencing pointers to all the nodes is quadratic in the size of the file! The solution to this problem is to speed up the children search -- rather than doing a linear lookup, we can use binary search to locate the child with the desired interval. Doing this optimization was one of the motivations (or rather, side effects) of rust-lang#6857. That's why `rowan` grew the useful `child_or_token_at_range` method which does exactly this binary search. But looks like we've never actually switch to this method? Oups. Lesson learned: do not leave broken windows in the fundamental infra. Otherwise, you'll have to repeatedly re-investigate the issue, by digging from the top of the Everest down to the foundation! [1]: https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/.60syntax_highlighting_not_quadratic.60.20failure/near/240811501 [2]: https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Syntax.20highlighting.20is.20quadratic [3]: https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Syntax.20highlighting.20is.20quadratic/near/243412392
lunacookies
force-pushed
the
pub-modifier
branch
from
e0804d4 to
922d76a
Compare
|
Done! |
|
No luck, it seems. |
|
Aha, it seems that this PR does introduce O(N^2) in the end. Running this in release: #[test]
fn benchmark_syntax_highlighting_long_struct() {
for i in 1..=20 {
let n = i * 500;
let fixture = bench_fixture::big_struct_n(n);
let (analysis, file_id) = fixture::file(&fixture);
let t = std::time::Instant::now();
analysis.highlight(file_id).unwrap();
eprintln!("{:3}: {:?}", i, t.elapsed());
}
}I get with master and with this PR. Let me take a closer look here.... |
|
Yeah, it's pertty clear why this is quadratic: impl ItemScope {
pub fn entries<'a>(&'a self) -> impl Iterator<Item = (&'a Name, PerNs)> + 'a { ... }
pub fn visibility_of(&self, def: ModuleDefId) -> Option<Visibility> {
self.name_of(ItemInNs::Types(def))
.or_else(|| self.name_of(ItemInNs::Values(def)))
.map(|(_, v)| v)
}
pub(crate) fn name_of(&self, item: ItemInNs) -> Option<(&Name, Visibility)> {
for (name, per_ns) in self.entries() {
if let Some(vis) = item.match_with(per_ns) {
return Some((name, vis));
}
}
None
} |
matklad
removed
the
Broken Window
lunacookies
force-pushed
the
pub-modifier
branch
from
922d76a to
caa82df
Compare
|
Thank you for the thorough analysis. Finding performance issues wasn’t my intention, but I guess it worked out :) |
lunacookies
force-pushed
the
pub-modifier
branch
from
caa82df to
3e7472f
Compare
|
bors try |
|
🔒 Permission denied Existing reviewers: click here to make arzg a reviewer |
|
odd, the workflow isnt starting for some reason |
|
bors r+ |
Veykril
changed the title
public semantic token modifier for public itemspublic semantic token modifier for public items
5 participants
Closes #8943.