Avoid Vector_quickSort pivotIndex overflow

[Explorer09]

I don't know if I need to bother fix this one. Since we could replace the whole "quickSort" algorithm with srandard C qsort(). This quickSort function can be trace to this very old commit: 7ca1081. I have no idea why @hishamhm chose a custom "quickSort" algorithm rather than a standard C qsort() function.

[natoscott]

+1 for using qsort.

[hishamhm]

I have no idea why @hishamhm chose a custom "quickSort" algorithm rather than a standard C qsort() function.

The answer is much simpler and mundane than you can imagine: I was still learning C. :-) In fact, besides "scratching my itch for a better top", practicing C was the other motivation that 20+-years-younger me had for starting this project.

That also explains other oddities from the early codebase, such as trying to avoid maintaining .h files by hand (which I found nonsensical), the Java-esque OOP collections, and my favorite, which I think I never told to anyone before: the utility module with functions for setting up drawing to the screen, "CRT", is named so as a nod to the one in Turbo Pascal. :-D

Most importantly, I think all of those rookie oddities show how you don't have to be an experienced programmer to make a significant contribution to FOSS!

[Explorer09]

@hishamhm Well. It's not a bad thing for contributing to OSS for trying to learn C. The question that interests me is: You seems to be capable of using Object_Compare function type but not realising the size_t data type when you wrote a sort function.

It seems there are no objections for ditching this custom quickSort for standard C qsort. I can revise this pull request to make the switch. This gives me one less int type issue to worry about when I try to migrate the Vector size data types to size_t.

[hishamhm]

@Explorer09 I don't recall the details; maybe I just wasn't aware of the qsort function, who knows! And for sure I wasn't super consistent with my handling of integer types. Back then I was most certainly running Linux i686, so it was the era of 32-bit pointers, and using int everywhere "just worked".

[Explorer09]

@hishamhm In 32-bit systems int type has the same size as size_t and so many people weren't aware of the differences. But as we are now in 64 bits the int vs. size_t differences becomes significant and this is why the recent set of pull requests to eventually address that. 🙂

Also note that (left + right) / 2 was a common mistake for people who implement binary search themselves (not technically necessary for quicksort, but I don't blame them of choosing (left + right) / 2 as a pivot). left + right can overflow for large arrays and so the proper way should be left + ((right - left) / 2).
The other potential problem of a custom quicksort is not addressing the worst case (O(n²)) properly:

• When sorting sub-arrays after partitioning, the sub-array of the smaller size should get sorted first. That way the compiler can optimize tail calls when sorting the larger sub-array and minimize the recursion depth.
• Standard C library's qsort could work better by implementing Introsort rather than a naïve Quicksort. You know, Introsort switches to Heapsort after a certain recursion depth is reached, thus it keeps the worst case time complexity down.

[hishamhm]

Another sign of the times, now that talking about it reminds me of the old days, was that when I started htop, pid numbers were much smaller. Back in the day, it was IIRC a signed 16-bit value. That means I originally didn't expect any Vectors to be larger than 32768, so overflow using 32-bit indices was not a practical concern! Fun trip down memory lane.

Of course, later Vectors were used for more things, and pids (and systems!) got larger. But back when I first implemented that Vector it was unthinkable to me that its 32-bit indices could overflow: I don't think my machine had enough RAM for that. :)

[BenBE]

Don't keep commented-out code around. That's what a VCS is for …

[Explorer09]

Then I would have to delete the commented-out combSort code as well...
Would @hishamhm comment on this?

[hishamhm]

I'm happy to provide historical commentary, but I defer to the current maintainers on all things code review!

[BenBE]

The combSort vs. insertionSort place is a different part of the code.

But as sorting has currently two implementations, this is some other cleanup task (#1785) anyway …

[BenBE]

If it weren't for qsort_r only being standardized in POSIX.2024, I'd opt for using it; avoiding the need for global variables. Speaking of which: the wrapper/adapter should check the vectorCompareFn for NULL (marked unlikely to the optimizer).

FWIW: If we wanna be fancy, we could detect a compatible qsort_r implementation in configure.ac and prefer that if available; falling back to qsort only if stars don't align.

[Explorer09]

I wish to keep the changes small and leave the qsort_r support to a separate commit.

POSIX standardised the GNU version of qsort_r but before GNU, FreeBSD had a version of qsort_r that was ABI-incompatible. Although checking for qsort_r is possible in configure, it's not trivial code. Given that we still need to support platforms without qsort_r, I consider this qsort_r thing a lower priority.

[BenBE]

Same here. Exactly because qsort_r support is such a mess I'd suggested to skip it for now and revisit it later..

Which leaves the one NULL check I mentioned above as an item TBD …

[Explorer09]

@BenBE Yes. I plan to add an assert(!vectorCompareFn) before assignment in order to catch whether Vector_*Sort is called in a signal handler or multithreaded code by mistake.

[BenBE]

That's not what I was referring to. And also won't actually help except as a canary.

[BenBE]

I pushed the relevant (short-term) fix from this PR in ee1d1a8.

The question whether we want to update the actual sorting implementation and replace it by something more sprakling should be discussed while known bugs in existing code are fixed. I.e. focus on fixing the original issue, improve the algorithm in a second step.

@Explorer09 Please try to base your algorithm replacement work onto d3cd557. That way we can later compare the different sort implementations.