improve performance of rangeForDefun (cause of repl slowness) #942

[brdloush]

What has Changed?

• rangeForDefun got rewritten so that it no longer needs to navigate from the very beginning of the document (which caused slowness when appending to repl window)
• added way more tests for rangeForDefun (as there weren't any :-))

Greatly improves #942. Before the fix, outputting become sluggish at 6k lines and was nearly unusable at just 10k (freezes for 30-60s). Now even 100k lines is bearable : execution of repl expression takes about 5 seconds.

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Added to or updated docs in this branch, if appropriate
• Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. NB: You need to sign in/up at Circle CI to find the Artifacts tab.
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • [ ] If I am fixing the issue, I have used GitHub's fixes/closes syntax
  • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• [ ] Created the issue I am fixing/addressing, if it was not present.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

• Made sure the PR is directed at the dev branch (unless reasons).
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
• Read the source changes.
• Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• If need be, had a chat within the team about particular changes.

Ping @PEZ, @bpringe

[brdloush]

I've rebased this PR to current dev, so that it VSIX for testing has those 2.0.205 changes included.

[PEZ]

Awesome. This performance bottleneck is causing Calva to feel quite broken when I run in to it.

Can you add some pointers to what people should test when they get hold of the VSIX to the PR description? From the top of my mind:

• The debugger (maybe @bpringe can tell us how to confirm that the change hasn't broken things here)
• Rainbow brackets
• Evaluating top level form
• Evaluating top level form to cursor
• Custom REPL command variables involving the top level form
• Evaluating at the REPL window prompt

I am probably missing things, am in a bit of a hurry...

[bpringe]

For the debugger, testing that breakpoints are found and that stepping works correctly should suffice.

[bpringe]

I've quickly tested all of the points above and the only thing that I see an issue with is when I add an extra/unbalanced parenthesis, it's not highlighted in red like it is currently in Calva.

Currently in Calva:

With the VSIX from this PR:

[brdloush]

Nicely spot @bpringe, I never noticed that these unbalanced parentheses gets highlighted in red, so I wasn't really missing this behavior.

@PEZ I've debugged current dev vs defun-range-experiments and found out that we slightly changed the behavior in new rangeForDefun. Old dev version returned at least [offset, offset] range "when nothing better was found". Our current version returns undefined in such case. This breaks mentioned ranbow brackets behavior in updateRainbowBrackets as this throws "NPE" (I'm not sure they call it like that in JS 😄) :

      endRange = endCursor.rangeForDefun(endOffset, false),
      rangeStart = startRange[0],
      rangeEnd = endRange[1];      // <-- NPE here as endRange is null

A simple fix might be adding two additional !== undefined checks when working with lastCandidateRange:

    rangeForDefun(offset: number, commentCreatesTopLevel = true): [number, number] {
        const cursor = this.doc.getTokenCursor(offset);
        let lastCandidateRange: [number, number] = cursor.rangeForCurrentForm(offset);
        while (cursor.forwardList() && cursor.upList()) {
            const commentCursor = cursor.clone();
            commentCursor.backwardDownList();
            if (!commentCreatesTopLevel || commentCursor.getToken().raw !== ')' || commentCursor.getFunctionName() !== 'comment') {
                const currentFormRange = cursor.rangeForCurrentForm(cursor.offsetStart);
                if (currentFormRange !== undefined) {
                    lastCandidateRange = currentFormRange;
                }
            }
        }
        return (lastCandidateRange !== undefined)
            ? lastCandidateRange
            : [offset, offset];
    }

This alone helps in situation where the unbalanced bracket is the last form in editor viewport that gets examined in updateRainbowBrackets. But if the are some more forms after the unbalanced parenthesis, highlighting still doesn't work correctly.

I suspect that it might somehow be related to yet another changed behavior:

(ns a
  (:require [clojure.string :as s]))

(defn foo []
  (let [bar 1]
    "bar")) )  ;; <- last paren on this line is unbalanced
   
(println (s/upper-case "test output"))
|

When I evaluate this in old dev version, it does nothing.. basically no form after the unbalanced paren can get evaluated. In new version, it evals the println form.

Unfortunately the whole logic in updateRainbowBrackets is way too heavy for my brain. Perhaps you might have some idea how to fix it (if it's really the root problem of this highlighting issue) 🤞

[PEZ]

Great find, @bpringe, and awesome investigation, @brdloush!

As for new evaluation behaviour with unbalanced parens, I don't worry about that. Unbalance is unbalance. 😀

I'm thinking that instead of fixing the call site with that highlight miss, is it hard to fix the defun method to keep the behaviour it used to have in respect to what is returned in this particular case? I'm far afk, so can't check myself right now.

[PEZ]

OK. So rereading @brdloush 's comment with a desktop browser, I see there is a suggestion to keep the old undefined behaviour.

Testing this I notice that clj-kondo has a check for unbalance that is better than Calva's. Hmmm...

[brdloush]

@PEZ I have one more observation. Consider this code

|(ns a
  (:require [clojure.string :as s])) ;; <-- forwardUpSexp normally jumps after this paren, but not with unbalanced paren..

(defn foo []
  (let [bar 1]
    "bar")) )  ;; <-- this last paren is ubalanced - forwardUpSexp actually ends up here  

:last-form 

If there's this unbalanced paren on "bar" line and I have a cursor at the very beginning (|), then even at dev version, doing a ctrl+alt+down (forwardUpSexp) actually makes the cursor jump just after the unbalanced closing paren, instead of jumping to closing paren of ns form.

I believe this is a problem for that updateRainbowBrackets code, as in mentioned case, startPaintingFrom is assigned an offset that's behind the actual problematic unbalanced paren. When I manually made a hack to that variable and reassigned it to 0 (which is what startPaintingFrom for that block of code is assigned in dev version), than the unbalanced paren gets the red background.

@PEZ what do you think about that forwardUpSexp behavior in such case? Is it broken even in current dev, or is such behavior a "non-defined feature"? Perhaps if we find a way to fix it, our new rangeForDefun will work fine, including the highlighting.

[PEZ]

@brdloush I think it is worth a try to fix the case you are pointing at. That said, it remains hard to define the ”correct” behaviour in the face of unbalance. Let's not do it in this PR.

Thinking about the highlight problem I started to think it makes sense that when we don't find a top level form, we return undefined. So I pushed a change where the highlighter call site deals with that instead of assuming a zero-width range. This should fix the issue that you found, @bpringe, can you test again with the same files?

[brdloush]

@PEZ your commit fixes this case

but as mentioned, not the one when there are more forms after the imbalance:

[PEZ]

@brdloush is the case that is not fixed caused by this PR or something we already have in the released version?

[brdloush]

@brdloush is the case that is not fixed caused by this PR or something we already have in the released version?

The highlighting itself works ok in released version.

As I wrote in my previous comment, I think that the fact that highlighting is broken in our new branch is related to the fact that we now rely on forwardUpSexp, which - even in released version - is "broken":

Even in released version, forwardUpSexp jumps to end of line 5, when you use ctrl+alt+down at the very beginning of the document: (offset 0). Ideally, the cursor should end up at offset 0, not jump to end of line 5. It stays at offset 0 if there's not that imbalanced paren on line 5.

[PEZ]

Ah, I'm starting to get it. =)

[PEZ]

Though I suspect it is forwardList that is the problem:

[PEZ]

Pushed a fix for forwardList now. In my testing it fixes the problem.

[brdloush]

👍 It's behaving very well now including the highlighting 👏 I'll install the VSIX later today or tomorrow and try to re-test everything more thoroughly.

@bpringe can you perhaps re-test your scenarios as well?

[bpringe]

I've been pretty busy but I'll try to test this in the next couple of days, if it's still needed.

[PEZ]

Thanks for helping with analysing this problem and persisting with analysing it down to the root causes, @brdloush! I think that we managed to navigate this such that the right fixes where applied and also the tests improved a lot. Let's deal with any regressions and room for improvements in future PRs.

[brdloush]

Awesome, looking forward to the next release 👏