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

Improve OpenGL text drawing performance. #3003

Closed

Conversation

unpacklo
Copy link

@unpacklo unpacklo commented Jan 19, 2023

Description

Improve text drawing performance with OpenGL by using a static instance of QOpenGLPaintDevice. I still haven't narrowed down exactly what is causing the performance problem but I can see creating a new QOpenGLPaintDevice causes every drawText() call to create and destroy GL buffer objects.

Yields about a 2x speedup for my typical setup which draws lots of lines for the celestial sphere.

Fixes # (issue)

Screenshots (if appropriate):

Before, 10 FPS:
image

After, 20 FPS:
image

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update

How Has This Been Tested?

Manually ran the change with a locally built stellarium and profiled with Instruments.

Test Configuration:

  • Operating system: macOS 13.1
  • Graphics Card: Radeon Pro 560X 4 GB
    Intel UHD Graphics 630 1536 MB

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (header file)
  • I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding your first pull request to Stellarium. If you have questions, please do not hesitate to contact us.

@alex-w alex-w added this to the 23.1 milestone Jan 19, 2023
Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@10110111
Copy link
Contributor

Even though this improves performance, this is still a hack, so it deserves a comment explaining why this object is static and what OS environment needs this.

@gzotti
Copy link
Member

gzotti commented Jan 19, 2023

I tend to agree with @10110111 on this. The Qt docs of this class warns to be careful when interacting with the GL state. Can we guarantee that nothing can break when we leave a static object for re-use? (Or is it better to use an instance member variable in StelPainter and take care of this?)

@10110111
Copy link
Contributor

Instance member will be worse because it will partially restore the performance dip, since StelPainter is created and destroyed multiple times per frame.

@gzotti
Copy link
Member

gzotti commented Jan 19, 2023

Maybe then the StelPainter should be kept alive and handed around for a longer time?

@10110111
Copy link
Contributor

Maybe then the StelPainter should be kept alive and handed around for a longer time?

It's not as easy, because StelPainter restores GL state on destruction. This would require a large refactoring.

@gzotti
Copy link
Member

gzotti commented Jan 19, 2023

Yes, I know. But if creation&destruction, i.e., state switching&restoring, happen so often, of course this is a plausible cause of slowdown. If we can hand around a StelPainter for a longer time, it should save a lot of overhead.

@alex-w
Copy link
Member

alex-w commented Mar 5, 2023

I got report about performance regression in Windows also :(

@10110111
Copy link
Contributor

10110111 commented Mar 5, 2023

I got report about performance regression in Windows also

Does this PR improve the situation?

@alex-w
Copy link
Member

alex-w commented Mar 5, 2023

Does this PR improve the situation?

  • 1.2 release with equatorial grid - 17-35 fps
  • 1.2 release with equatorial grid, no atmosphere, DSO, meridian, no ground - 17-20 fs
  • the current branch with equatorial grid - 17-40 fps
  • the current branch with equatorial grid, no atmosphere, DSO, meridian, no ground - 17-20 fs

macOS 13.2.1, MBP, M1

@alex-w
Copy link
Member

alex-w commented Mar 15, 2023

@unpacklo could you share your config.ini file?

@alex-w alex-w modified the milestones: 23.1, 23.2 Mar 21, 2023
@alex-w alex-w modified the milestones: 23.2, 23.3 Jun 22, 2023
@github-actions github-actions bot added the has conflicts The pull request has conflicts label Jul 27, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@10110111
Copy link
Contributor

The code that this PR changed has now been removed, hopefully the performance will be acceptable now. The PR itself is therefore obsolete.

@gzotti
Copy link
Member

gzotti commented Jul 30, 2023

We can close this, thanks to all participants!

@gzotti gzotti closed this Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has conflicts The pull request has conflicts
Development

Successfully merging this pull request may close these issues.

4 participants