Timothy Peierls <[EMAIL PROTECTED]> writes: > Daniel Rall wrote: > >> That means the same data is buffered in memory (in various >>> forms) a minimum of 4 times (!!). If the response contains >>> large quantities of data, imagine the repercussions... > > I've observed significant overhead when returning large result > objects. It used to be worse, before the intermediate values were > cleared in XmlRpc.Worker, but it's still wasteful. > > So far, the response to this has centered on how the possibilities for > streaming are limited by the need to produce the response body in > memory before writing the Content-length header. However, there are > other ways things could be improved.
Tim, thanks a lot for your comments. > 1. It might be reasonable to perform two passes over the result > object, one to compute the content length, and then one to stream it > out. The value of this change would depend heavily on how long it takes to create the response, and just doesn't seem very elegant (though neither does a frozen XML-RPC spec, bleh). > 2. In any event, why not have XmlWriter write directly to an > OutputStreamWriter that wraps a ByteArrayOutputStream (maybe itself > wrapped in a BufferedWriter)? This would eliminate the need for a > StringBuffer, which is one of the problem areas that Dan is worried > about. I was thinking along the same lines, but your description really helped crystalize my thoughts. I made XmlWriter sub-class the JDK's OutputStreamWriter (for simplicity), and moved writeObject() into it. I am about to commit to CVS, but have attached the patch to the end of this message for comment and review. As this is a fairly extensive change, it would be great to get some eyes on it. > 3. And, at the risk of being a one-note Johnny, I'll point out that > were we to use List and Map instead of Vector and Hashtable, server > side code that, for example, currently constructs a Vector in memory > just to hold the results of iterating over some internal data > structure might instead be able to use a custom List implementation > that would effectively stream those results directly from the List's > Iterator without having to form an intermediate array or Vector. Tim, I would love to see some code implementing this idea! All, please comment on these changes: Index: src/java/org/apache/xmlrpc/XmlRpc.java =================================================================== RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpc.java,v retrieving revision 1.16 diff -u -r1.16 XmlRpc.java --- src/java/org/apache/xmlrpc/XmlRpc.java 18 Feb 2002 17:06:15 -0000 1.16 +++ src/java/org/apache/xmlrpc/XmlRpc.java 18 Feb 2002 22:44:16 -0000 @@ -72,6 +72,7 @@ * @see XmlRpcClient * * @author <a href="mailto:[EMAIL PROTECTED]";>Hannes Wallnoefer</a> + * @author <a href="mailto:[EMAIL PROTECTED]";>Daniel Rall</a> */ public abstract class XmlRpc extends HandlerBase @@ -331,89 +332,6 @@ } /** - * Writes the XML representation of a supported Java object to - * the XML writer. - */ - void writeObject (Object what, XmlWriter writer) - { - writer.startElement ("value"); - if (what == null) - { - throw new RuntimeException ("null value not supported by XML-RPC"); - } - else if (what instanceof String) - { - writer.chardata (what.toString ()); - } - else if (what instanceof Integer) - { - writer.startElement ("int"); - writer.write (what.toString ()); - writer.endElement ("int"); - } - else if (what instanceof Boolean) - { - writer.startElement ("boolean"); - writer.write (((Boolean) what).booleanValue () ? "1" : "0"); - writer.endElement ("boolean"); - } - else if (what instanceof Double || what instanceof Float) - { - writer.startElement ("double"); - writer.write (what.toString ()); - writer.endElement ("double"); - } - else if (what instanceof Date) - { - writer.startElement ("dateTime.iso8601"); - Date d = (Date) what; - writer.write (dateformat.format (d)); - writer.endElement ("dateTime.iso8601"); - } - else if (what instanceof byte[]) - { - writer.startElement ("base64"); - writer.write (Base64.encode ((byte[]) what)); - writer.endElement ("base64"); - } - else if (what instanceof Vector) - { - writer.startElement ("array"); - writer.startElement ("data"); - Vector v = (Vector) what; - int l2 = v.size (); - for (int i2 = 0; i2 < l2; i2++) - writeObject (v.elementAt (i2), writer); - writer.endElement ("data"); - writer.endElement ("array"); - } - else if (what instanceof Hashtable) - { - writer.startElement ("struct"); - Hashtable h = (Hashtable) what; - for (Enumeration e = h.keys (); e.hasMoreElements ();) - { - String nextkey = (String) e.nextElement (); - Object nextval = h.get (nextkey); - writer.startElement ("member"); - writer.startElement ("name"); - writer.write (nextkey); - writer.endElement ("name"); - writeObject (nextval, writer); - writer.endElement ("member"); - } - writer.endElement ("struct"); - } - else - { - throw new RuntimeException ("unsupported Java type: " + - what.getClass ()); - } - writer.endElement ("value"); - } - - - /** * This method is called when a root level object has been parsed. */ abstract void objectParsed (Object what); @@ -700,9 +618,14 @@ /** - * A quick and dirty XML writer. + * A quick and dirty XML writer. If you feed it a + * <code>ByteArrayInputStream</code>, it may be necessary to call + * <code>writer.flush()</code> before calling + * <code>buffer.toByteArray()</code> to get the data written to + * your byte buffer. */ class XmlWriter + extends OutputStreamWriter { protected static final String PROLOG_START = "<?xml version=\"1.0\" encoding=\""; @@ -713,55 +636,137 @@ protected static final String GREATER_THAN_ENTITY = ">"; protected static final String AMPERSAND_ENTITY = "&"; - /** - * The buffer to write to. - */ - StringBuffer buf; - - /** - * The encoding to use. - */ - String enc; - - public XmlWriter (StringBuffer buf) + public XmlWriter (OutputStream out) + throws UnsupportedEncodingException, IOException { // The default encoding used for XML-RPC is ISO-8859-1. - this (buf, encoding); + this (out, encoding); } - public XmlWriter (StringBuffer buf, String enc) + public XmlWriter (OutputStream out, String enc) + throws UnsupportedEncodingException, IOException { - this.buf = buf; - this.enc = enc; + super(out, enc); // Add the XML prolog (which includes the encoding) - buf.append (PROLOG_START); - buf.append (encodings.getProperty (enc, enc)); - buf.append (PROLOG_END); + write(PROLOG_START); + write(encodings.getProperty(enc, enc)); + write(PROLOG_END); } - public void startElement (String elem) + /** + * Writes the XML representation of a supported Java object + * type. + * + * @param obj The <code>Object</code> to write. + */ + public void writeObject (Object obj) + throws IOException { - buf.append ('<'); - buf.append (elem); - buf.append ('>'); + startElement ("value"); + if (obj == null) + { + throw new RuntimeException("null value not supported by XML-RPC"); + } + else if (obj instanceof String) + { + chardata(obj.toString()); + } + else if (obj instanceof Integer) + { + startElement("int"); + write(obj.toString()); + endElement("int"); + } + else if (obj instanceof Boolean) + { + startElement("boolean"); + write(((Boolean) obj).booleanValue() ? "1" : "0"); + endElement("boolean"); + } + else if (obj instanceof Double || obj instanceof Float) + { + startElement("double"); + write(obj.toString()); + endElement("double"); + } + else if (obj instanceof Date) + { + startElement("dateTime.iso8601"); + Date d = (Date) obj; + write(dateformat.format(d)); + endElement("dateTime.iso8601"); + } + else if (obj instanceof byte[]) + { + startElement("base64"); + write(Base64.encode((byte[]) obj)); + endElement("base64"); + } + else if (obj instanceof Vector) + { + startElement("array"); + startElement("data"); + Vector array = (Vector) obj; + int size = array.size(); + for (int i = 0; i < size; i++) + { + writeObject(array.elementAt(i)); + } + endElement("data"); + endElement("array"); + } + else if (obj instanceof Hashtable) + { + startElement("struct"); + Hashtable struct = (Hashtable) obj; + for (Enumeration e = struct.keys(); e.hasMoreElements(); ) + { + String nextkey = (String) e.nextElement(); + Object nextval = struct.get(nextkey); + startElement("member"); + startElement("name"); + write(nextkey); + endElement("name"); + writeObject(nextval); + endElement("member"); + } + endElement("struct"); + } + else + { + throw new RuntimeException("unsupported Java type: " + + obj.getClass()); + } + endElement("value"); + } + + protected void startElement (String elem) + throws IOException + { + write('<'); + write(elem); + write('>'); } - public void endElement (String elem) + protected void endElement (String elem) + throws IOException { - buf.append (CLOSING_TAG_START); - buf.append (elem); - buf.append ('>'); + write(CLOSING_TAG_START); + write(elem); + write('>'); } - public void emptyElement (String elem) + protected void emptyElement (String elem) + throws IOException { - buf.append ('<'); - buf.append (elem); - buf.append (SINGLE_TAG_END); + write('<'); + write(elem); + write(SINGLE_TAG_END); } - public void chardata (String text) + protected void chardata (String text) + throws IOException { int l = text.length (); for (int i = 0; i < l; i++) @@ -769,39 +774,19 @@ char c = text.charAt (i); switch (c) { - case '<' : - buf.append (LESS_THAN_ENTITY); + case '<': + write(LESS_THAN_ENTITY); break; - case '>' : - buf.append (GREATER_THAN_ENTITY); + case '>': + write(GREATER_THAN_ENTITY); break; - case '&' : - buf.append (AMPERSAND_ENTITY); + case '&': + write(AMPERSAND_ENTITY); break; - default : - buf.append (c); + default: + write(c); } } - } - - public void write (char[] text) - { - buf.append (text); - } - - public void write (String text) - { - buf.append (text); - } - - public String toString () - { - return buf.toString (); - } - - public byte[] getBytes () throws UnsupportedEncodingException - { - return buf.toString ().getBytes (enc); } } } Index: src/java/org/apache/xmlrpc/XmlRpcClient.java =================================================================== RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpcClient.java,v retrieving revision 1.5 diff -u -r1.5 XmlRpcClient.java --- src/java/org/apache/xmlrpc/XmlRpcClient.java 18 Feb 2002 17:06:15 -0000 1.5 +++ src/java/org/apache/xmlrpc/XmlRpcClient.java 18 Feb 2002 22:44:17 -0000 @@ -254,7 +254,11 @@ { boolean fault; Object result = null; - StringBuffer strbuf; + + /** + * The output buffer used in creating a request. + */ + ByteArrayOutputStream buffer; CallData call; @@ -298,12 +302,15 @@ catch (Exception x) { if (callback != null) + { try { callback.handleError (x, url, method); } catch (Exception ignore) - {} + { + } + } } } @@ -323,14 +330,19 @@ { ByteArrayOutputStream bout = new ByteArrayOutputStream (); - if (strbuf == null) - strbuf = new StringBuffer (); + if (buffer == null) + { + buffer = new ByteArrayOutputStream(); + } else - strbuf.setLength (0); + { + buffer.reset(); + } - XmlWriter writer = new XmlWriter (strbuf); + XmlWriter writer = new XmlWriter (buffer); writeRequest (writer, method, params); - byte[] request = writer.getBytes(); + writer.flush(); + byte[] request = buffer.toByteArray(); URLConnection con = url.openConnection (); con.setDoInput (true); @@ -407,7 +419,7 @@ for (int i = 0; i < l; i++) { writer.startElement ("param"); - writeObject (params.elementAt (i), writer); + writer.writeObject (params.elementAt (i)); writer.endElement ("param"); } writer.endElement ("params"); Index: src/java/org/apache/xmlrpc/XmlRpcClientLite.java =================================================================== RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpcClientLite.java,v retrieving revision 1.2 diff -u -r1.2 XmlRpcClientLite.java --- src/java/org/apache/xmlrpc/XmlRpcClientLite.java 18 Feb 2002 17:06:15 -0000 1.2 +++ src/java/org/apache/xmlrpc/XmlRpcClientLite.java 18 Feb 2002 22:44:18 -0000 @@ -126,7 +126,6 @@ class LiteWorker extends Worker implements Runnable { - HttpClient client = null; public LiteWorker () @@ -134,21 +133,25 @@ super (); } - - Object execute (String method, - Vector params) throws XmlRpcException, IOException + Object execute (String method, Vector params) + throws XmlRpcException, IOException { long now = System.currentTimeMillis (); fault = false; try { - if (strbuf == null) - strbuf = new StringBuffer (); + if (buffer == null) + { + buffer = new ByteArrayOutputStream(); + } else - strbuf.setLength (0); - XmlWriter writer = new XmlWriter (strbuf); + { + buffer.reset(); + } + XmlWriter writer = new XmlWriter (buffer); writeRequest (writer, method, params); - byte[] request = writer.getBytes(); + writer.flush(); + byte[] request = buffer.toByteArray(); // and send it to the server if (client == null) Index: src/java/org/apache/xmlrpc/XmlRpcServer.java =================================================================== RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpcServer.java,v retrieving revision 1.10 diff -u -r1.10 XmlRpcServer.java --- src/java/org/apache/xmlrpc/XmlRpcServer.java 18 Feb 2002 17:06:15 -0000 1.10 +++ src/java/org/apache/xmlrpc/XmlRpcServer.java 18 Feb 2002 22:44:19 -0000 @@ -66,6 +66,7 @@ * HTTP listener, use the WebServer class instead. * * @author <a href="mailto:[EMAIL PROTECTED]";>Hannes Wallnoefer</a> + * @author <a href="mailto:[EMAIL PROTECTED]";>Daniel Rall</a> */ public class XmlRpcServer { @@ -150,8 +151,9 @@ */ class Worker extends XmlRpc { - Vector inParams; - StringBuffer strbuf; + private Vector inParams; + private ByteArrayOutputStream buffer; + private XmlWriter writer; /** * Creates a new instance. @@ -159,7 +161,7 @@ protected Worker() { inParams = new Vector(); - strbuf = new StringBuffer(); + buffer = new ByteArrayOutputStream(); } /** @@ -175,7 +177,7 @@ finally { // Release most of our resources - strbuf.setLength (0); + buffer.reset(); inParams.removeAllElements(); } } @@ -227,17 +229,22 @@ Object outParam; if (handler instanceof AuthenticatedXmlRpcHandler) + { outParam = ((AuthenticatedXmlRpcHandler) handler). execute (methodName, inParams, user, password); + } else + { outParam = ((XmlRpcHandler) handler).execute ( methodName, inParams); + } if (debug) System.err.println ("outparam = "+outParam); - XmlWriter writer = new XmlWriter (strbuf); + writer = new XmlWriter(buffer); writeResponse (outParam, writer); - result = writer.getBytes (); + writer.flush(); + result = buffer.toByteArray(); } catch (Exception x) { @@ -249,32 +256,69 @@ // It is possible that something is in the buffer // if there were an exception during the writeResponse() // call above. - strbuf.setLength(0); + buffer.reset(); + + writer = null; + try + { + writer = new XmlWriter(buffer); + } + catch (UnsupportedEncodingException encx) + { + System.err.println("XmlRpcServer attempted to use " + + "unsupported encoding: " + encx); + // NOTE: If we weren't already using the default + // encoding, we could try it here. + } + catch (IOException iox) + { + System.err.println("XmlRpcServer experienced I/O error " + + "writing error response: " + iox); + } - XmlWriter writer = new XmlWriter (strbuf); String message = x.toString (); - // check if XmlRpcException was thrown so we can get an error code + // Retrieve XmlRpcException error code (if possible). int code = x instanceof XmlRpcException ? ((XmlRpcException) x).code : 0; try { writeError (code, message, writer); + writer.flush(); } - catch (XmlRpcException e) + catch (Exception e) { // Unlikely to occur, as we just sent a struct // with an int and a string. System.err.println("Unable to send error response to " + "client: " + e); } - try + + // If we were able to create a XmlWriter, we should + // have a response. + if (writer != null) { - result = writer.getBytes (); + result = buffer.toByteArray(); } - catch (UnsupportedEncodingException encx) + else + { + result = "".getBytes(); + } + } + finally + { + if (writer != null) { - System.err.println ("XmlRpcServer.execute: "+encx); - result = writer.toString().getBytes(); + try + { + writer.close(); + } + catch (IOException iox) + { + // This is non-fatal, but worth logging a + // warning for. + System.err.println("Exception closing output stream: " + + iox); + } } } if (debug) @@ -284,8 +328,9 @@ } /** - * Called when an object to be added to the argument list has been parsed. - */ + * Called when an object to be added to the argument list has + * been parsed. + */ void objectParsed (Object what) { inParams.addElement (what); @@ -294,14 +339,14 @@ /** * Writes an XML-RPC response to the XML writer. */ - void writeResponse (Object param, - XmlWriter writer) throws XmlRpcException + void writeResponse (Object param, XmlWriter writer) + throws XmlRpcException, IOException { writer.startElement ("methodResponse"); // if (param == null) param = ""; // workaround for Frontier bug writer.startElement ("params"); writer.startElement ("param"); - writeObject (param, writer); + writer.writeObject (param); writer.endElement ("param"); writer.endElement ("params"); writer.endElement ("methodResponse"); @@ -310,8 +355,8 @@ /** * Writes an XML-RPC error response to the XML writer. */ - void writeError (int code, String message, - XmlWriter writer) throws XmlRpcException + void writeError (int code, String message, XmlWriter writer) + throws XmlRpcException, IOException { // System.err.println ("error: "+message); Hashtable h = new Hashtable (); @@ -319,7 +364,7 @@ h.put ("faultString", message); writer.startElement ("methodResponse"); writer.startElement ("fault"); - writeObject (h, writer); + writer.writeObject (h); writer.endElement ("fault"); writer.endElement ("methodResponse"); }
