6

I'm currently writing a quick sort algorithm in Java to sort random arrays of integers and then timing them using System.nanoTime(). The size of these arrays are powers of ten, starting from 10^3 and ending at 10^7. Furthermore, the random lists have different properties. I am sorting purely random lists, lists with some identical values (fewUnique), reversed sorted lists, sorted lists and nearly sorted lists.

The sort works. It performs a quick sort recursively on the array until it needs to sort 30 elements or less of the array, in which case it performs an insertion sort.

All was fine for 10^3 and 10^4 but once I got to 10^5 values it would only sort the random, few unique and random lists but would incur a Stack Overflow error when sorting nearly sorted and sorted lists.

I don't believe the issue lies in the way the lists are generated because the Stack Overflow occurs within the sorting algorithm (the line which is being referred to by the compiler is the first within the findPivot() method). Also, it will often sort between 1 and 6 lists before crashing. It must therefore be some way that the algorithm itself interacts with nearly sorted and sorted lists. Also, the generation for reversed lists involves invoking the code for creating a sorted list (and then reversing it).

Also, I find it unlikely that the issue is just to the algorithm having to, for some reason, invoke the partitioning off of sections of the array by recursion in a nearly sorted and sorted list significantly more times rather than the other list types, as it can sort random lists with 10^7 values, which would require far more partitioning than would a nearly sorted list with 10^5 values.

I realise it must have something to do with how these list types interact with the recursion of my quick sort, but if someone could shed light on it that would be great. I've put the code to both the quick sort algorithm in full and the random list generators below.

QUICK SORT ALGORITHM

/**
 * Performs a quick sort given the indexes of the bounds of an integer array
 * @param arr The array to be sorted
 * @param highE The index of the upper element
 * @param lowE The index of the lower element
 */
public static void quickSort(int[] arr, int highE, int lowE)
{       
    //Only perform an action if arr.length > 30, otherwise insertion sort [recursive base case])
    if (lowE + 29 < highE)
    {
        //Get the element and then value of the pivot
        int pivotE = findPivot(arr, highE, lowE);
        int pivotVal = arr[pivotE], storeE = lowE;

        //Swap the pivot and the last value.
        swapElements(arr, pivotE, highE);

        //For each element in the selection that is not the pivot, check to see if it is lower than the pivot and if so, move it to the leftmost untouched element.
        for (int i = lowE; i < highE; i++)
        {
            if (arr[i] < pivotVal)
            {
                swapElements(arr, storeE, i);

                //Increment storeE so that the element that is being switched moves to the right location
                storeE++;
            }
        }

        //Finally swap the pivot into its proper position and recrusively call quickSort on the lesser and greater portions of the array
        swapElements(arr, storeE, highE);                   
        //Lesser
        quickSort(arr, storeE - 1, lowE);
        //Greater
        quickSort(arr, highE, storeE + 1);
    }
    else
    {
        insertSort(arr, highE, lowE);
    }
}




/**
 * Finds the pivot element
 * @param arr The array to be sorted
 * @param highE The index of the top element
 * @param lowE The index of the bottom element
 * @return The index of the pivot.
 */
public static int findPivot(int[] arr, int highE, int lowE)
{
    //Finds the middle element
    int mid = (int) Math.floor(lowE + (highE - lowE) / 2);

    //Returns the value of the median of the first, middle and last elements in the array.
    if ((arr[lowE] >= arr[mid]) && (arr[lowE] >= arr[highE])) 
    {
        if (arr[mid] > arr[highE]) {return mid;}
        else {return highE;}
    }
    else if ((arr[mid] >= arr[lowE]) && (arr[mid] >= arr[highE])) 
    {
        if (arr[lowE] > arr[highE]) {return lowE;}
        else {return highE;}
    }
    else 
    {
        if (arr[lowE] > arr[mid]) {return lowE;}
    }

    return mid;
}




/**
 *Performs an insertion sort on part of an array
 * @param arr The array to be sorted.
 * @param highE The index of the top element.
 * @param lowE The index of the low element.
 */
public static void insertSort(int[] arr, int highE, int lowE)
{
    //Sorts elements lowE to i in array, with i being gradually incremented.
    for (int i = lowE + 1; i <= highE; i++)
    {
        for (int j = i; j > lowE; j--)
        {
            if (arr[j] < arr[j - 1])
            {
                swapElements(arr, j, j-1);
            }
            else {break;}
        }
    }
}

RANDOM LIST GENERATORS

/**
 * Creates a random list
 * @param arr The array to place the list inside of
 */
public static void randomList(int[] arr)
{
    //Places a random number at each element of the array

    for (int i = 0; i < arr.length; i++)
    {
        arr[i] = (int) Math.floor(Math.random() * RAND_MAX);
    }
}




/**
 * Creates a nearly sorted list of random numbers
 * @param arr the array to place the list inside of
 */
