You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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
The text was updated successfully, but these errors were encountered:
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.
Uh oh!
There was an error while loading. Please reload this page.
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:std::sort
or pdqsort are good defaults and don't require dragging a whole template-heavy library as a dependency.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.low_comparisons_sorter
is used, which now feels kind of arbitrary (which specifically that one), but also super expensive!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.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.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 testevery_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:cppsort::default_sorter
[[deprecated]]
in branch 1.xThe text was updated successfully, but these errors were encountered: