Bug 531639

Summary:	[StyledText] Improve performance on load when setLineSpacingProvider is called
Product:	[Eclipse Project] Platform	Reporter:	Angelo ZERR <azerr>
Component:	SWT	Assignee:	Angelo ZERR <azerr>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	mistria
Version:	4.8	Keywords:	performance
Target Milestone:	4.8 M6
Hardware:	All
OS:	All
See Also:	https://git.eclipse.org/r/118100 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a8dcd5187f8a0d9d39ee455a2e49300cfa0b5566
Whiteboard:

Description Angelo ZERR

2018-02-25 00:19:50 EST

When StyledText#setLineSpacingProvider  is called, it call 2 code line:

---------------------------------------------
setVariableLineHeight();
resetCache(0, content.getLineCount());
---------------------------------------------

Those 2 calls can be omitted for several reasons:

 * performance: when setLineSpacingProvider   is called, it resets the whole line height/width of the StyledText which are already computed. This code is interesting if there are some line spacing of some lines, but this resetCache is already done in StyledTextRenderer#getTextLayout when a line spacing changed.

 * today, when there are none custom line spacing returns by the StyledTextLineSpacingProvider, the StyledText switch to "variable line height", although it should not switch. The case of "not switch" is interesting for display only LineContentCodeMining (ex : JDT Java method parameter mining). In this config, the only display of LineContentCodeMining will be fast. Please note that setVariableLineHeight is called in getTextLayout too, so it is not required to call setVariableLineHeight in StyledText#setLineSpacingProvider.

Comment 1 Eclipse Genie

2018-02-25 00:22:05 EST

New Gerrit change created: https://git.eclipse.org/r/118100

Comment 2 Mickael Istria

2018-03-01 06:27:30 EST

(In reply to Angelo ZERR from comment #0)
> when setLineSpacingProvider  is called, it resets the whole
> line height/width of the StyledText which are already computed. This code is
> interesting if there are some line spacing of some lines, but this
> resetCache is already done in StyledTextRenderer#getTextLayout when a line
> spacing changed.

The resetCache AND the setVariableLineHeight() - the 2nd is important too and you only mention it later in your comment so I thought it would miss.

>  * today, when there are none custom line spacing returns by the
> StyledTextLineSpacingProvider, the StyledText switch to "variable line
> height", although it should not switch. The case of "not switch" is
> interesting for display only LineContentCodeMining (ex : JDT Java method
> parameter mining). In this config, the only display of LineContentCodeMining
> will be fast.

Ok, seems correct.

> Please note that setVariableLineHeight is called in
> getTextLayout too, so it is not required to call setVariableLineHeight in
> StyledText#setLineSpacingProvider.

That seems like a good improvement to reduce invocation of setVariableLineHeight and to make it invoked more dynamically according to real state rather than statically according to providers.

Comment 3 Angelo ZERR

2018-03-01 11:14:01 EST

> The resetCache AND the setVariableLineHeight() - the 2nd is important too and you only mention it later in your comment so I thought it would miss.

@Mickael, please see my last gerrit patch where I loop for each lines and reset the well lines according the custom line spacing coming from the call of lineSpacingProvider.getLineSpacing(i) . It fixes the JUnit in win32. In real case, this code is not required since super.draw() redraw lines and call getTextLayout which set the variable line height.

In real case with Java Editor and big file like StyledText.java, it takes around 30ms which is very fast. More if there is none custom line spacing (case with only draw line content mining like draw of Java parameter name mining, the StyledText keeps in fixed line height, so performance are very good)

Comment 4 Mickael Istria

2018-03-01 16:47:11 EST

Thanks Angelo.

Comment 5 Eclipse Genie

2018-03-01 16:47:14 EST

Gerrit change https://git.eclipse.org/r/118100 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a8dcd5187f8a0d9d39ee455a2e49300cfa0b5566

Comment 6 Angelo ZERR

2018-03-01 16:49:23 EST

Thanks @Mickael!