Oozie
  1. Oozie
  2. OOZIE-1186

Image load for Job DAG visualization should handle resources better

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: trunk, 3.3.1
    • Fix Version/s: 3.3.2
    • Component/s: None
    • Labels:
      None

      Description

      The Job DAG visualization loads an image into memory to be streamed on outputstream. However, it does not free up memory and I/O resources leading to 'Out of Java heap space' errors.

      1. HeapMemoryWithPatchedCode.png
        27 kB
        Mona Chitnis
      2. HeapMemoryExistingTrunkCode.png
        24 kB
        Mona Chitnis
      3. OOZIE-1186v2.patch
        5 kB
        Mona Chitnis

        Issue Links

          Activity

          Hide
          Mona Chitnis added a comment -

          WIP patch to free up memory resources used during image generation, and i/o resources in servlet.

          Conducted memory profiling of Oozie server JVM run with and without the patch and saw the difference.
          Need to think about unit tests for the same.

          Show
          Mona Chitnis added a comment - WIP patch to free up memory resources used during image generation, and i/o resources in servlet. Conducted memory profiling of Oozie server JVM run with and without the patch and saw the difference. Need to think about unit tests for the same.
          Hide
          Rohini Palaniswamy added a comment -

          Do not close response.getOutputStream();. Tomcat takes care of that.

          Show
          Rohini Palaniswamy added a comment - Do not close response.getOutputStream();. Tomcat takes care of that.
          Hide
          Virag Kothari added a comment -

          From the java docs, dispose() is not required when paint() method of component is used (http://docs.oracle.com/javase/6/docs/api/java/awt/Graphics.html#dispose())
          From the below code, it seems like construction of BufferedImage can occupy lot of memory if d.width and d.height are huge as each pixel is an int.

           BufferedImage img = new BufferedImage(d.width, d.height, BufferedImage.TYPE_INT_RGB);
          

          I dont think img.flush() will free this databuffer holding the pixel data. (http://docs.oracle.com/javase/6/docs/api/java/awt/Image.html#flush())
          So, I am not sure which portion of the patch is actually releasing the resources and making a difference in memory consumption.
          It seems creation of BufferedImage itself can lead to OOM if multiple servlets are creating this image simultaneously.

          Show
          Virag Kothari added a comment - From the java docs, dispose() is not required when paint() method of component is used ( http://docs.oracle.com/javase/6/docs/api/java/awt/Graphics.html#dispose( )) From the below code, it seems like construction of BufferedImage can occupy lot of memory if d.width and d.height are huge as each pixel is an int. BufferedImage img = new BufferedImage(d.width, d.height, BufferedImage.TYPE_INT_RGB); I dont think img.flush() will free this databuffer holding the pixel data. ( http://docs.oracle.com/javase/6/docs/api/java/awt/Image.html#flush( )) So, I am not sure which portion of the patch is actually releasing the resources and making a difference in memory consumption. It seems creation of BufferedImage itself can lead to OOM if multiple servlets are creating this image simultaneously.
          Hide
          Virag Kothari added a comment -

          Comment not related to the patch. Saw this while looking at the code

           public void finalize() {
                  // No-op; just to avoid finalizer attack
                  // as the constructor is throwing an exception
              }
          

          To avoid the finalizer attack, the method should be final. As its a one-line change, it would be good to have this added as part of the patch.

          Show
          Virag Kothari added a comment - Comment not related to the patch. Saw this while looking at the code public void finalize() { // No-op; just to avoid finalizer attack // as the constructor is throwing an exception } To avoid the finalizer attack, the method should be final. As its a one-line change, it would be good to have this added as part of the patch.
          Hide
          Mona Chitnis added a comment -

          Uploaded patch after review comments - https://reviews.apache.org/r/9079/

          Show
          Mona Chitnis added a comment - Uploaded patch after review comments - https://reviews.apache.org/r/9079/
          Hide
          Mona Chitnis added a comment -

          See attachments for snapshots from the memory profiler, with a WF job (DAG image of about 1MB size) run with and without the patch. I hit reload on the 'show=graph' command 5 times for both cases to see how multiple servlet responses are handled.

          Basic observation is heap memory occupied goes on increasing without patch, whereas stays lower and constant with patch.

          I am still digging further into any lingering references to the BufferedImage object that are not released after every image load. The javadoc for img.flush() or Graphics2D.dispose seems to indicate no obvious relinquish in our case.

          Show
          Mona Chitnis added a comment - See attachments for snapshots from the memory profiler, with a WF job (DAG image of about 1MB size) run with and without the patch. I hit reload on the 'show=graph' command 5 times for both cases to see how multiple servlet responses are handled. Basic observation is heap memory occupied goes on increasing without patch, whereas stays lower and constant with patch. I am still digging further into any lingering references to the BufferedImage object that are not released after every image load. The javadoc for img.flush() or Graphics2D.dispose seems to indicate no obvious relinquish in our case.
          Hide
          Virag Kothari added a comment -

          I did some testing to figure out the effect of flush()
          I created a bufferedimage with width and height of 3000*3000. As RGB is used, each pixel occupies a 'int'. So the buffered image instance occupies around (3000*3000*4/(1024*1024) MB. I then tried with and without img.flush() but no difference in memory consumption to make any conclusion. Basically, only the garbage collector will reclaim the databuffer when the img instance is out of scope.
          So, ideally we should be creating a image which can be written serially to disk (row by row) instead of bringing it entirely in memory. But that would be non-trivial with the graph library being used. So, short term fix would be increase the tomcat memory if OOM's are seen.
          But, it would be good to have flush() which will flush any system resources used for rendering the image. dispose() is not required as its already called by the component's paintAll() method.

          Show
          Virag Kothari added a comment - I did some testing to figure out the effect of flush() I created a bufferedimage with width and height of 3000*3000. As RGB is used, each pixel occupies a 'int'. So the buffered image instance occupies around (3000*3000*4/(1024*1024) MB. I then tried with and without img.flush() but no difference in memory consumption to make any conclusion. Basically, only the garbage collector will reclaim the databuffer when the img instance is out of scope. So, ideally we should be creating a image which can be written serially to disk (row by row) instead of bringing it entirely in memory. But that would be non-trivial with the graph library being used. So, short term fix would be increase the tomcat memory if OOM's are seen. But, it would be good to have flush() which will flush any system resources used for rendering the image. dispose() is not required as its already called by the component's paintAll() method.
          Hide
          Hadoop QA added a comment -

          Testing JIRA OOZIE-1186

          Cleaning local svn workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 1 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          +1 TESTS
          . Tests run: 947
          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          +1 Overall result, good!, no -1s

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/296/

          Show
          Hadoop QA added a comment - Testing JIRA OOZIE-1186 Cleaning local svn workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files +1 TESTS . Tests run: 947 +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- +1 Overall result, good!, no -1s The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/296/
          Hide
          Mona Chitnis added a comment -

          Thanks for review. Committed to branch-3.3, trunk and hcat-intre

          Show
          Mona Chitnis added a comment - Thanks for review. Committed to branch-3.3, trunk and hcat-intre
          Hide
          Robert Kanter added a comment -

          Closing issue; Oozie 3.3.2 is released

          Show
          Robert Kanter added a comment - Closing issue; Oozie 3.3.2 is released

            People

            • Assignee:
              Mona Chitnis
              Reporter:
              Mona Chitnis
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development