4 messages in net.java.dev.wsit.devA few tips to write efficient code wi...| From | Sent On | Attachments |
|---|---|---|
| Kohsuke Kawaguchi | Jul 28, 2006 10:27 am | |
| Jitendra Kotamraju | Jul 28, 2006 10:50 am | |
| Oleksiy Stashok | Aug 1, 2006 5:33 am | |
| Kohsuke Kawaguchi | Aug 1, 2006 9:18 am |
| Subject: | A few tips to write efficient code without bending over backwards | |
|---|---|---|
| From: | Kohsuke Kawaguchi (Kohs...@Sun.COM) | |
| Date: | Jul 28, 2006 10:27:08 am | |
| List: | net.java.dev.wsit.dev | |
Bharath analyzed the performance problem in WSIT and found a few things that seem to be a common mistake in WSIT. Those are not the kind of problems where you need to be very clever to fix. Instead, those are problems that you can avoid by just knowing a few simple guidelines.
So I'd like to take this opportunity to explain some of those, so that people know what they need to care about.
TIP 1. Never create the same JAXBContext more than once
------------------------------------------------------- Always, always assign JAXBContext to a static final field. A JAXBContext object can be shared safely by multiple threads. This is a very expensive to create, so create it once, and share it.
We found a violation of this tip in JAX-WSA, and numerous violations in the security code. For example, the following code illustrates this problem. SAML packages alone contains 39 JAXBContext instantiations, yet they are always only creating two kinds of JAXBContext.
public static Assertion fromElement(org.w3c.dom.Element element)
throws SAMLException {
try {
if ( System.getProperty("com.sun.xml.wss.saml.binding.jaxb") != null
) {
JAXBContext jc =
JAXBContext.newInstance("com.sun.xml.wss.saml.internal.saml11.jaxb10");
javax.xml.bind.Unmarshaller u = jc.createUnmarshaller();
return new
com.sun.xml.wss.saml.assertion.saml11.jaxb10.Assertion(
(com.sun.xml.wss.saml.internal.saml11.jaxb10.impl.AssertionImpl)u.unmarshal(element));
} else {
JAXBContext jc =
JAXBContext.newInstance("com.sun.xml.wss.saml.internal.saml11.jaxb20");
javax.xml.bind.Unmarshaller u = jc.createUnmarshaller();
Object el = u.unmarshal(element);
return new
com.sun.xml.wss.saml.assertion.saml11.jaxb20.Assertion((AssertionType)((JAXBElement)el).getValue());
}
} catch ( Exception ex) {
// log here
throw new SAMLException(ex);
}
}
TIP 2. Factory look-up is expensive
----------------------------------- In general, factory lookup is an expensive operation. This is because those factory look up needs to follow elaborate steps defined in the spec. So it's always a good idea to stop and think if you can reuse it, although you don't have to bend over backward to do it. This is also tricky because often those objects are not thread-safe.
We designed the JAX-WS RI to help you do this. The JAX-WS RI guarantees that each pipe instance can operate in the "single thread mode" --- no two threads will execute on the same pipe instance at any given point.
This allows you to store thread-unsafe resources (like JAXB unmarshaller, marshaller, or JAXP DocumentBuilder, etc) as instance variables of pipes, or objects owned by pipes. If you have any questions around this area, please contact the JAX-WS team.
So code like this needs to raise a mental warning flag:
public Element toElement(RequestSecurityToken rst) { try { DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); dbf.setNamespaceAware(true); DocumentBuilder db = dbf.newDocumentBuilder(); Document doc = db.newDocument();
javax.xml.bind.Marshaller marshaller =
getContext().createMarshaller();
JAXBElement<RequestSecurityTokenType> rstElement = (new
ObjectFactory()).createRequestSecurityToken((RequestSecurityTokenType)rst);
marshaller.marshal(rstElement, doc);
return doc.getDocumentElement();
} catch (Exception ex) { throw new RuntimeException(ex.getMessage(), ex); } }
Aside from blind catch(Exception) which is always a bad sign, this code wastes a lot of objects --- DocumentBuilderFactory, DocumentBuilder, and Marshaller, just to convert RequestSecurityToken into XML.
You definitely need to stop here and think if you can make this class (WSTrustElementFactoryImpl) thread unsafe. That would allow you to store Marshaller and DocumentBuilder as instance variable, and reuse it for all the other 6 invocations of such code.
As long as I'm on WSTrustElementFactory, the code like that is not only unnecessary, but harmful, because it is not synchronized correctly.
public static WSTrustElementFactory newInstance() { if (trustElemFactory == null) { trustElemFactory = new WSTrustElementFactoryImpl(); } return trustElemFactory; }
Google internet with "singleton concurrency" and you'll learn why. If you want to guarantee a singleton semantics, just do:
private static WSTrustElementFactory trustElemFactory = new
WSTrustElementFactoryImpl();
public static WSTrustElementFactory newInstance() { return trustElemFactory; }
-- Kohsuke Kawaguchi Sun Microsystems kohs...@sun.com