Optimize rendering by bulk-adding text highlights and reducing unnecessary work

[chylex]

It's not trivial to test how much faster this is, so I recorded a video and counted frames between the loudest point of the key press waveform and the when search highlights (not markers) started appearing on the screen. Excluding the first key press because it takes a bit longer, the averages are:

Before: 0.424s
After:  0.050s

One thing I did was

        // TODO this is a lag-fest, results should be cached before this gets turned back on
        //if (highlighter.startOffset == Selector.nearestVisibleMatches().firstOrNull()) {
        //  color = naturalCaretColor
        //  drawLine(start.x, startY, start.x, startY + rectHeight)
        //}

As far as I can tell this only adds a small caret highlight, but repeatedly calling Selector.nearestVisibleMatches() made for a sizeable chunk in the profiler, so I commented it out for now. If you'd rather have the results cached somewhere, let me know how you'd want to implement it, there appears to be a lot of global state scattered around so I'm not sure.

[chylex]

I'm trying out using bulk editor changes for removing highlighters too, if it works I'll add one more commit.

[breandan]

there appears to be a lot of global state scattered around

There is indeed a bunch of global state which needs to be removed. In specific scenarios, this is known to cause race conditions (cf. #187, #132, #142, #147, #338), which I have tried (unsuccessfully) to debug on several occasions, as you might have noticed with the synchronized, volatile, runNow, runLater blocks everywhere. The whole approach needs to be reconsidered. If you think of a solution, please take whatever measures you feel are necessary to address (ruthless refactoring welcome). We could probably also use some asynchronous tests (cf. #139).

[chylex]

If you think of a solution, please take whatever measures you feel are necessary to address (ruthless refactoring welcome).

I was thinking about that. Would you be okay with bumping the line width limit from 80 to 140? If I were to rewrite a bunch of things I find 80 is too restrictive and leads to high vertical density, because if lines can be longer it's easier to vertically space them out.

[breandan]

The default line width is set here, although it was never strictly enforced:

AceJump/.editorconfig

Line 7 in b1d69bf

max_line_length=80

In order to preserve the Git blame history, I would just change the locations you feel are necessary to semantically modify.

[chylex]

It wouldn't be a good idea to pull in the refactor as-is because I basically moved everything (which obviously breaks external usages) and haven't re-implemented all the features and subtleties of how AceJump works to work without all the global state.

I think the best option after I finish the rewrite would be to write down a list of all the bugfixes and tweaks I made, let you pick what you want again, and port those to the current codebase just so the bugs and annoyances are addressed first. I'm not sure if the refactor will be in a pullable state, but at minimum I can think of some things that could be integrated pretty easily:

• I wrote a new search boundary system that correctly works with long lines (but is a bit slower)
• I moved all editor actions into plugin.xml
• I changed all editor.document.text into editor.document.immutableCharSequence to avoid copying the contents

I also very recently realized (related to this PR) that switching the bulk editing mode takes some time, so I ended up only turning on isInBulkUpdate for >1000 highlighter changes which seems close to a sweet spot for maximum performance in all cases.

Anyway, my work-in-progress refactor branch is at https://github.com/chylex/AceJump/tree/humongous-refactor if you want to take an early look and perhaps integrate some things immediately, but you would have to be careful to only take parts that haven't moved/removed old behaviors, which might be a challenge. The only significant global state is now in SessionManager, and everything runs in the AWT thread directly from user interactions so runNow or runLater were removed, both of which are major parts of the refactoring so anything that used the old Model or the two threading functions probably can't easily be ported over on its own.

[breandan]

Looks great! The only things we need to be careful about modifying are the methods annotated with @ExternalUsage. Otherwise the class and method structure are open to reinterpretation. Feel free to restructure if you find a better cohesion/coupling tradeoff.

One idea I've meant to implement for a while is a trie-based index. Not sure if this is what you had in mind with the EditorOffsetCache, but basically you want to index every bi/tri/.../ngram and their locations in the editor ahead of time. Instead of searching through the editor each time AceJump is triggered, scanning and tag assignment would just consist of doing a lookup.

This would be extremely fast and solve our latency problems once and for all, however it would need to listen for editor changes in realtime and be updated incrementally each time the editor text was modified. Keeping the index in sync with the editor would require some ingenuity, but would allow instantaneous tagging and meet the constraints of the the tag assignment problem.

The only significant global state is now in SessionManager...

Great! This looks like just what we needed.

old behaviors

Better to remove non-essential behaviors and focus on the default case. The regex-based search and tag placement rules, for example are notoriously buggy. I would just tear out everything you think is not essential and focus on code quality. We can figure out how to reimplement them later once we have a stateless implementation.

[chylex]

Decided to open #348 to continue the discussion, so it doesn't get easily lost in an unrelated PR.