Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad performance for validate/repaint on Linux/GTK3 with overlay scrollbars #425

Closed
wiresketch opened this issue Apr 16, 2024 · 7 comments · Fixed by #428
Closed

Bad performance for validate/repaint on Linux/GTK3 with overlay scrollbars #425

wiresketch opened this issue Apr 16, 2024 · 7 comments · Fixed by #428

Comments

@wiresketch
Copy link
Contributor

I've received several performance reports from my users and after some extensive testing and debugging I've identified a pretty serious performance issue in Draw2d's validate/repaint cycle. This cycle is performed by DeferredUpdatedManager and the performance issues appear specifically on Linux/GTK3 with overlay scrollbars enabled (which is normally the default on current Linux distributions). For this issue to manifest itself the scrollbars must be visible, meaning the content in the graphical viewer should be larger than the available area.

I believe this is the same issue that is mentioned by @azoitl in #372 where he talks about the drawing performance caused by ScrollPaneSolver, but it looks like he didn't investigate deeper.

The issue is indeed in ScrollPaneSolver.solve and the core of this issue is the relationship between SWT's Control.getBounds, Scrollable.getClientArea and Scrollable.computeTrim. The normal behavior that can be seen on Windows and macOS is the following:

  1. Scrollbars are invisible: clientArea = bounds.
  2. Scrollbars are visible clientArea = bounds - trims

The ScrollPaneSolver.solve method encodes this relationship between bounds/clientArea/trim to calculate and set Viewport's bounds. This method is called from FigureCanvas.layoutViewport. If scrollbars are visible then the solve method will subtract scrollbar's width/height from the passed bounds to calculate the viewportArea. This area needs to be the same as the client area as returned from FigureCanvas.getClientArea as can be seen lower in the FigureCanvas.layoutViewport method which uses FigureCanvas.getClientArea to set it on RootFigure.

This all fails on Linux/GTK when overlay scrollbars are enabled (which is normally the default now). With overlay scrollbars enabled the relationship between bounds, client area and trims changes. Regardless of the fact that scrollbars are visible or not, the client area is always the same with the bounds of the Scrollable (FigureCanvas in our case). ScrollPaneSolver.solve doesn't account for this case, which on it's turn causes the bounds of the Viewport to be set to the calculated viewportArea and later to the actual client area (which will be different) (via FigureCanvas.layoutViewport / RootFigure.setBounds / layout on next cycle / FigureCanvas.setBounds). This means that the bounds of the Viewport will change on each validation cycle (when scrollbars are visible) and cause the entire visible area of the GraphicalViewer to be repainted, instead of repairing only the damaged area.

Overlay scrollbars behavior can be disabled using GTK_OVERLAY_SCROLLING=0 environment variable. The check for this variable can also be found in SWT code. With this variable set the space for visible scrollbars is always reserved and so the relationship between bounds/clientArea/trims becomes the same as on Windows and macOS and the performance issue disappears.

I've also tested older versions of GEF as far back as to 2016. The problem is present there too so my conclusion is that it's an issue caused by the addition of overlay scrollbars in modern Linux distributions which was never accounted for in GEF code (unlike in SWT).

To make things complete I've also made some tests with GTK2. There it worked as expected as no overlay scrollbars are being used there.

I've created a SWT snippet which allows to visualize (in the console) the relationship between bounds/clientArea/trims when scrollbars are enabled/disabled. Feel free to test it out. Double-click toggles scrollbars visibility. Single click prints out bounds/clientArea/trims.
ScrollbarsTest.java.txt

I am still not sure what the correct solution is. The most obvious one would be to modify ScrollPaneSolver.solve to check for GTK platform and for GTK_OVERLAY_SCROLLING environment variable to alter its behavior to match the native one. Please chime in with better proposals.

ScrollPaneSolver.solve is also being used in ScrollPanelLayout which on it's turn is used at least in Palette's drawers. I didn't check the behavior there and so for now I am not sure how the fix would impact this class.

It would be great if this could be fixed for Eclipse 2024-06 as in some cases the performance issue can be quite severe.

@ptziegler
Copy link
Contributor

ptziegler commented Apr 16, 2024

I am still not sure what the correct solution is. The most obvious one would be to modify ScrollPaneSolver.solve to check for GTK platform and for GTK_OVERLAY_SCROLLING environment variable to alter its behavior to match the native one. Please chime in with better proposals.

I'd like to avoid any platform-specific checks, if possible. But I believe one can achieve something similar by comparing Scrollable.getScrollbarsMode() to SWT.SCROLLBAR_OVERLAY.

I can try to have a look over the next few weeks, but as it is with FOSS, there is always a lot to do with only a limited amount of capacity... :/

@merks
Copy link
Contributor

merks commented Apr 16, 2024

It’s really quite easy to set up a development environment and create a pull request

