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

Move QgsRasterMinMaxOrigin enums to Qgis, promote to enum class #59644

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

nyalldawson
Copy link
Collaborator

No description provided.

@nyalldawson nyalldawson added the Cleanup Code cleanup label Nov 29, 2024
@github-actions github-actions bot added this to the 3.42.0 milestone Nov 29, 2024
Copy link

github-actions bot commented Nov 29, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 648d53d)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 648d53d)

@ptitjano
Copy link
Contributor

This looks good to me.

@rouault
Copy link
Contributor

rouault commented Nov 29, 2024

This has perhaps be discussed before, but shouldn't we feel concerned that qgis.h has become a 6700 lines monster that gets parsed again and again for all QGIS .cpp files ... ? Perhaps that's neglectable compared to other mandatory Qt headers ?

@nyalldawson
Copy link
Collaborator Author

This has perhaps be discussed before, but shouldn't we feel concerned that qgis.h has become a 6700 lines monster that gets parsed again and again for all QGIS .cpp files ... ?

Is it that problematic though? The bulk of the header is documentation or trivial enum declarations.

The intention here was to move enums (which are rarely changed) to a place that doesn't change much, so that we can change more headers to forward declare and ultimately make recompilation quicker. It's certainly much faster nowadays to recompile after a change to eg QgsMapLayer, where a few years ago this would have triggered an almost-full recompilation.

We're still in an in-between state though, because qgis.h ends up being changed semi regularly as we slowly move more enums into it. Over time this should slow (and eventually stop), and then it'll only be very rarely touched.

Perhaps that's neglectable compared to other mandatory Qt headers ?

QNamespace is around 2000, qstring 2000, qvariant 1000, qglobal around 1000. It certainly adds up... 🤪

@rouault
Copy link
Contributor

rouault commented Nov 29, 2024

doing some homework, creating a test program that just includes qgis.h,

on ubuntu 22.04:

# time g++  -fPIC -c ../src/core/qgis_light.cpp -I/usr/include/x86_64-linux-gnu/qt5/QtCore -I/usr/include/x86_64-linux-gnu/qt5 -Isrc/core
real	0m0.978s

# time clang++  -fPIC -c ../src/core/qgis_light.cpp -I/usr/include/x86_64-linux-gnu/qt5/QtCore -I/usr/include/x86_64-linux-gnu/qt5 -Isrc/core
real	0m1.010s

on fedora:rawhide:

# time g++  -fPIC -c ../src/core/qgis_light.cpp -I/usr/include/qt6/QtCore -I/usr/include/qt6 -Isrc/core
real	0m1.497s

# time clang++  -fPIC -c ../src/core/qgis_light.cpp -I/usr/include/qt6/QtCore -I/usr/include/qt6 -Isrc/core 2>&1 | grep open | wc -l
0
real	0m2.214s

This isn't due to the content we've added, which accounts for about 200 ms of the total time. The main culprit is just including QMetaEnum

But with precompiled headers:

on ubuntu 22.04:

# g++ -fPIC -c  ../src/core/qgis.h  -I/usr/include/x86_64-linux-gnu/qt5/QtCore -I/usr/include/x86_64-linux-gnu/qt5 -Isrc/core
# time g++  -fPIC -c ../src/core/qgis_light.cpp -I/usr/include/x86_64-linux-gnu/qt5/QtCore -I/usr/include/x86_64-linux-gnu/qt5 -Isrc/core
real	0m0.176s

and on fedora:rawhide:

# g++   -fPIC -c ../src/core/qgis.h  -I/usr/include/qt6/QtCore -I/usr/include/qt6 -Isrc/core
# time g++  -fPIC -c ../src/core/qgis_light.cpp -I/usr/include/qt6/QtCore -I/usr/include/qt6 -Isrc/core
real	0m0.228s

So there would be some value in having a precompiler header for qgis.h

@nyalldawson
Copy link
Collaborator Author

@rouault

Interesting!

So what's your conclusion? Should we continue these cleanups for now?

@rouault
Copy link
Contributor

rouault commented Nov 29, 2024

So what's your conclusion? Should we continue these cleanups for now?

Yes. We have other options to optimize build times

@nyalldawson nyalldawson merged commit c31d4dc into qgis:master Nov 29, 2024
39 checks passed
@nyalldawson nyalldawson deleted the raster_range_enums branch November 29, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants