Create Task
Maniphest T17188

Proposal for changing the way how we link internal Krita libraries together
Open, Needs TriagePublic
Actions

Description

Last week I investigated the problem of too slow Krita's build speed on the Windows CI and I found quite a few interesting issues that I would like to discuss with the wider community.

Problem definition

  1. Our libraries export too many include paths in their public interface E.g. kritaui has 97(!) inclide path, which are passed with -I option. You can see them in the list below.

    Most of these paths are basically useless, since just link to the build folders of the various libraries (for foobar_export.h headers and configuration files). The other part of the paths is the subfolders of the library, like 9(!) subfolders of kritaui and 5(!) subfolders of kritaimage.
-IC:/dev/env-8/b-krita-ccache/libs/ui 
-IC:/dev/env-8/krita/libs/ui 
-IC:/dev/env-8/b-krita-ccache/libs/ui/kritaui_autogen/include 
-IC:/dev/env-8/b-krita-ccache 
-IC:/dev/env-8/krita 
-IC:/dev/env-8/krita/winquirks 
-IC:/dev/env-8/krita/libs/ui/qtlockedfile 
-IC:/dev/env-8/krita/libs/ui/canvas 
-IC:/dev/env-8/krita/libs/ui/flake 
-IC:/dev/env-8/krita/libs/ui/ora 
-IC:/dev/env-8/krita/libs/ui/tool 
-IC:/dev/env-8/krita/libs/ui/utils 
-IC:/dev/env-8/krita/libs/ui/widgets 
-IC:/dev/env-8/krita/libs/ui/widgets/gradient 
-IC:/dev/env-8/krita/libs/ui/input/wintab 
-IC:/dev/env-8/b-krita-ccache/libs/version 
-IC:/dev/env-8/krita/libs/version 
-IC:/dev/env-8/b-krita-ccache/libs/impex 
-IC:/dev/env-8/krita/libs/impex 
-IC:/dev/env-8/krita/libs/image/brushengine 
-IC:/dev/env-8/krita/libs/image/filter 
-IC:/dev/env-8/krita/libs/image/generator 
-IC:/dev/env-8/krita/libs/image/layerstyles 
-IC:/dev/env-8/krita/libs/image/processing 
-IC:/dev/env-8/b-krita-ccache/libs/image 
-IC:/dev/env-8/krita/libs/image 
-IC:/dev/env-8/b-krita-ccache/libs/widgets 
-IC:/dev/env-8/krita/libs/widgets 
-IC:/dev/env-8/b-krita-ccache/libs/global 
-IC:/dev/env-8/krita/libs/global 
-IC:/dev/env-8/krita/libs/flake/commands 
-IC:/dev/env-8/krita/libs/flake/tools 
-IC:/dev/env-8/krita/libs/flake/svg 
-IC:/dev/env-8/krita/libs/flake/text 
-IC:/dev/env-8/b-krita-ccache/libs/flake 
-IC:/dev/env-8/krita/libs/flake 
-IC:/dev/env-8/krita/libs/pigment/resources 
-IC:/dev/env-8/krita/libs/pigment/compositeops 
-IC:/dev/env-8/b-krita-ccache/libs/pigment 
-IC:/dev/env-8/krita/libs/pigment 
-IC:/dev/env-8/b-krita-ccache/libs/koplugin 
-IC:/dev/env-8/krita/libs/koplugin 
-IC:/dev/env-8/b-krita-ccache/libs/store 
-IC:/dev/env-8/krita/libs/store 
-IC:/dev/env-8/b-krita-ccache/libs/resources 
-IC:/dev/env-8/krita/libs/resources 
-IC:/dev/env-8/b-krita-ccache/libs/command 
-IC:/dev/env-8/krita/libs/command 
-IC:/dev/env-8/krita/libs/widgetutils/config 
-IC:/dev/env-8/krita/libs/widgetutils/xmlgui 
-IC:/dev/env-8/b-krita-ccache/libs/widgetutils 
-IC:/dev/env-8/krita/libs/widgetutils 
-IC:/dev/env-8/b-krita-ccache/libs/multiarch 
-IC:/dev/env-8/krita/libs/multiarch 
-IC:/dev/env-8/b-krita-ccache/libs/resourcewidgets 
-IC:/dev/env-8/krita/libs/resourcewidgets 
-IC:/dev/env-8/b-krita-ccache/libs/psdutils 
-IC:/dev/env-8/krita/libs/psdutils 
-IC:/dev/env-8/b-krita-ccache/libs/metadata 
-IC:/dev/env-8/krita/libs/metadata 
-IC:/dev/env-8/b-krita-ccache/libs/color 
-IC:/dev/env-8/krita/libs/color 
-IC:/dev/env-8/b-krita-ccache/libs/brush 
-IC:/dev/env-8/krita/libs/brush 
-Isystem C:/dev/env-8/i/include/QtGui/5.15.7 
-Isystem C:/dev/env-8/i/include/QtGui/5.15.7/QtGui 
-Isystem C:/dev/env-8/i/include/QtCore/5.15.7 
-Isystem C:/dev/env-8/i/include/QtCore/5.15.7/QtCore 
-Isystem C:/dev/env-8/i/include/KF5/KCoreAddons 
-Isystem C:/dev/env-8/i/include/KF5 
-Isystem C:/dev/env-8/i/include 
-Isystem C:/dev/env-8/i/include/QtCore 
-Isystem C:/dev/env-8/i/./mkspecs/win32-clang-g++ 
-Isystem C:/dev/env-8/i/include/KF5/KCompletion 
-Isystem C:/dev/env-8/i/include/QtWidgets 
-Isystem C:/dev/env-8/i/include/QtGui 
-Isystem C:/dev/env-8/i/include/KF5/KI18n 
-Isystem C:/dev/env-8/i/include/KF5/KItemViews 
-Isystem C:/dev/env-8/i/include/QtNetwork 
-Isystem C:/dev/env-8/i/include/eigen3 
-Isystem C:/dev/env-8/i/include/boost-1_80 
-Isystem C:/dev/env-8/i/include/QtConcurrent 
-Isystem C:/dev/env-8/i/include/QtXml 
-Isystem C:/dev/env-8/i/include/QtSql 
-Isystem C:/dev/env-8/i/include/KF5/KConfig 
-Isystem C:/dev/env-8/i/include/KF5/KConfigGui 
-Isystem C:/dev/env-8/i/include/KF5/KConfigCore 
-Isystem C:/dev/env-8/i/include/OpenEXR 
-Isystem C:/dev/env-8/i/include/KF5/KWidgetsAddons 
-Isystem C:/dev/env-8/i/include/QtSvg 
-Isystem C:/dev/env-8/i/include/freetype2 
-Isystem C:/dev/env-8/i/include/harfbuzz 
-Isystem C:/dev/env-8/i/include/QtPrintSupport 
-Isystem C:/dev/env-8/i/include/KF5/KGuiAddons 
-Isystem C:/dev/env-8/i/include/SDL2 
-Isystem C:/dev/env-8/i/include/mlt-7 
-Isystem C:/dev/env-8/i/include/mlt-7/mlt++
  1. Every time clang-15.exe or moc.exe see #include <foobar.h> line in the source it has to browse through the entire list to locate the actual location of the header.
  1. Clang and moc use GetFileAttributes and GetFileAttributesEx API calls correspondingly to check if the file exists in a particular location. And the problem is that these calls are EXTREMELY SLOW on Windows running over NTFS filesystem. And, yes, that is the fastest way available on Windows for checking for the existence of the file, and, yes, NTFS is the only filesystem available on Windows :(

    I personally tried to use CreateFile method for moc.exe, but it was much slower.
  1. GetFileAttributesEx is so slow that fully(!) ccache'd build of Krita on CI takes about 25 minutes(!) and all this time is spent on generation of the .moc files. And 78% of the time in moc.exe is spent in GetFileAttributesEx, looking up header files...
  1. Some of the include paths (e.g. freetype2 and harfbuzz) appear in the list because the library exports them to PUBLIC for unittests.

So we have a problem of extremely slow builds on Windows (and Windows CI). And the cause of these slow builds is a mess in includes that our libraries export (via target_link_libraries(foo PUBLIC bar)). I did some experiments with that, so I would like to propose a set of guidelines/policies on how we should manage our inter-krita library dependencies:

Proposed guidelines (to be discussed)

I made a list of guidelines that could probably help reduce the amount of libraries we link. I don't feel very confident in this topic, so I would really like to hear some input on that:

  1. KDECMakeSettings scripts enables CMAKE_INCLUDE_CURRENT_DIR and CMAKE_INCLUDE_CURRENT_DIR_IN_INTERFACE for the entire Krita project. These macros add current source and build directories into the public interface of the library. We should disable that and add current directory only into PRIVATE section and only when needed.
    • TODO: how should we do that? patch KDECMakeSettings or just unset variable in our code right after the include? (the latter approach works in the current version of CMake, I tested, but it is basically UB)
  1. All configure_file and generate_export_header macros should generate the headers into some common directory, e.g. <krita-build-root>/generated. Then we won't have a 21 include paths, each containing a single generated header.
  1. A krita library (e.g. kritaimage) should publish only one "root" library. E.g. for kritaimage it is <krita-src-root>/libs/image
  1. [NEEDS DISCUSSION] Subfolders. Subfolders is a big problem. I'm not really sure how to handle it. My original suggestion was like that:
    • if files from a subfolder are exported a lot, then just cut it off it into a separate library (e.g. libs/widgetutils/xmlgui)
    • if files from a subfolder are mostly used inside the library, then keep them in a subfolder and access via angled notation with a subfolder name, e.g. #include <layerstyles/kis_layer_style_projection_plane.h>

      TODO: The main problem of this approach is that I'm not sure that splitting stuff into a separate library will recude the number of -I switches in the kritaui library. Perhaps we have an issue that kritaui has become a "superlibrary" that links to everything?
  1. [NEEDS DISCUSSION] As far as I can understand we should prefer angular includes for the files that might be used outside of the current library, e.g. #include <kis_image.h> instead of `#include "kis_image.h". That is google's policy for includes, and, afaict, the reasoning for that is that the meaning for ""-includes is implementation-defined and differs between MSVC and gcc/clang.
  1. We shouldn't export a library via PUBLIC interface if the only use for this export is a unittest. It pollutes the overal interface of the library.
    • TODO: should we introduce some custom properties like UNITTEST_INTERFACE_LIBRARIES for that?
dkazakov created this task.Mar 6 2024, 10:02 AM
rempt added a subscriber: rempt.Mar 6 2024, 10:56 AM
  1. I'm not sure how to do that.
  2. that sounds like a good idea
  3. I am simply not sure.
  4. I'm pretty sure that kritaui _is_ a super library that links to everything , and that's an issue. I'm fine with the subfolder/header.h approach, but that needs a lot of, hopefully automatable, changes. I don't think it's useful to split those folders out in separate libraries, since I'm not sure those files actually don't refer to files in other subfolders of the same library, or files in the toplevel library. I'm also not sure it would actually reduce the number of -I switches.
  5. The angular brackets thing for includes outside has actually been our coding standard since the kimageshop days, but lots of people are pretty sloppy -- and then also, files get moved from library to library.
  6. if that's possible, it's a very good idea
dkazakov added a comment.Mar 6 2024, 3:07 PM

The angular brackets thing for includes outside has actually been our coding standard since the kimageshop days, but lots of people are pretty sloppy -- and then also, files get moved from library to library.

As far as I can tell, Google's policy is to use angular brackets even for accessing files inside the same directory/library. Was it also the case for us?

vanyossi added a comment.Mar 12 2024, 7:55 AM
  1. We could patch it to add the to default PRIVATE, it may break all other framework builds though. So I would better suggest to turn OFF CMAKE_INCLUDE_CURRENT_DIR and CMAKE_INCLUDE_CURRENT_DIR_IN_INTERFACE to avoid the possible mess with the other packages depending on ecm.
  2. Sounds like a good idea to do so.
  3. Im not very in favor of monolithic libs, however since we are not developing library only package, it might be a point in favor of reducing the number of libraries.
  4. Im not sure either, and I suspect the case of a lot of export from a subfolder is not very generalized. In any case Im not sure either if the includes would reduce by doing this.
  5. We were never very vocal about it, neither in HACKING or Framework_Coding_Style. However I do think its a good approach.
  6. True, let's keep things in their scope.