Smoothed diagnostics refresh to not happen for every keystroke.

[orizi]

Now only starts after previous one was done.

---

This change is 

[piotmag769]

Checked it and it doesn't really feel smoother from user perspective imo

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @mkaput)

[orizi]

How large was the project you used it on?
And what was the effect you've actually seen?

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @mkaput)

[mkaput]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, and @orizi)

---

crates/cairo-lang-language-server/src/lib.rs line 336 at r1 (raw file):

        // Making sure only a single set refreshes diagnostics as the same time.
        // This way, following small updates to the db would not cause refreshes per sub-change.
        let _lock = self.refresh_lock.lock().await;

I'm afraid that this lock may end up being contested too heavily. Do you have a profile file generated before and after this change?

[mkaput]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)

---

crates/cairo-lang-language-server/src/lib.rs line 334 at r1 (raw file):

    #[tracing::instrument(level = "debug", skip_all)]
    async fn refresh_diagnostics(&self) -> LSPResult<()> {
        // Making sure only a single set refreshes diagnostics as the same time.

Please add a TODO comment on me to remove this lock when cancellation will be implemented (#6131)

[mkaput]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)

[orizi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lib.rs line 334 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Please add a TODO comment on me to remove this lock when cancellation will be implemented (#6131)

even if cancellation is implemented - i'm not sure it should be removed - still no reason for a 2 diag refreshes to be running at the same time.

---

crates/cairo-lang-language-server/src/lib.rs line 336 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I'm afraid that this lock may end up being contested too heavily. Do you have a profile file generated before and after this change?

i do not - i'm trying to find an actual example test project.
but do note this will have no actual effect currently - the db is locked all the time right after, so this basically does nothing.
we possibly may want to have only a single waiter at a point (as 2 waiters are useless, as the 2nd one would do exactly the same),.

[mkaput]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)

---

crates/cairo-lang-language-server/src/lib.rs line 334 at r1 (raw file):

Previously, orizi wrote…

even if cancellation is implemented - i'm not sure it should be removed - still no reason for a 2 diag refreshes to be running at the same time.

I think it can be removed because with cancellation we'll be able to very quickly detect "oh, I'm running out-of-date DB snapshot, so I have to abandon this run", so even if this will be called in parallel, each irrelevant parallel execution will promptly terminate.

Now, this lock will indeed serialize runs of refresh_diagnostics, but each run will still do a full pass over the entire codebase which will take time even if it hits cache, and will contend for further locks.

---

crates/cairo-lang-language-server/src/lib.rs line 336 at r1 (raw file):

Previously, orizi wrote…

i do not - i'm trying to find an actual example test project.
but do note this will have no actual effect currently - the db is locked all the time right after, so this basically does nothing.
we possibly may want to have only a single waiter at a point (as 2 waiters are useless, as the 2nd one would do exactly the same),.

1. Open Zeppelin Contracts are my benchmark for such things
2. Yeah, we should use database snapshots here

[orizi]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lib.rs line 334 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I think it can be removed because with cancellation we'll be able to very quickly detect "oh, I'm running out-of-date DB snapshot, so I have to abandon this run", so even if this will be called in parallel, each irrelevant parallel execution will promptly terminate.

Now, this lock will indeed serialize runs of refresh_diagnostics, but each run will still do a full pass over the entire codebase which will take time even if it hits cache, and will contend for further locks.

Done.

[orizi]

Looks much better now.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lib.rs line 336 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

1. Open Zeppelin Contracts are my benchmark for such things
2. Yeah, we should use database snapshots here

Used another project - but in any case - everything looks much better.
profile comparison as well.

[mkaput]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, and @orizi)

---

crates/cairo-lang-language-server/src/lib.rs line 343 at r3 (raw file):

        // together before querying the database, as well as releasing extra threads waiting to
        // start diagnostics updates. TODO(orizi): Consider removing when request
        // cancellation is supported.

Suggestion:

        // Making sure only a single thread is refreshing diagnostics at a time, and that at most
        // one thread is waiting to start refreshing. This allows changed to be grouped
        // together before querying the database, as well as releasing extra threads waiting to
        // start diagnostics updates.
        // TODO(orizi): Consider removing when request cancellation is supported.

[orizi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lib.rs line 343 at r3 (raw file):

        // together before querying the database, as well as releasing extra threads waiting to
        // start diagnostics updates. TODO(orizi): Consider removing when request
        // cancellation is supported.

Done.

[mkaput]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @Draggu)