Uploaded image for project: 'Kuali Rice Development'
  1. Kuali Rice Development
  2. KULRICE-4070

XmlHelper.appendXml(Node, String) can be really slow. (i.e. takes minutes)

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.0.1.1
    • Fix Version/s: KC Release 2.0, 1.0.2
    • Component/s: Development
    • Labels:
      None
    • Rice Module:
      KEW

      Description

      Our local KC instance was taking a very long time (e.g. > 12 minutes) to save a document. Using a profiler, we determined that appendXml runs for a very long time. Invocations of that method vary widely in performance, but in one instance, appending about 1MB of XML took 415 seconds (~7 minutes).

      The original method uses XSLT to append the XML. I rewrote the method to use pure DOM to append the XML. After doing so, operations that used to take mintues now take seconds. Here is the rewritten method:

      public static void appendXml(Node parentNode, String xml) throws TransformerException {
      DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
      factory.setValidating(false);
      try

      { org.w3c.dom.Document xmlDocument = factory.newDocumentBuilder().parse(new InputSource(new StringReader(xml))); org.w3c.dom.Element xmlDocumentElement = xmlDocument.getDocumentElement(); Node importedNode = parentNode.getOwnerDocument().importNode(xmlDocumentElement, true); parentNode.appendChild(importedNode); }

      catch (Exception e)

      { LOG.error("Error when appending XML", e); throw new RuntimeException("Error when appending XML", e); }

      }

        Attachments

          Issue Links

            Activity

            Hide
            wliang Warren Liang added a comment -

            I just took the simplest route to get the rewritten method to work under the simplest test case. Feel free to get rid of the try/catch/throw RuntimeExcetpion, but it'll require more changes in code that call that method.

            Show
            wliang Warren Liang added a comment - I just took the simplest route to get the rewritten method to work under the simplest test case. Feel free to get rid of the try/catch/throw RuntimeExcetpion, but it'll require more changes in code that call that method.
            Hide
            g1zhang Ge Zhang (Inactive) added a comment -

            Warren:
            I guess you will like this: replacing throws Exception with throws IOException, SAXException, ParserConfigurationException, InvalidXmlException, actually the test case is waiting for a WorkflowException which is super class of InvalidXmlException.

            It passed the test cases, please let me know if we need finer tuning on it, then I will check in the change.

            Show
            g1zhang Ge Zhang (Inactive) added a comment - Warren: I guess you will like this: replacing throws Exception with throws IOException, SAXException, ParserConfigurationException, InvalidXmlException, actually the test case is waiting for a WorkflowException which is super class of InvalidXmlException. It passed the test cases, please let me know if we need finer tuning on it, then I will check in the change.
            Hide
            g1zhang Ge Zhang (Inactive) added a comment -

            finer Exception handling only throws IOException, SAXException, ParserConfigurationException which are the possible exceptions could be thrown in the method,, avoid throwing RuntimeException, so that DTOConverter.handleException can wrap it into WorkflowException and catchable by other callers.

            Show
            g1zhang Ge Zhang (Inactive) added a comment - finer Exception handling only throws IOException, SAXException, ParserConfigurationException which are the possible exceptions could be thrown in the method,, avoid throwing RuntimeException, so that DTOConverter.handleException can wrap it into WorkflowException and catchable by other callers.
            Hide
            wliang Warren Liang added a comment -

            Hey Ge,

            I think as long as it throws any exception it'll be fine. When I rewrote the method, I made it throw a RuntimeException because I did not want to change a lot of rice code to catch other exceptions. Do whatever you feel is best regarding exceptions.

            Show
            wliang Warren Liang added a comment - Hey Ge, I think as long as it throws any exception it'll be fine. When I rewrote the method, I made it throw a RuntimeException because I did not want to change a lot of rice code to catch other exceptions. Do whatever you feel is best regarding exceptions.
            Hide
            g1zhang Ge Zhang (Inactive) added a comment -

            Right, throwing any checked exception is fine, because org.kuali.rice.kew.dto.DTOConverter.handleException() will wrap all checked exception into WorkflowException, and unchecked exception into RuntimrException in the immediate caller of this method.

            Making it throwing finer checked exceptions is for making a better interface in case some other methods really need to check them in the future.

            The change has been committed and passed the tests

            Thanks

            Show
            g1zhang Ge Zhang (Inactive) added a comment - Right, throwing any checked exception is fine, because org.kuali.rice.kew.dto.DTOConverter.handleException() will wrap all checked exception into WorkflowException, and unchecked exception into RuntimrException in the immediate caller of this method. Making it throwing finer checked exceptions is for making a better interface in case some other methods really need to check them in the future. The change has been committed and passed the tests Thanks

              People

              • Assignee:
                g1zhang Ge Zhang (Inactive)
                Reporter:
                wliang Warren Liang
              • Votes:
                1 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: