Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

🐛 46x performance regression in LSP server #4776

Open
1 task done
strager opened this issue Aug 22, 2023 · 3 comments
Open
1 task done

🐛 46x performance regression in LSP server #4776

strager opened this issue Aug 22, 2023 · 3 comments
Labels
S-To triage Status: user report of a possible bug that needs to be triaged

Comments

@strager
Copy link
Contributor

strager commented Aug 22, 2023

Environment information

CLI:
  Version:              0.0.0
  Color support:        true

Platform:
  CPU Architecture:     x86_64
  OS:                   linux

Environment:
  ROME_LOG_DIR:         unset
  NO_COLOR:             unset
  TERM:                 "xterm-256color"

Rome Configuration:
  Status:               loaded
  Formatter disabled:   false
  Linter disabled:      false

Workspace:
  Open Documents:       0

Discovering running Rome servers...

Running Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ The client isn't connected to any server but rage discovered this running Rome server.

Server:
  Version:              0.0.0
  Name:                 rome_lsp
  CPU Architecture:     x86_64
  OS:                   linux

Workspace:
  Open Documents:       0

Other Active Server Workspaces:

Workspace:
  Open Documents:       1

Rome Server Log:

⚠ Please review the content of the log file before sharing it publicly as it may contain sensitive information:
  * Path names that may reveal your name, a project name, or the name of your employer.
  * Source code

[logs are too long for GitHub]

Incompatible Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ Rage discovered this running server using an incompatible version of Rome.

Server:
  Version:              <=10.0.0

Incompatible Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ Rage discovered this running server using an incompatible version of Rome.

Server:
  Version:              12.1.3

What happened?

I have a suite of JS LSP server benchmarks (https://quick-lint-js.com/benchmarks/). About a year ago, Rome performed well. Recent versions of Rome perform about 46x worse! Rome is 5x as slow as ESLint in my benchmarks.

I bisected the Rome repo. There appear to be at least three big regressions:

  • 9x: somewhere between commits 7d15325 (~2.22 ms) and 6345f97 (~19.82 ms)
  • 4x: commit 5809d01 (~19.66 ms -> ~84.35 ms)
  • 30%: somewhere between commits c3fb1a8 (~79.90 ms) and d31c9cf (~103.85 ms) (likely multiple smaller regressions)

For newer versions, my benchmarks stop the Rome server between samples. Each sample includes 1 warmup iteration and 10 timed iterations.

The benchmark basically lints this file 10 times: https://github.com/quick-lint/quick-lint-js/blob/1433b434f37c4dae212c35307a55849cfb633586/benchmark/benchmark-lsp/corpus/express-router.js

Benchmark framework: https://github.com/quick-lint/quick-lint-js/tree/1433b434f37c4dae212c35307a55849cfb633586/benchmark/benchmark-lsp

Here is performance data at various commits on my 5950X on Linux for quick-lint-js-benchmark-lsp-servers Rome/full-change-wait/express-router.js --iterations 10 --samples 3:

Rome 9be13ca -- published benchmarks
2.36 ms per iteration
2.24 ms per iteration
2.32 ms per iteration

Rome 8b80b13
2.19 ms per iteration
2.28 ms per iteration
2.18 ms per iteration

Rome 7d15325
2.22 ms per iteration
2.27 ms per iteration
2.24 ms per iteration

Rome c2927dc
started using socket LSP; hard to test

Rome b567557
hard to test

Rome 6345f97
added lsp-proxy command
19.82 ms per iteration
26.40 ms per iteration
23.58 ms per iteration

Rome 145c38b
20.62 ms per iteration
19.35 ms per iteration
21.32 ms per iteration

Rome 519df0b
19.47 ms per iteration
18.72 ms per iteration
18.94 ms per iteration

Rome 452aede
19.30 ms per iteration
19.23 ms per iteration
19.97 ms per iteration

Rome 5248e2b
21.68 ms per iteration
22.64 ms per iteration
19.66 ms per iteration

Rome 5809d01
big performance regression
84.35 ms per iteration
85.85 ms per iteration
87.99 ms per iteration

Rome 58a8e38
88.39 ms per iteration
89.44 ms per iteration
89.41 ms per iteration

Rome 8b6b6d0
87.18 ms per iteration
89.08 ms per iteration
87.97 ms per iteration

Rome a090d60
84.87 ms per iteration
82.63 ms per iteration
80.02 ms per iteration

Rome c3fb1a8
79.90 ms per iteration
82.71 ms per iteration
82.43 ms per iteration

Rome 6da1475
113.45 ms per iteration
116.18 ms per iteration
119.82 ms per iteration

Rome 6bd0a69
99.98 ms per iteration
102.63 ms per iteration
97.38 ms per iteration

Rome b77547d
97.02 ms per iteration
101.20 ms per iteration
98.93 ms per iteration

Rome d31c9cf
105.01 ms per iteration
105.91 ms per iteration
103.85 ms per iteration

Rome 817d5ac -- latest commit tested
104.27 ms per iteration
108.58 ms per iteration
109.32 ms per iteration

Expected result

Rome's LSP server is fast 🚀

Code of Conduct

  • I agree to follow Rome's Code of Conduct
@strager strager added the S-To triage Status: user report of a possible bug that needs to be triaged label Aug 22, 2023
@Conaclos
Copy link
Contributor

The regression impacts only the LSP?

@strager
Copy link
Contributor Author

strager commented Aug 22, 2023

The regression impacts only the LSP?

I don't know. I only measured LSP.

@denbezrukov
Copy link
Contributor

denbezrukov commented Aug 28, 2023

flamegraph1
I believe I've found the problem:

pub fn as_text_edits(&self) -> Option<(TextRange, TextEdit)> {
let mut range = None;
tracing::debug!(" changes {:?}", &self.changes);
for change in &self.changes {
let parent = change.parent.as_ref().unwrap_or(&self.root);
let delete = match parent.slots().nth(change.new_node_slot) {
Some(SyntaxSlot::Node(node)) => node.text_range(),
Some(SyntaxSlot::Token(token)) => token.text_range(),
_ => continue,
};
range = match range {
None => Some(delete),
Some(range) => Some(range.cover(delete)),
};
}
let text_range = range?;
let old = self.root.to_string();
let new = self.clone().commit().to_string();
let text_edit = TextEdit::from_unicode_words(&old, &new);
Some((text_range, text_edit))
}

TextEdit::from_unicode_words(&old, &new); is the bottleneck.
We can try reducing the range of the string for calculating the text diff, because currently, we use the entire strings both before and after mutation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S-To triage Status: user report of a possible bug that needs to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants