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

spinsort and flat_stable_sort don't compile with -fno-exceptions or -fno-operator-names GCC flags #45

Open
vedgy opened this issue Nov 2, 2020 · 12 comments
Assignees

Comments

@vedgy
Copy link

vedgy commented Nov 2, 2020

Boost version: 1.72.0 (Arch Linux package version: boost 1.72.0-2).

The other two sorting methods I tested - spreadsort::float_sort and pdqsort - work just fine with these compiler flags.

These two flags are used by default in KDE: https://invent.kde.org/frameworks/extra-cmake-modules/-/blob/master/kde-modules/KDECompilerSettings.cmake. Possible workaround (which will hopefully work): create a tiny static library that disables these flags and calls spinsort or flat_stable_sort.

@fjtapia
Copy link
Collaborator

fjtapia commented Nov 3, 2020 via email

@shehab-ashraf
Copy link

Hey @fjtapia ,
are you willing to accept new contributors? I am interested into contributing to this repository so, if you could explain a bit more this feature request, it would be great.
Thanks

@nigels-com
Copy link
Contributor

I was doing some performance characterisation work recently. I had some difficulty building in gcc --no-exceptions mode due to boost::block_indirect_sort. The workaround for me was to simply comment out the offending try, catch and throws.

But I did also happen across BOOST_NO_EXCEPTIONS and Boost.ThrowException which does seem intended as a way of supporting both with and without exception handling.

There are only 20 or so places in boost::sort throwing exceptions, so seems quite doable to make a change to support BOOST_NO_EXCEPTIONS mode. The performance improvement in my testing amounted to about 2% overall, which isn't so compelling on the face of it. But it does seem like it might be more significant for a different code path and we might like to use --no-exceptions mode, and it would be nice for boost::sort to support that.

I don't think that boost::sort performance would be significantly improved with --no-exceptions directly. But if the rest of the codebase is in C, or not especially exception-happy C++, it could make sense. The other example I came across is spdlog SPDLOG_NO_EXCEPTIONS.

@fjtapia
Copy link
Collaborator

fjtapia commented Feb 1, 2024 via email

@fjtapia
Copy link
Collaborator

fjtapia commented Feb 29, 2024 via email

@vedgy
Copy link
Author

vedgy commented Feb 29, 2024

Hi Francisco,

I am not a proponent or advocate of this flag myself. I had tried to use flat_stable_sort() to speed up opening a project in KDevelop, found the missing support a significant obstacle, and so logged this bug. Eventually I had found and switched to timsort(), because it turned out to be much faster than flat_stable_sort() for the KDevelop's use case (flat_stable_sort() had been a strong second-fast sorting algorithm for the use case).

I just searched and found the original proposal and discussion of -fno-operator-names in KDE: https://phabricator.kde.org/D3850. I think the discussion can be summarized like this:

  • the flags contradict the C++ standard, so enabling them by default in KDE code is not universally agreed on;
  • there is a practical portability benefit (not breaking a MSVC build) of using this flag by default to compile KDE code, because not all KDE projects are regularly compiled on Windows;
  • the flag can be overridden for [sub]projects that depend on 3rd-party libraries/headers, which do not compile with it.

@fjtapia
Copy link
Collaborator

fjtapia commented Feb 29, 2024 via email

@vedgy
Copy link
Author

vedgy commented Mar 1, 2024

In the KDevelop use case, the elements are relatively expensive to compare and move, and indeed are mostly sorted from the start. So flat_stable_sort() handily outperforms std::stable_sort(), std::sort(), boost::sort::spinsort() and every other sorting algorithm I have tried except for timsort(), which is even more fine-tuned for this specific task.

Thanks for making flat_stable_sort() compile with -fno-exceptions and -fno-operator-names. Now KDE projects can use it without hindrance.

EDIT: reopening the issue because boost::sort::spinsort() does not seem to compile with these flags yet.

@vedgy vedgy closed this as completed Mar 1, 2024
@vedgy vedgy reopened this Mar 1, 2024
@nigels-com
Copy link
Contributor

I think there is fairly strong argument to avoid operator names.

At our organisation they are strongly discouraged. The compiler flags are set accordingly.
Having to manage that for dependencies with other opinions is simply work that needs doing, of no functional or practical value.

Ease of integration with other code bases is a positive, even if it isn't as we'd personally prefer it to be.
Minimise the friction.

@fjtapia
Copy link
Collaborator

fjtapia commented Mar 1, 2024 via email

@vedgy
Copy link
Author

vedgy commented Mar 1, 2024

If you could provide me with some information about the type of data to
sort, the size of the data, the number of elements to sort, and approximate
times, I would appreciate it

First of all, timsort() works fast enough that further optimization is not very important to me. So I am not asking you to optimize for my use case :)

I have already described general characteristics of the sorting task. More specifically, the element is a struct that contains: 1) two copy-on-write QVector<QString>s with a custom optimized comparison; 2) an expensive to move/copy but cheap to compare custom KDevelop type. The initial order consists of usually long runs of already sorted elements: paths obtained from a file system API, then inserted into a file-system tree, then appended to a QVector (a Qt class similar to std::vector) via depth-first-search of the tree.

If you need more details, here is the optimizing commit: https://commits.kde.org/kdevelop/09426bcff8060b1b6f1bb450c2ce6595861f3c0f. There are automated KDevelop benchmarks that are affected by the choice of sorting algorithm. But the main criterion of my sorting algorithm selection was manual benchmarking: opening large projects in KDevelop and comparing intrusive QElapsedTimer measurements of the sorting call.

@fjtapia
Copy link
Collaborator

fjtapia commented Mar 1, 2024 via email

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

No branches or pull requests

4 participants