StyledText#setLineVerticalIndent: fix overlapping code minings in editor

[tobiasmelcher]

Ensure problem annotation code minings are rendered below source lines to prevent visual overlap during vertical scrolling.

Fixes: #2512

[mickaelistria]

IIRC, this block of code can be very expensive. I see it's now going to be executed more often than usual. Instead of fully removing the guard, isn't there a way to refine it?
However, I have not actually tried it on real big files. If you did so and are confident performance remains good, I'm fine with a merge, but code itself makes me unsure about it.

[tobiasmelcher]

I reduced it to "resetCache(lineIndex, 1);". Do you now have less concerns regarding performance?

[mickaelistria]

resetCache per se is the operation that triggers a lot of time recomputing sizes and so on. So yes, adding this can be a concern for performance.
However, as mentioned, if you've happily tested on a big enough file, I would trust my concerns are vain and would be fine seeing this patch merged.

[tobiasmelcher]

I created following performance test to compare the runtime of the setLineVerticalndent method.

@Test
public void testPerformanceSetLineVerticalIndent() throws Exception{
	Shell shell = new Shell();
	shell.setLayout(new FillLayout());
	StyledText text = new StyledText(shell, SWT.V_SCROLL | SWT.H_SCROLL);
	shell.setSize(500,500);
	shell.setVisible(true);
	StringBuilder str = new StringBuilder();
	for (int i=0;i<10_000;i++) {
		str.append("Line ").append(i).append("\n");
	}
	int lineHeight = text.getLineHeight();
	text.setText(str.toString());
	while(Display.getDefault().readAndDispatch()) {
	}

	long start = System.currentTimeMillis();
	for (int i=0;i<10_000;i++) {
		text.setLineVerticalIndent(i, lineHeight*2);
	}
	long diff = System.currentTimeMillis()-start;
	System.out.println(diff);
}

On my machine, I see following numbers.
3609ms in the old code base
17318ms with the changes of this pull request where resetCache is called much more often. Factor 4.8 slower is quite high. But I would say that this is not a real world scenario. setLineVerticalIndent will never be called for 10.000 lines.

I don't really understand what method resetCache is doing and why it is not working properly in the scrolling scenario mentioned in the issue (#2512). Can you maybe give some more insights and give some hints which parts of the code might make the problem? I have the feeling that StyledTextRenderer#calculateIdle is not thread-safe.

I found a more efficient solution which will not has any performance impact by increasing the initialBottomIndex variable by value 2 in setLineVerticalIndent. But don't ask me for the reason, I cannot explain. I only see that the else if (lineIndex > initialBottomIndex) { branch is not complete from my point of view and the lines are then not correctly rendered together with the code minings.

How should we proceed here?

[mickaelistria]

Factor 4.8 slower is quite high.

I guess this factor is the same even for smaller files, or is it quadratic?

I don't really understand what method resetCache

resetCache() resets some info for the given lines (width/height and maybe even the position). Whenever it's invoked, it will cause cairo to recompute the particular line position) before next drawing completion.
I think maybe there is a way to make things smarter by shifting some line positions without clearing the cache.
However, I haven't touched that code for a while so I'm not sure I remember things correctly enough...

I found a more efficient solution which will not has any performance impact by increasing the initialBottomIndex variable by value 2 in setLineVerticalIndent

That's interesting. Maybe caching a few more lines ahead of time and anticipating the scrolling is a way to prevent for recomputing positions too often...

[tobiasmelcher]

Ok, thanks a lot. Then I would propose to go for the +2 approach.

[github-actions]

Test Results

  118 files  ±0    118 suites  ±0   10m 50s ⏱️ -12s
4 431 tests ±0  4 410 ✅  - 4  17 💤 ±0  4 ❌ +4 
  298 runs  ±0    290 ✅  - 4   4 💤 ±0  4 ❌ +4 

For more details on these failures, see this check.

Results for commit 79283ab. ± Comparison against base commit 2edab10.

♻️ This comment has been updated with latest results.