Pig
  1. Pig
  2. PIG-170

Memory manager spills bags in the wrong order

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.1.0
    • Component/s: None
    • Labels:
      None

      Description

      For optimal performance, we want to spill the largest bags first. This is not what is happening right now and could be causing some of our memory issues.

      1. PIG-170_0_20080327.patch
        1 kB
        Amir Youssefi
      2. compareMemUsage.gif
        76 kB
        Pi Song

        Activity

        Hide
        Amir Youssefi added a comment - - edited

        Ben and I had a closer look at spilling and identified this issue but assessing the impact of this patch is not complete yet. I am conducting a few long-running tests in comming days. Attaching patch per request.

        Show
        Amir Youssefi added a comment - - edited Ben and I had a closer look at spilling and identified this issue but assessing the impact of this patch is not complete yet. I am conducting a few long-running tests in comming days. Attaching patch per request.
        Hide
        Alan Gates added a comment -

        +1

        Show
        Alan Gates added a comment - +1
        Hide
        Pi Song added a comment -

        In certain scenarios, for example, a lot of small intermediate bags are used to calculate something and outputs are stored in a big bag. The big bag will become the first node after

        Collections.sort(spillables, new Comparator<WeakReference<Spillable>>()
        

        and the below code will no longer work.

            public void registerSpillable(Spillable s) {
                synchronized(spillables) {
                    // Cleaing the entire list is too expensive.  Just trim off the front while
                    // we can.
                    WeakReference<Spillable> first = spillables.peek();
                    while (first != null && first.get() == null) {
                        spillables.remove();
                        first = spillables.peek();
                    }
                    spillables.add(new WeakReference<Spillable>(s));
                }
            }
        

        I

        Show
        Pi Song added a comment - In certain scenarios, for example, a lot of small intermediate bags are used to calculate something and outputs are stored in a big bag. The big bag will become the first node after Collections.sort(spillables, new Comparator<WeakReference<Spillable>>() and the below code will no longer work. public void registerSpillable(Spillable s) { synchronized(spillables) { // Cleaing the entire list is too expensive. Just trim off the front while // we can. WeakReference<Spillable> first = spillables.peek(); while (first != null && first.get() == null) { spillables.remove(); first = spillables.peek(); } spillables.add(new WeakReference<Spillable>(s)); } } I
        Hide
        Pi Song added a comment -

        By profiling memory usage, there is no significant difference. My system might be too small.

        A gap between horizonal grid lines = 50MB
        Input size = 540MB
        ChildTask process Max Heap = 200MB (Only 1 process)

        a1 = group (load 'file:/tmp/bigfile1') ALL
        store a1 into '/tmp/out3' USING BinStorage() ;

        Show
        Pi Song added a comment - By profiling memory usage, there is no significant difference. My system might be too small. A gap between horizonal grid lines = 50MB Input size = 540MB ChildTask process Max Heap = 200MB (Only 1 process) a1 = group (load 'file:/tmp/bigfile1') ALL store a1 into '/tmp/out3' USING BinStorage() ;
        Hide
        Alan Gates added a comment -

        Pi is correct that there are scenarios where a big bag could be sorted ahead of smaller bags that will empty faster. But to get into this circumstance, a very specific set of conditions have to occur:

        1) many small bags are created
        2) small bags are moved into large bag, without yet being released
        3) spill happens, forcing a sort of the linked list
        4) small bags go out of scope

        This set of events seems fairly rare. And even when it does occur, the worst that happens is we are not as aggressive as we could be about cleaning the list. In the very worst case it will cause an early spill.

        We cannot clean the entire list on every register call, as that is far too expensive (I tried it, it slowed performance by an order of magnitude on large scripts). We want to spill large bags first so that we spill as few bags as possible. We could change the code to copy the list and sort that copy, thus avoiding reordering the existing list. However, once we're in the spill code, we are in a low memory situation. Copying a potentially large list to sort it is a bad idea in that case. So I don't see a better solution.

        Show
        Alan Gates added a comment - Pi is correct that there are scenarios where a big bag could be sorted ahead of smaller bags that will empty faster. But to get into this circumstance, a very specific set of conditions have to occur: 1) many small bags are created 2) small bags are moved into large bag, without yet being released 3) spill happens, forcing a sort of the linked list 4) small bags go out of scope This set of events seems fairly rare. And even when it does occur, the worst that happens is we are not as aggressive as we could be about cleaning the list. In the very worst case it will cause an early spill. We cannot clean the entire list on every register call, as that is far too expensive (I tried it, it slowed performance by an order of magnitude on large scripts). We want to spill large bags first so that we spill as few bags as possible. We could change the code to copy the list and sort that copy, thus avoiding reordering the existing list. However, once we're in the spill code, we are in a low memory situation. Copying a potentially large list to sort it is a bad idea in that case. So I don't see a better solution.
        Hide
        Olga Natkovich added a comment -

        Given, what alan described I feel that we should commit the patch since it will improve common case. Objections?

        Show
        Olga Natkovich added a comment - Given, what alan described I feel that we should commit the patch since it will improve common case. Objections?
        Hide
        Benjamin Reed added a comment -

        +1 I agree it should be committed. I should also point out that this doesn't invalidate the cleanup in the registerSpillable. That code will still clean up weak references of bags that have been freed since the last GC, which is exactly what it is supposed to do. We do a global cleanup after a GC when our threshold callback is invoked.

        Show
        Benjamin Reed added a comment - +1 I agree it should be committed. I should also point out that this doesn't invalidate the cleanup in the registerSpillable. That code will still clean up weak references of bags that have been freed since the last GC, which is exactly what it is supposed to do. We do a global cleanup after a GC when our threshold callback is invoked.
        Hide
        Olga Natkovich added a comment -

        ok, will commit it shortly

        Show
        Olga Natkovich added a comment - ok, will commit it shortly
        Hide
        Pi Song added a comment -

        +1, no objection.

        Show
        Pi Song added a comment - +1, no objection.
        Hide
        Amir Youssefi added a comment -

        Initial results of tests showed that application of PIG-170 makes a job fail (it was not failing before application).

        Show
        Amir Youssefi added a comment - Initial results of tests showed that application of PIG-170 makes a job fail (it was not failing before application).
        Hide
        Amir Youssefi added a comment -

        Now I am investigating whether this failure is directly related to PIG-170 or it's cluster-wide issue happening because we spill more data to Disks.

        Show
        Amir Youssefi added a comment - Now I am investigating whether this failure is directly related to PIG-170 or it's cluster-wide issue happening because we spill more data to Disks.
        Hide
        Olga Natkovich added a comment -

        patch committed. Thanks Amir for contributing!

        Show
        Olga Natkovich added a comment - patch committed. Thanks Amir for contributing!
        Hide
        Olga Natkovich added a comment -

        Amir, I did not see your comments and I committed the patch. Please, update your bug with the results of the investigation so that we can decide whether to roll it back.

        Alan, please, help amir to figure out the disk full issue, thanks.

        Show
        Olga Natkovich added a comment - Amir, I did not see your comments and I committed the patch. Please, update your bug with the results of the investigation so that we can decide whether to roll it back. Alan, please, help amir to figure out the disk full issue, thanks.

          People

          • Assignee:
            Amir Youssefi
            Reporter:
            Olga Natkovich
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development