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 Improvement
    • Status: Closed Closed
    • Priority: Critical 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
    • Similar issues:
      KULRICE-12988KC app startup time really slow with the latest rice 2.5 revision
      KULRICE-9583LightTables are still slow in IE
      KULRICE-5548KRAD: IE is extremely slow to load pages
      KULRICE-13070Pages load really slow after rice upgrade
      KULRICE-3313Look into why unit test runs are taking ~40 minutes longer now. Which changes were impacting?
      KULRICE-7962IE script taking too long
      KULRICE-8837CM: Adding groups takes over 10 minutes
      KULRICE-4037KFS user preference records cause batch slowness due to too many krew_usr_optn_t records in Rice 1.0.1.1
      KULRICE-9915Add line client Javascript runs for over a minute
      KULRICE-10050DatePicker widgets can take a lot of time to create client-side as more are used on the page, ie collections
    • 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); }

      }

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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:
              Ge Zhang (Inactive)
              Reporter:
              Warren Liang
            • Votes:
              1 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Structure Helper Panel