https://github.com/eclipse/gef-classic/blob/master/CONTRIBUTING.md#set-up-a-development-environment

For most of my projects I’m much more motivated to help when I see folks take steps to help. The analysis of the underlying details of the problem is really very well done so you probably have some good ideas for how it might be addressed.

@azoitl
Copy link
Contributor

azoitl commented Apr 16, 2024

Thank you for the deep analysis. This sounds a lot like the problem that I initially observed. There are two reasons I stopped looking further:

  1. Under Linux we have a much bigger problem that the ways Draw2d interacts with SWT creates a tremendous amount of garbage, which was for me a much bigger performance issue. One reason I started Added new Figure Container to serve as light weight drawing container #368 and the work on the scrolling.
  2. I was never sure if it is a real problem as I couldn't consistently reproduce it. But with your work this changed now.

@wiresketch
Copy link
Contributor Author

@ptziegler

I'd like to avoid any platform-specific checks, if possible. But I believe one can achieve something similar by comparing Scrollable.getScrollbarsMode() to SWT.SCROLLBAR_OVERLAY.

I've tested out Scrollable.getScrollbarsMode() method but unfortunately it's result is not reliable. My results are as follows:

  1. Windows - Always SWT.NONE - ok
  2. macOS - A fluke on snippet start, as the result is SWT.SCROLLBAR_OVERLAY, but afterwards SWT.NONE as expected. ok enough.
  3. Linux/GTK3 - Always SWT.SCROLLBAR_OVERLAY regardless of the presence of GTK_OVERLAY_SCROLLING=0 environment variable.

So on Linux this method cannot be relied upon. I can see is that Scrollable.gtk_draw method uses OS.GTK_OVERLAY_SCROLLING_DISABLED constant (which is populated explicitly from GTK_OVERLAY_SCROLLING environment variable) for some kind of workaround. So this is a good indication that there too the result of getScrollbarsMode is not enough.

This might be a SWT bug I am not competent enough to affirm that. My feeling is that Scrollable.getScrollbarsMode also needs to take in consideration the value of OS.GTK_OVERLAY_SCROLLING_DISABLED on GTK3.

wiresketch added a commit to wiresketch/gef-classic-issue-425 that referenced this issue Apr 17, 2024
is enabled (eclipse#425)

Detect when overlay scrolling is enabled on GTK and it's the case then
ignore scrollbar trims when calculating the viewport client area.
@wiresketch
Copy link
Contributor Author

I've added a pull request with a potential fix for this issue. The fix is not ideal as explained in the pull request, but it solves the immediate problem.

This fix only touches FigureCanvas and doesn't touch ScrollPaneSolver class which is being also used in ScrollPaneLayout. Since ScrollPaneLayout doesn't use native Control.getBounds or Scrollable.getClientArea methods the fix is unnecessary there.

Please give it a try.

@azoitl Since you've experienced performance problems yourself, could you confirm that it improves things for you?

ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 17, 2024
Depending on whether the GTK_OVERLAY_SCROLLING_ENABLED property is set,
the width and height of the scrollbar should be ignored when calculating
the bounds of the root figure.

eclipse#425
@ptziegler
Copy link
Contributor

For the sake of understanding: Assuming I have a canvas with size (500, 500) and scroll bars with width/height 19.

If GTK_OVERLAY_SCROLLING is enabled, then the client area should always be (500, 500), given that the scroll bars are on top of the client area.

But if the flag is disabled, then the client area should only be (481, 481), as the scroll bars are now a separate entity and therefore take up some of the available space.

Did I get this right?

@wiresketch
Copy link
Contributor Author

@ptziegler Yes, your understanding is correct.

ptziegler pushed a commit to ptziegler/gef-classic that referenced this issue Apr 25, 2024
is enabled (eclipse#425)

Detect when overlay scrolling is enabled on GTK and it's the case then
ignore scrollbar trims when calculating the viewport client area.
ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 25, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

eclipse#425
ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 25, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

eclipse#425
ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 25, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

eclipse#425
wiresketch added a commit to wiresketch/gef-classic-issue-425 that referenced this issue Apr 26, 2024
is enabled (eclipse#425)

Detect when overlay scrolling is enabled on GTK and it's the case then
ignore scrollbar trims when calculating the viewport client area.
azoitl pushed a commit that referenced this issue Apr 26, 2024
is enabled (#425)

Detect when overlay scrolling is enabled on GTK and it's the case then
ignore scrollbar trims when calculating the viewport client area.
ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 26, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

eclipse#425
ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 26, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

eclipse#425
ptziegler added a commit to ptziegler/gef-classic that referenced this issue Apr 30, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

eclipse#425
azoitl pushed a commit that referenced this issue May 1, 2024
Depending on the value of Scrollable.getScrollbarsMode(), the width and
height of the scrollbar should be ignored when calculating the bounds of
the root figure.

#425
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants