Skip to content

Remove default_sorter #168

Closed
Closed
@Morwenn

Description

@Morwenn

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    Status

    Done

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions