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

Remove default_sorter #168

Closed
4 tasks done
Morwenn opened this issue Jul 16, 2020 · 0 comments
Closed
4 tasks done

Remove default_sorter #168

Morwenn opened this issue Jul 16, 2020 · 0 comments
Milestone

Comments

@Morwenn
Copy link
Owner

Morwenn commented Jul 16, 2020

default_sorter is wasteful and arguably not a good default, it would be better to remove it in cpp-sort 2.0.0, here are a few reasons why:

  • The goal of the library is to provide a variety of sorting algorithms and related tools to experiment; if one wants a default sorting algorithm which is good enough for most situations, then std::sort or pdqsort are good defaults and don't require dragging a whole template-heavy library as a dependency.
  • The backing sorters used are either pdq_sorter for random-access-iterators or quick_sorter for other iterators, which means that the complexity degrades from O(n log n) to O(n²) when the iterators are not random-access. It's quite a hard difference in some scenarios and not always acceptable for a default.
  • When the collection passed is a small fixed-size arrays, then low_comparisons_sorter is used, which now feels kind of arbitrary (which specifically that one), but also super expensive!
  • Moreover when adapted with stable_adapter, default_sorter simply uses merge_sorter, which is a different algorithm with a different space complexity altogether. The difference might not matter much for std::sort/std::stable_sort since those are definitely presented as different algorithms, but this is cpp-sort and one should expect stable_adapter to actually apply its usual rules and not to switch to a wholly different algorithm.
  • The whole thing is wrapped in self_sort_adapter, which is actually something you probably want to decide yourself, and not have as a default and not understand why the errors are sometimes coming from a different place. And I just noticed it but its stable_adapter specialization does not use self_sort_adapter, which both surprising and a bug.
  • One could argue that we need a default sorter for cppsort::sort and cppsort::stable_sort, but I also plan to get rid of those in version 2.0.0 (Remove cppsort::sort and cppsort::stable_sort #167).

We are pretty much done for the design decision of killing default_sorter, however there are other down-to-earth reasons to remove it: the deep nesting of several sorters and adapters means that it is slow to compile and produces hard to read error messages, which is something we want to avoid (see #125 and #28) and especially so for a default sorter.

Here is a flame graph generated with Clang's -ftime-trace showing the time spent parsing some includes of the test every_sorter.cpp:

As can be seen, default_sorter takes as much time to parse as most of the other sorters combined, but it also produces a bunch of wasteful instantiations while most of its features will most likely end up unused - if not totally unwanted - in the common scenarios.

Basically it is a confusing & badly designed mess, and while some aspects could be changed/fixed, it will only waste resources and developer time, and does not quite fit the design of the library. It needs to go.


However it is a rather valuable tool for the test suite thanks to its deeply nested nature, so it might be worth preserving there under another name (without the stable_adapter specialization). A quick roadmap:

  • Nuke cppsort::default_sorter
  • Mark it as [[deprecated]] in branch 1.x
  • Update the documentation accordingly
  • Add an equivalent to the test suite under a different name
@Morwenn Morwenn added this to the 2.0.0 milestone Jul 16, 2020
Morwenn added a commit that referenced this issue Jul 17, 2020
This is the first step to needed to deprecate and remove components from
the library (see issues #167 and #168).
Morwenn added a commit that referenced this issue Jul 18, 2020
Morwenn added a commit that referenced this issue Aug 18, 2020
The specialization stable_adapter<default_sorter> triggered a
deprecation warning when default_sorter.h was included, even when the
class was not otherwise used. This commit defines the specialization
before default_sorter itself to avoid that warning.
Morwenn added a commit that referenced this issue Jul 4, 2022
Morwenn added a commit that referenced this issue Jul 4, 2022
@Morwenn Morwenn closed this as completed Nov 12, 2022
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

1 participant