Description
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 orquick_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 usesmerge_sorter
, which is a different algorithm with a different space complexity altogether. The difference might not matter much forstd::sort
/std::stable_sort
since those are definitely presented as different algorithms, but this is cpp-sort and one should expectstable_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 itsstable_adapter
specialization does not useself_sort_adapter
, which both surprising and a bug. - One could argue that we need a default sorter for
cppsort::sort
andcppsort::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
Labels
Projects
Status