public static void nearSortList(int[] arr)
{
    //Creates a sorted list in arr
    sortList(arr);



    int swaps = (int) (Math.ceil(Math.pow((Math.log(arr.length)), 2.0)));

    //The two values to be switched each time
    int a, b;

    //Performs a number of swaps equal to swaps [log(N) ^ 2] rounded up, with numbers switched no more than ln(N) places away
    for (int i = 0; i < swaps; i++)
    {
        a = (int) Math.floor(Math.random() * arr.length);

        b = (int) (a + Math.random() * 2 * Math.log(arr.length) - Math.log(arr.length));

        //Accounts for cases in which b is either greater or smaller than the array bounds
        if (b < 0)
        {
            b = -b;
        }
        else if (b >= arr.length)
        {
            b = -1 * (arr.length - b);
        }

        swapElements(arr, a, b);
    }
}




/**
 * Creates a random list with many unique values in
 * @param arr the array to place the list inside of
 */
public static void fewUniqueList(int[] arr)
{
    int[] smallArr = new int[(int) Math.floor(Math.pow(arr.length, 9.0 / 10.0))];


    //Creates a smaller array of random values
    randomList(smallArr);



    //Fills the larger list up with values from the smaller list, ensuring aproximately N / N ^ (9/10) instances of each number in the array and ensuring, at most, there are N ^ (9/10) (rounded down) unique values in the large array
    for (int i = 0; i < arr.length; i++)
    {
        arr[i] = smallArr[(int) Math.floor(Math.random() * smallArr.length)];
    }
}




/**
 * Creates a reversed list of random numbers
 * @param arr the array to place the list inside of
 */
public static void reversedList(int[] arr)
{
    //Creates a sorted list in arr
    sortList(arr);




    //Switches each ith elements with its mirror on the other end of the array until the value of i reaches the middle of the array
    for (int i = 0; i < (int) (arr.length / 2.0); i++)
    {
        swapElements(arr, i, arr.length - 1 - i);
    }
}




/**
 * Creates a sorted list of random numbers using a merge sort
 * @param arr the array to place the list inside of
 */
public static void sortList(int[] arr)
{
    //Creates a random list in arr
    randomList(arr);

    Arrays.sort(arr);
}

EDIT: [Defunct]

EDIT 2:

I've replaced the basic recursive calls with the following code in order to only call the smallest of the two partitions at EJPs recommendation and it still is not fixing the issue.

if (storeE - 1 - lowE < highE - storeE + 1)
{
    //Lesser
    quickSort(arr, storeE - 1, lowE);
    //Greater
    quickSort(arr, highE, storeE + 1);
}
else
{
    //Greater
    quickSort(arr, highE, storeE + 1);
    //Lesser
    quickSort(arr, storeE - 1, lowE);
}

EDIT 3:

Ok, it is now evident that the recursion depth is far greater than I gave it credit for for nearly sorted and sorted lists. But now I need to work out why this is the case, and why random lists only had a depth of 28 for 10^7 values but nearly sorted and sorted lists have depths of over 3000

  • Can you add a counter to check the max recursion depth you reach? It seems highly unlikely that the depth is too big, given the way you choose the pivot, indeed, but worth a check ... – chill Nov 27 '13 at 22:38
  • I'm late to the party, but Median-of-Three pivot selection is really effective at eliminating this all too common problem. – Jeremy West Feb 26 '14 at 05:41

5 Answers5

4

Don Knuth in [ACP][1] suggests always pushing the larger of the two partitions and sorting the smaller one immediately, to limit stack growth. In your code that corresponds to recursively sorting the smaller of the two partitions first, then the other one.

[1]: The Art of Computer Programming, vol III, #5.2.2 p.114.

user207421
  • 305,947
  • 44
  • 307
  • 483
  • Ok, I'll give implementing it a whirl. – Zach Beavon-Collin Nov 27 '13 at 22:58
  • In that case you have a bigger problem, as this will have made an enormous difference. Knuth also recommends a *single* insertion sort at the end, not one per partition. – user207421 Nov 27 '13 at 23:22
  • I've implemented it but it is still not helping. Would it reduce the stack any more than by 1, given that the insertSort method doesn't invoke any other methods? – Zach Beavon-Collin Nov 27 '13 at 23:32
  • And I don't get this `storeE` business. The elements that are the wrong side of the pivot should be swapped with each other. Your central loop isn't a quicksort as I know it. There should be two of them, one going up until a too-high element is found, the other going down until a too-low element is found, then those are swapped and the algorithm continues. – user207421 Nov 27 '13 at 23:41
  • 2
    Doesn't this only help if doing tail-recursion optimization, which Java doesn't do? – Bernhard Barker Nov 27 '13 at 23:43
  • 2
    I'm pretty sure that the idea behind this is to use tail recursion on the larger partition. Java does not optimize tail recursive calls out of the call stack, so this won't help. – Floegipoky Nov 27 '13 at 23:45
  • Starting from the beginning of the partition, it checks each element to see whether or not it is smaller than the pivot. If it is, it is moved to the front of the partition. storeE is there to store the new position of the next element in the partition smaller than the pivot. It's doing the same thing, just in a slightly different way to save space – Zach Beavon-Collin Nov 27 '13 at 23:45
  • @ZachBeavon-Collin It's not the same. You're moving everything < the pivot to the beginning of the partition regardless of whether it is already in the correct partition or not. So you're doing many times as many exchanges as necessary. And you're doing nothing with the elements in the left partition that are > the pivot. – user207421 Nov 27 '13 at 23:56
  • @Floegipoky Tail recursion has nothing to do with it. If you always sort the smaller partition first you don't recurse as deep. – user207421 Nov 28 '13 at 00:00
  • 2
    @EJP Recursing on the smaller partition and recursing on the larger partition are completely independent of each other - sorting the smaller one first doesn't result in less work sorting the larger one. The order only matters if you're doing tail-recursion optimization. Unless I'm missing something. – Bernhard Barker Nov 28 '13 at 00:03
  • 3
    @EJP I'm sorry but that's not correct. With tail recursion elimination, you can guarantee that the stack size will not exceed O(logn). Without tail recursion elimination, your call tree will be exactly the same maximum depth O(n). It will just look like an inverted verizon symbol. – Floegipoky Nov 28 '13 at 00:18
  • But note you can remove tail recursion by hand even if the compiler won't. If there is interest I can show how this is done. – Gene Nov 28 '13 at 00:25
  • @Gene Do you mean directly utilizing iteration in the code, or performing some sort of black magic trickery with the JVM? Because I'm set on the first, but I'd love to see the second if it's a thing =) – Floegipoky Nov 28 '13 at 00:43
  • @Floegipoky The simplest way is to convert the tail call to re-assignment of the new argument values followed by a `goto` a label at the method head. This is just what a compiler does. Of course since Java doesn't have `goto` you must get the same effect with a loop. – Gene Nov 28 '13 at 01:18
  • @Gene "you can remove tail recursion by hand" - yeah, see [this answer](http://stackoverflow.com/a/20255628/1711796). – Bernhard Barker Nov 28 '13 at 02:04
  • @Dukeling Yes, that's one example. My intent was to show how it can be done every time. – Gene Nov 28 '13 at 03:19
4

For a random array, you could partition off massive chunks of the data.
But for a (nearly) sorted array, you'd mostly be partitioning off 1 element at a time.

So, for a sorted array, your stack size would end up being the same as the size of the array, while, for a random array, it's much more likely to be about a logarithm of that size.

So, even if the random array is much larger than a nearly sorted one, it's not surprising that the smaller one throws an exception, but the larger one doesn't.

Modifying your code

In terms of a fix, as EJP pointed out, you should do the smaller partition first to limit stack growth. But this in itself won't fix the problem as Java doesn't support tail-call optimization (well, it's optional for an implementation, as I understand that question).

A fairly simple fix here is to throw your function into a while-loop, essentially hard-coding the tail-call optimization.

To give a better idea of what I mean:

public static void quickSort(int[] arr, int highE, int lowE)
{
    while (true)
    {
        if (lowE + 29 < highE)
        {
            ...
            quickSort(arr, storeE - 1, lowE);

            // not doing this any more
            //quickSort(arr, highE, storeE + 1);

            // instead, simply set the parameters to their new values
            // highE = highE;
            lowE = storeE + 1;
        }
        else
        {
            insertSort(arr, highE, lowE);
            return;
        }
    }
}

Well, now that you have the basic idea, this would look better (functionally equivalent to the above, just more concise):

public static void quickSort(int[] arr, int highE, int lowE)
{
    while (lowE + 29 < highE)
    {
        ...
        quickSort(arr, storeE - 1, lowE);
        lowE = storeE + 1;
    }
    insertSort(arr, highE, lowE);
}

This of course doesn't actually do the smaller one first, but I'll leave that to you to figure out (seems you already have a fair idea of how to do this).

How this works

For some made up values...

Your current code does this: (an indent indicates what happens inside that function call - thus increasing indentation means recursion)

quickSort(arr, 100, 0)
   quickSort(arr, 49, 0)
      quickSort(arr, 24, 0)
         insertion sort
      quickSort(arr, 49, 26)
         insertion sort
   quickSort(arr, 100, 51)
      quickSort(arr, 76, 0)
         insertion sort
      quickSort(arr, 100, 74)
         insertion sort

The modified code does this:

quickSort(arr, 100, 0)
   quickSort(arr, 49, 0)
      quickSort(arr, 24, 0)
         break out of the while loop
         insertion sort
   lowE = 26
   break out of the while loop
      insertion sort
lowE = 51
run another iteration of the while-loop
    quickSort(arr, 76, 0)
      break out of the while loop
      insertion sort
lowE = 74
break out of the while loop
   insertion sort

Increase the stack size

Not sure whether you've considered this, or whether it would work with your parameters, but you can always consider simply increasing the stack size with the -Xss command-line parameter.

Community
  • 1
  • 1
Bernhard Barker
  • 54,589
  • 14
  • 104
  • 138
  • Won't that just create an infinite loop? – Zach Beavon-Collin Nov 28 '13 at 00:35
  • It shouldn't - you're changing `lowE` at every step (and returning in the `else`). This should give identical results to the recursive version (except without the exception). – Bernhard Barker Nov 28 '13 at 00:37
  • Except lowE isn't a static variable. They'll remain the same in the first call of the function which will keep looping. Also, just to clarify, I'm referring to the better version – Zach Beavon-Collin Nov 28 '13 at 00:41
  • The first call here is exactly the same as the first call in your code - if you see that the function otherwise works, it should make sense that it doesn't create an infinite loop (because that call is just calling this function again). Note that there's very little difference between the two versions I posted - just check what happens when the if-statement condition versus the while-loop condition is true. I didn't just want to post the second version, because it seems more difficult to understand how I got there. I edited my post to show what happens - I hope that helps to explain it better. – Bernhard Barker Nov 28 '13 at 01:02
1

StackOverflowError is most likely related to too deep recursion. With more elements to sort your quicksort must do more recursive calls to quicksort() before entering the insertion sort part. At some point this recursion is too deep and there are too many method calls on the stack.

It might be that recursion on already sorted lists lead to deeper recursion and therefore crashing earlier with less elements than sorting an unsorted list. This depends on implementation.

For non-academic and non-learning purposes it is always preferable to implement these algs with imperative style instead of using recursion.

Fabian Barney
  • 14,219
  • 5
  • 40
  • 60
  • The problem is that I don't understand why this would occur given that, after I implemented a counter at chill's recommendation, sorting a 10^7 list with random numbers is implementing the quick sort recursion 100s of thousands of times. It doesn't make sense why it would be too much recursion when other list types are using far more recursion and going much deeper and not crashing the exact same code. – Zach Beavon-Collin Nov 27 '13 at 23:01
  • 1
    It does not matter how often it is called but it does matter what maximum recursion depth you reach. It's the depth which hurts not how often recursive calls happen. A counter how often it was called is pointless. You need a depth counter. And here you're interested in the maximum depth. – Fabian Barney Nov 27 '13 at 23:06
  • Ok, point taken. And it would seem that you are most definitely right. It was going 28 deep on a random list for 10^7 values but up to 3000 deep for nearly sorted. Now I just need to work out why. – Zach Beavon-Collin Nov 27 '13 at 23:16
  • This means that it degenerates for some reason. quicksort performs best when lesser and greater parts are equal in size having a perfect pivot. Worst case is having a bad pivot element and all other elements are less or all are greater than pivot. I would debug the code with a conditional breakpoint before recursive calls to quicksort and one of the greater/lesser parts having much more elemtns than the other. Then have a look at the values in the relevant part of the array. – Fabian Barney Nov 27 '13 at 23:49
0

Check if you have long runs of identical elements. The partition part:

for (int i = lowE; i < highE; i++)
{
    if (arr[i] < pivotVal)
    {
        swapElements(arr, storeE, i);
        storeE++;
    }
}

partitions a list containing the same elements in the worst possible way.

chill
  • 16,470
  • 2
  • 40
  • 44
  • I can take a look. The only issue is that I am already testing random lists with several identical values (see fewUnique) and whilst I can optimise the algorithm further, it certainly isn't going as deep into recursion for it than for the nearlySorted, which doesn't necessarily have any identical values – Zach Beavon-Collin Nov 27 '13 at 23:35
0

With sorted or nearly sorted data set, QuickSort exibits worst case running time of O(n^2). With large value of N, recursion tree goes so deep such that system stack exhaust to spawn further recursions. Generally such algorithms should be implemented with iterative approach instead of recursive approach.

Vallabh Patade
  • 4,960
  • 6
  • 31
  • 40
  • Yes, but as I've said in my post, I need to work out why it goes as deep as it does for nearly sorted and sorted lists and not nearly as deep for other list types, such as random lists – Zach Beavon-Collin Nov 27 '13 at 23:56
  • Because in a sorted list you take the first or last element and the rest ends up on one side, so stack grows by 1 for each element in the list. Divide and conquer doesn't work. On random lists the average is 50/50, 50% on smaller side 50% on larger side, so the stack grows at log n, not n. – Roman Rabinovich Aug 25 '18 at 15:57