Pluto
  1. Pluto
  2. PLUTO-601

Remove expensive object creation in PortletAppDescriptorServiceImpl

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.3, 2.1.0-M1, 2.1.0
    • Component/s: portlet container
    • Labels:
      None

      Description

      PortletAppDescriptorServiceImpl creates new JAXBContext, XMLInputFactory, DocumentBuilderFactory, and Pattern objects every time they are needed. All of these objects are thread-safe once created and configured and should only be created once at the class level and re-used for the life of the class.

        Activity

        Hide
        Ate Douma added a comment -

        Hi Eric,

        I'm sorry but I have a problem with (some of) these changes.
        First of all both XMLInputFactory and DocumentBuilderFactory are not (guaranteed) thread safe!
        While some implementations do claim to be thread safe, that is not actually required.
        Stax for instance claims to provide a thread safe XMLInputFactory, but not for the configuration, e.g. when using setProperty() to influence its behavior is not thread safe.
        The same seems to hold for DocumentBuilderFactory, hence you can find plenty of advises on the internet to create new instances per usage to ensure stable behaviour.
        The JAXBContext (and Pattern) however is supposed to be thread safe, so I'm fine with the improvements on those.
        Note: I haven't yet validated these changes against Portlet TCK 2.0 or current Jetspeed Portal.

        Another issue is with regard to the creation of the XMLInputFactory instance: that should always be done against the container ClassLoader, e.g. ensure the Thread ContextClassloader is (temporarily) forced.
        See also the comment I provided on the original getXMLInputFactory(ClassLoader) method.

        Regards,

        Ate

        Show
        Ate Douma added a comment - Hi Eric, I'm sorry but I have a problem with (some of) these changes. First of all both XMLInputFactory and DocumentBuilderFactory are not (guaranteed) thread safe! While some implementations do claim to be thread safe, that is not actually required. Stax for instance claims to provide a thread safe XMLInputFactory, but not for the configuration, e.g. when using setProperty() to influence its behavior is not thread safe. The same seems to hold for DocumentBuilderFactory, hence you can find plenty of advises on the internet to create new instances per usage to ensure stable behaviour. The JAXBContext (and Pattern) however is supposed to be thread safe, so I'm fine with the improvements on those. Note: I haven't yet validated these changes against Portlet TCK 2.0 or current Jetspeed Portal. Another issue is with regard to the creation of the XMLInputFactory instance: that should always be done against the container ClassLoader, e.g. ensure the Thread ContextClassloader is (temporarily) forced. See also the comment I provided on the original getXMLInputFactory(ClassLoader) method. Regards, Ate
        Hide
        Eric Dalquist added a comment -

        My understanding was that the objects were thread-safe once created and configured. Since we don't ever leak references to them outside of the class I didn't think there would be a problem. I can change them back though.
        The class loader code was changed because if we were able to create single instances of those classes we would always be within the Pluto (or portal's) context during creation. I'll change the XMLInputFactory and DocumentBuilderFactory back and be sure the correct classloader is used.

        Show
        Eric Dalquist added a comment - My understanding was that the objects were thread-safe once created and configured. Since we don't ever leak references to them outside of the class I didn't think there would be a problem. I can change them back though. The class loader code was changed because if we were able to create single instances of those classes we would always be within the Pluto (or portal's) context during creation. I'll change the XMLInputFactory and DocumentBuilderFactory back and be sure the correct classloader is used.
        Hide
        Eric Dalquist added a comment -

        So I noticed that in the previous code DocumentBuilderFactory was being treated as a class level field as well, it was just lazily initialized. Should I leave it as is or do you think it should be created every time it is needed?

        Show
        Eric Dalquist added a comment - So I noticed that in the previous code DocumentBuilderFactory was being treated as a class level field as well, it was just lazily initialized. Should I leave it as is or do you think it should be created every time it is needed?
        Hide
        Ate Douma added a comment -

        Completely right, nice observation
        I think to be on the safe side, yes we should instantiate even de DocumentBuilderFactory every time, or make the usage synchronized which I don't advise.
        All in all, I wouldn't think this service causing lots of memory/performance overhead is it? And the (webapp) loading by itself most likely will account for a multitude of that anyway.

        Show
        Ate Douma added a comment - Completely right, nice observation I think to be on the safe side, yes we should instantiate even de DocumentBuilderFactory every time, or make the usage synchronized which I don't advise. All in all, I wouldn't think this service causing lots of memory/performance overhead is it? And the (webapp) loading by itself most likely will account for a multitude of that anyway.
        Hide
        Eric Dalquist added a comment -

        Right now it isn't but some basic testing shows that creating that JAXBContext can take 500-1000ms every time. We have some uPortal deployers with more than 75 webapps and it adds up in Tomcat startup time pretty fast. All the TCK tests pass so the changes appear to be safe from that context.

        Show
        Eric Dalquist added a comment - Right now it isn't but some basic testing shows that creating that JAXBContext can take 500-1000ms every time. We have some uPortal deployers with more than 75 webapps and it adds up in Tomcat startup time pretty fast. All the TCK tests pass so the changes appear to be safe from that context.
        Hide
        Ate Douma added a comment -

        OK. Your changes at least should improve the JAXBContext overhead then!
        (500-1000ms is quite a lot, as are 75 webapps ...)

        Cheers, Ate

        Show
        Ate Douma added a comment - OK. Your changes at least should improve the JAXBContext overhead then! (500-1000ms is quite a lot, as are 75 webapps ...) Cheers, Ate
        Hide
        Ate Douma added a comment - - edited

        Hi Eric,

        Can you fix the build error on PortletAppDescriptorServiceImpl.java?
        PortletAppDescriptorServiceImpl.java:[329,18] cannot find symbol
        symbol : constructor IOException(java.lang.String,javax.xml.bind.JAXBException)
        location: class java.io.IOException

        Java 1.5 doesn't have such a IOException constructor (1.6 does).

        See also Hudson build status, https://hudson.apache.org/hudson/view/Portals/
        (also notified on commits@portals.apache.org)

        Thanks, Ate

        Show
        Ate Douma added a comment - - edited Hi Eric, Can you fix the build error on PortletAppDescriptorServiceImpl.java? PortletAppDescriptorServiceImpl.java: [329,18] cannot find symbol symbol : constructor IOException(java.lang.String,javax.xml.bind.JAXBException) location: class java.io.IOException Java 1.5 doesn't have such a IOException constructor (1.6 does). See also Hudson build status, https://hudson.apache.org/hudson/view/Portals/ (also notified on commits@portals.apache.org) Thanks, Ate
        Hide
        Eric Dalquist added a comment -

        Wow, sorry for all the trouble on what was supposed to be a simple change.

        I've fixed the API usage, managed to get the JDK 1.5 libraries back on OSX so I can actually test the pluto build under JDK 1.5. I also subscribed to the pluto RSS feeds from Hudson so I'll keep an eye on that as well.

        Show
        Eric Dalquist added a comment - Wow, sorry for all the trouble on what was supposed to be a simple change. I've fixed the API usage, managed to get the JDK 1.5 libraries back on OSX so I can actually test the pluto build under JDK 1.5. I also subscribed to the pluto RSS feeds from Hudson so I'll keep an eye on that as well.

          People

          • Assignee:
            Eric Dalquist
            Reporter:
            Eric Dalquist
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development