You are viewing a plain text version of this content. The canonical link for it is here.
Posted to xmlrpc-dev@ws.apache.org by Timothy Peierls <ti...@peierls.net> on 2002/01/15 19:15:17 UTC
cleanup in XmlRpcServer.Worker
The XmlRpcServer.Worker fields inParams, outParam, result, and
strbuf are not being cleared on exit from the execute() method, and
I believe they should be. All it takes is to wrap the execute() body
with
try {
...
}
finally {
inParams = null;
outParam = null;
result = null;
strBuf.setLength(0);
}
and remove the existing else clause that has "strBuf.setLength(0)".
Even better, if outParam and result were made local variables
instead of fields of Worker, you could remove the second and third
lines of the finally clause.
Why does this matter? Because there is no guarantee that the
execute() method will be called again any time soon, or ever. Since
there is no size restriction on arguments and results in the XML-RPC
spec, you can have arbitrarily large objects hanging around for
arbitrarily long intervals, a classic Java memory leak.
I saw this happen in practice: The service defined a method to
return the state of the entire system (as XML-RPC structs and
arrays). Huge chunks of memory that could have been freed up on
return were left dangling uselessly until the Worker was next used.
Performance noticeably improved when we made a local fix to the
Worker class.
I asked about this over a year ago, but I think it got lost in the
cracks.
Tim Peierls <ma...@peierls.net>
Re: cleanup in XmlRpcServer.Worker
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Timothy Peierls <ti...@peierls.net> writes:
> Daniel Rall wrote:
> > Hi Tim. I've committed code implementing the general idea behind your
>> patch to CVS. Thanks much!
>
> You're welcome.
>
> I notice you left the byte[] result as a field rather than make it a
> local variable. Though it won't make any difference for small responses,
> it is still a potential memory leak if the responses get very large. Is
> there some other reason to use a field here?
Just to reuse the pointer. You're right that what it's pointing at
should be cleared -- probably better to just make it local as you
suggested. Done.
> Come to think of it, the use of setLength(0) to clear the StringBuffer
> is a bit risky. There is no guarantee that setLength releases any
> memory. Maybe it would be better simply to allocate a new StringBuffer
> for each execute?
>
> Same reasoning applies to the argument Vector, in theory, but in this
> case the "leak" is proportional to the number of arguments, which is
> unlikely to be significant, unlike the StringBuffer and byte array. So
> what you did with removeAllElements() seems like the right choice to me.
I didn't expect there to be enough input arguments to make a
difference for the Vector (if there are, the technology is probably
just not being used right in the first place). For the StringBuffer,
I also wanted to hold on to the buffer to prevent having to go back to
the heap for more. Perhaps it makes sense to release the StringBuffer
and/or byte[] only if they surpass a given size (say, 2kb?).
Thoughts?
Re: Dropping JDK 1.1 compatibility
Posted by Jason van Zyl <jv...@zenplex.com>.
On 1/26/02 8:17 AM, "Hannes Wallnoefer" <ha...@helma.at> wrote:
> Daniel Rall wrote:
>
>> Timothy Peierls <ti...@peierls.net> writes:
>>
>>
>>> Speaking of Vector: Now that there is language in the README that
>>> suggests a Java version requirement of 1.2+, is it time to switch from
>>> Vector to List, and from Hashtable to Map? I brought this up almost two
>>> years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
>>> but at the time many folks were counting on 1.1 compatibility.
>>>
>>
>> I would like to propose that we drop JDK 1.1 compatibility in favor of
>> 1.2 and higher. With 1.4 right around the corner, and the rich set of
>> collections offered by 1.2, it is definitely a step in the right
>> direction.
>>
>> Can I get some +1's on this from committers, please?
>
>
> Yes, I guess it's about time.
>
> +1
+1
> Hannes
>
>
--
jvz.
Jason van Zyl
http://tambora.zenplex.org
http://jakarta.apache.org/turbine
http://jakarta.apache.org/velocity
http://jakarta.apache.org/alexandria
http://jakarta.apache.org/commons
Re: Dropping JDK 1.1 compatibility
Posted by Jason van Zyl <jv...@zenplex.com>.
On 1/26/02 8:17 AM, "Hannes Wallnoefer" <ha...@helma.at> wrote:
> Daniel Rall wrote:
>
>> Timothy Peierls <ti...@peierls.net> writes:
>>
>>
>>> Speaking of Vector: Now that there is language in the README that
>>> suggests a Java version requirement of 1.2+, is it time to switch from
>>> Vector to List, and from Hashtable to Map? I brought this up almost two
>>> years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
>>> but at the time many folks were counting on 1.1 compatibility.
>>>
>>
>> I would like to propose that we drop JDK 1.1 compatibility in favor of
>> 1.2 and higher. With 1.4 right around the corner, and the rich set of
>> collections offered by 1.2, it is definitely a step in the right
>> direction.
>>
>> Can I get some +1's on this from committers, please?
>
>
> Yes, I guess it's about time.
>
> +1
+1
> Hannes
>
>
--
jvz.
Jason van Zyl
http://tambora.zenplex.org
http://jakarta.apache.org/turbine
http://jakarta.apache.org/velocity
http://jakarta.apache.org/alexandria
http://jakarta.apache.org/commons
Re: Dropping JDK 1.1 compatibility
Posted by Hannes Wallnoefer <ha...@helma.at>.
Daniel Rall wrote:
> Timothy Peierls <ti...@peierls.net> writes:
>
>
>>Speaking of Vector: Now that there is language in the README that
>>suggests a Java version requirement of 1.2+, is it time to switch from
>>Vector to List, and from Hashtable to Map? I brought this up almost two
>>years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
>>but at the time many folks were counting on 1.1 compatibility.
>>
>
> I would like to propose that we drop JDK 1.1 compatibility in favor of
> 1.2 and higher. With 1.4 right around the corner, and the rich set of
> collections offered by 1.2, it is definitely a step in the right
> direction.
>
> Can I get some +1's on this from committers, please?
Yes, I guess it's about time.
+1
Hannes
Re: Dropping JDK 1.1 compatibility
Posted by Timothy Peierls <ti...@peierls.net>.
Timothy Peierls <ti...@peierls.net> writes:
> > 1) adding XmlRpc class properties that determine which implementations
> > of List and Map should be used when converting <array> and <struct>
> > arguments and results to Java objects, and
> >
> > 2) adding a backward compatibility class boolean that produces the
> > old behavior without having to do anything else.
Daniel Rall wrote:
> Item 1 sounds like a good idea, but I don't see the point of item 2 if
> item 1 is clearly documented.
Yeah, I guess it's not important. I just thought there would be howls
of protest if there were no simple way to get the old behavior.
Depends on whether you think
XmlRpc.structClass = java.util.Hashtable;
XmlRpc.arrayClass = java.util.Vector;
is too complicated.
> A patch would be terrific. ;-)
I'll see what I can do. I had one ready two years ago... :-)
--tim
Re: Dropping JDK 1.1 compatibility
Posted by Timothy Peierls <ti...@peierls.net>.
Timothy Peierls <ti...@peierls.net> writes:
> > 1) adding XmlRpc class properties that determine which implementations
> > of List and Map should be used when converting <array> and <struct>
> > arguments and results to Java objects, and
> >
> > 2) adding a backward compatibility class boolean that produces the
> > old behavior without having to do anything else.
Daniel Rall wrote:
> Item 1 sounds like a good idea, but I don't see the point of item 2 if
> item 1 is clearly documented.
Yeah, I guess it's not important. I just thought there would be howls
of protest if there were no simple way to get the old behavior.
Depends on whether you think
XmlRpc.structClass = java.util.Hashtable;
XmlRpc.arrayClass = java.util.Vector;
is too complicated.
> A patch would be terrific. ;-)
I'll see what I can do. I had one ready two years ago... :-)
--tim
Re: Dropping JDK 1.1 compatibility
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Timothy Peierls <ti...@peierls.net> writes:
> Dan Rall wrote:
> > I would like to propose that we drop JDK 1.1 compatibility in favor of
>> 1.2 and higher.
>
> If/when this is approved, I'd like to suggest that backward compatibility
> with Vector and Hashtable be maintained by
>
> 1) adding XmlRpc class properties that determine which implementations
> of List and Map should be used when converting <array> and <struct>
> arguments and results to Java objects, and
>
> 2) adding a backward compatibility class boolean that produces the
> old behavior without having to do anything else.
>
> Defaults for 1) would be ArrayList and HashMap. (Going from Java to
> <array> and <struct> is no problem: anything implementing List is
> an <array> and anything implementing Map is a <struct>.)
Item 1 sounds like a good idea, but I don't see the point of item 2 if
item 1 is clearly documented. A patch would be terrific. ;-)
Thanks, Dan
Re: Dropping JDK 1.1 compatibility
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Timothy Peierls <ti...@peierls.net> writes:
> Dan Rall wrote:
> > I would like to propose that we drop JDK 1.1 compatibility in favor of
>> 1.2 and higher.
>
> If/when this is approved, I'd like to suggest that backward compatibility
> with Vector and Hashtable be maintained by
>
> 1) adding XmlRpc class properties that determine which implementations
> of List and Map should be used when converting <array> and <struct>
> arguments and results to Java objects, and
>
> 2) adding a backward compatibility class boolean that produces the
> old behavior without having to do anything else.
>
> Defaults for 1) would be ArrayList and HashMap. (Going from Java to
> <array> and <struct> is no problem: anything implementing List is
> an <array> and anything implementing Map is a <struct>.)
Item 1 sounds like a good idea, but I don't see the point of item 2 if
item 1 is clearly documented. A patch would be terrific. ;-)
Thanks, Dan
Re: Dropping JDK 1.1 compatibility
Posted by Timothy Peierls <ti...@peierls.net>.
Dan Rall wrote:
> I would like to propose that we drop JDK 1.1 compatibility in favor of
> 1.2 and higher.
If/when this is approved, I'd like to suggest that backward compatibility
with Vector and Hashtable be maintained by
1) adding XmlRpc class properties that determine which implementations
of List and Map should be used when converting <array> and <struct>
arguments and results to Java objects, and
2) adding a backward compatibility class boolean that produces the
old behavior without having to do anything else.
Defaults for 1) would be ArrayList and HashMap. (Going from Java to
<array> and <struct> is no problem: anything implementing List is
an <array> and anything implementing Map is a <struct>.)
--tim
Re: Dropping JDK 1.1 compatibility
Posted by Hannes Wallnoefer <ha...@helma.at>.
Daniel Rall wrote:
> Timothy Peierls <ti...@peierls.net> writes:
>
>
>>Speaking of Vector: Now that there is language in the README that
>>suggests a Java version requirement of 1.2+, is it time to switch from
>>Vector to List, and from Hashtable to Map? I brought this up almost two
>>years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
>>but at the time many folks were counting on 1.1 compatibility.
>>
>
> I would like to propose that we drop JDK 1.1 compatibility in favor of
> 1.2 and higher. With 1.4 right around the corner, and the rich set of
> collections offered by 1.2, it is definitely a step in the right
> direction.
>
> Can I get some +1's on this from committers, please?
Yes, I guess it's about time.
+1
Hannes
Re: Dropping JDK 1.1 compatibility
Posted by Timothy Peierls <ti...@peierls.net>.
Dan Rall wrote:
> I would like to propose that we drop JDK 1.1 compatibility in favor of
> 1.2 and higher.
If/when this is approved, I'd like to suggest that backward compatibility
with Vector and Hashtable be maintained by
1) adding XmlRpc class properties that determine which implementations
of List and Map should be used when converting <array> and <struct>
arguments and results to Java objects, and
2) adding a backward compatibility class boolean that produces the
old behavior without having to do anything else.
Defaults for 1) would be ArrayList and HashMap. (Going from Java to
<array> and <struct> is no problem: anything implementing List is
an <array> and anything implementing Map is a <struct>.)
--tim
Dropping JDK 1.1 compatibility
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Timothy Peierls <ti...@peierls.net> writes:
> Speaking of Vector: Now that there is language in the README that
> suggests a Java version requirement of 1.2+, is it time to switch from
> Vector to List, and from Hashtable to Map? I brought this up almost two
> years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
> but at the time many folks were counting on 1.1 compatibility.
I would like to propose that we drop JDK 1.1 compatibility in favor of
1.2 and higher. With 1.4 right around the corner, and the rich set of
collections offered by 1.2, it is definitely a step in the right
direction.
Can I get some +1's on this from committers, please?
Dan
Re: cleanup in XmlRpcServer.Worker
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Timothy Peierls <ti...@peierls.net> writes:
> Daniel Rall wrote:
> > Hi Tim. I've committed code implementing the general idea behind your
>> patch to CVS. Thanks much!
>
> You're welcome.
>
> I notice you left the byte[] result as a field rather than make it a
> local variable. Though it won't make any difference for small responses,
> it is still a potential memory leak if the responses get very large. Is
> there some other reason to use a field here?
Just to reuse the pointer. You're right that what it's pointing at
should be cleared -- probably better to just make it local as you
suggested. Done.
> Come to think of it, the use of setLength(0) to clear the StringBuffer
> is a bit risky. There is no guarantee that setLength releases any
> memory. Maybe it would be better simply to allocate a new StringBuffer
> for each execute?
>
> Same reasoning applies to the argument Vector, in theory, but in this
> case the "leak" is proportional to the number of arguments, which is
> unlikely to be significant, unlike the StringBuffer and byte array. So
> what you did with removeAllElements() seems like the right choice to me.
I didn't expect there to be enough input arguments to make a
difference for the Vector (if there are, the technology is probably
just not being used right in the first place). For the StringBuffer,
I also wanted to hold on to the buffer to prevent having to go back to
the heap for more. Perhaps it makes sense to release the StringBuffer
and/or byte[] only if they surpass a given size (say, 2kb?).
Thoughts?
Dropping JDK 1.1 compatibility
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Timothy Peierls <ti...@peierls.net> writes:
> Speaking of Vector: Now that there is language in the README that
> suggests a Java version requirement of 1.2+, is it time to switch from
> Vector to List, and from Hashtable to Map? I brought this up almost two
> years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
> but at the time many folks were counting on 1.1 compatibility.
I would like to propose that we drop JDK 1.1 compatibility in favor of
1.2 and higher. With 1.4 right around the corner, and the rich set of
collections offered by 1.2, it is definitely a step in the right
direction.
Can I get some +1's on this from committers, please?
Dan
Re: cleanup in XmlRpcServer.Worker
Posted by Timothy Peierls <ti...@peierls.net>.
Daniel Rall wrote:
> Hi Tim. I've committed code implementing the general idea behind your
> patch to CVS. Thanks much!
You're welcome.
I notice you left the byte[] result as a field rather than make it a
local variable. Though it won't make any difference for small responses,
it is still a potential memory leak if the responses get very large. Is
there some other reason to use a field here?
Come to think of it, the use of setLength(0) to clear the StringBuffer
is a bit risky. There is no guarantee that setLength releases any
memory. Maybe it would be better simply to allocate a new StringBuffer
for each execute?
Same reasoning applies to the argument Vector, in theory, but in this
case the "leak" is proportional to the number of arguments, which is
unlikely to be significant, unlike the StringBuffer and byte array. So
what you did with removeAllElements() seems like the right choice to me.
Speaking of Vector: Now that there is language in the README that
suggests a Java version requirement of 1.2+, is it time to switch from
Vector to List, and from Hashtable to Map? I brought this up almost two
years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
but at the time many folks were counting on 1.1 compatibility.
--tim
Re: cleanup in XmlRpcServer.Worker
Posted by Timothy Peierls <ti...@peierls.net>.
Daniel Rall wrote:
> Hi Tim. I've committed code implementing the general idea behind your
> patch to CVS. Thanks much!
You're welcome.
I notice you left the byte[] result as a field rather than make it a
local variable. Though it won't make any difference for small responses,
it is still a potential memory leak if the responses get very large. Is
there some other reason to use a field here?
Come to think of it, the use of setLength(0) to clear the StringBuffer
is a bit risky. There is no guarantee that setLength releases any
memory. Maybe it would be better simply to allocate a new StringBuffer
for each execute?
Same reasoning applies to the argument Vector, in theory, but in this
case the "leak" is proportional to the number of arguments, which is
unlikely to be significant, unlike the StringBuffer and byte array. So
what you did with removeAllElements() seems like the right choice to me.
Speaking of Vector: Now that there is language in the README that
suggests a Java version requirement of 1.2+, is it time to switch from
Vector to List, and from Hashtable to Map? I brought this up almost two
years ago (http://helma.org/archives/xmlrpc/2000-April/000007.html),
but at the time many folks were counting on 1.1 compatibility.
--tim
Re: cleanup in XmlRpcServer.Worker
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Hi Tim. I've committed code implementing the general idea behind your
patch to CVS. Thanks much!
Dan
Timothy Peierls <ti...@peierls.net> writes:
> The XmlRpcServer.Worker fields inParams, outParam, result, and
> strbuf are not being cleared on exit from the execute() method, and
> I believe they should be. All it takes is to wrap the execute() body
> with
>
> try {
> ...
> }
> finally {
> inParams = null;
> outParam = null;
> result = null;
> strBuf.setLength(0);
> }
>
> and remove the existing else clause that has "strBuf.setLength(0)".
>
> Even better, if outParam and result were made local variables
> instead of fields of Worker, you could remove the second and third
> lines of the finally clause.
>
> Why does this matter? Because there is no guarantee that the
> execute() method will be called again any time soon, or ever. Since
> there is no size restriction on arguments and results in the XML-RPC
> spec, you can have arbitrarily large objects hanging around for
> arbitrarily long intervals, a classic Java memory leak.
>
> I saw this happen in practice: The service defined a method to
> return the state of the entire system (as XML-RPC structs and
> arrays). Huge chunks of memory that could have been freed up on
> return were left dangling uselessly until the Worker was next used.
> Performance noticeably improved when we made a local fix to the
> Worker class.
>
> I asked about this over a year ago, but I think it got lost in the
> cracks.
>
> Tim Peierls <ma...@peierls.net>
[PATCH] cleanup in XmlRpcServer.Worker
Posted by Timothy Peierls <ti...@peierls.net>.
> [Dan Rall wrote:]
> Tim, would you please submit a patch to this effect? Your suggested
> changes sound good.
Patch is attached. If people don't feel comfortable with my
use of a private method (executeInternal), it would be simple
enough to do it all inside execute. I just find it easier
to understand and reason about this way.
--tim
[PATCH] cleanup in XmlRpcServer.Worker
Posted by Timothy Peierls <ti...@peierls.net>.
> [Dan Rall wrote:]
> Tim, would you please submit a patch to this effect? Your suggested
> changes sound good.
Patch is attached. If people don't feel comfortable with my
use of a private method (executeInternal), it would be simple
enough to do it all inside execute. I just find it easier
to understand and reason about this way.
--tim
Re: cleanup in XmlRpcServer.Worker
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Tim, would you please submit a patch to this effect? Your suggested
changes sound good.
http://jakarta.apache.org/site/source.html
Thanks, Dan
Timothy Peierls <ti...@peierls.net> writes:
> The XmlRpcServer.Worker fields inParams, outParam, result, and
> strbuf are not being cleared on exit from the execute() method, and
> I believe they should be. All it takes is to wrap the execute() body
> with
>
> try {
> ...
> }
> finally {
> inParams = null;
> outParam = null;
> result = null;
> strBuf.setLength(0);
> }
>
> and remove the existing else clause that has "strBuf.setLength(0)".
>
> Even better, if outParam and result were made local variables
> instead of fields of Worker, you could remove the second and third
> lines of the finally clause.
>
> Why does this matter? Because there is no guarantee that the
> execute() method will be called again any time soon, or ever. Since
> there is no size restriction on arguments and results in the XML-RPC
> spec, you can have arbitrarily large objects hanging around for
> arbitrarily long intervals, a classic Java memory leak.
>
> I saw this happen in practice: The service defined a method to
> return the state of the entire system (as XML-RPC structs and
> arrays). Huge chunks of memory that could have been freed up on
> return were left dangling uselessly until the Worker was next used.
> Performance noticeably improved when we made a local fix to the
> Worker class.
>
> I asked about this over a year ago, but I think it got lost in the
> cracks.
Re: cleanup in XmlRpcServer.Worker
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Hi Tim. I've committed code implementing the general idea behind your
patch to CVS. Thanks much!
Dan
Timothy Peierls <ti...@peierls.net> writes:
> The XmlRpcServer.Worker fields inParams, outParam, result, and
> strbuf are not being cleared on exit from the execute() method, and
> I believe they should be. All it takes is to wrap the execute() body
> with
>
> try {
> ...
> }
> finally {
> inParams = null;
> outParam = null;
> result = null;
> strBuf.setLength(0);
> }
>
> and remove the existing else clause that has "strBuf.setLength(0)".
>
> Even better, if outParam and result were made local variables
> instead of fields of Worker, you could remove the second and third
> lines of the finally clause.
>
> Why does this matter? Because there is no guarantee that the
> execute() method will be called again any time soon, or ever. Since
> there is no size restriction on arguments and results in the XML-RPC
> spec, you can have arbitrarily large objects hanging around for
> arbitrarily long intervals, a classic Java memory leak.
>
> I saw this happen in practice: The service defined a method to
> return the state of the entire system (as XML-RPC structs and
> arrays). Huge chunks of memory that could have been freed up on
> return were left dangling uselessly until the Worker was next used.
> Performance noticeably improved when we made a local fix to the
> Worker class.
>
> I asked about this over a year ago, but I think it got lost in the
> cracks.
>
> Tim Peierls <ma...@peierls.net>
Re: cleanup in XmlRpcServer.Worker
Posted by Daniel Rall <dl...@finemaltcoding.com>.
Tim, would you please submit a patch to this effect? Your suggested
changes sound good.
http://jakarta.apache.org/site/source.html
Thanks, Dan
Timothy Peierls <ti...@peierls.net> writes:
> The XmlRpcServer.Worker fields inParams, outParam, result, and
> strbuf are not being cleared on exit from the execute() method, and
> I believe they should be. All it takes is to wrap the execute() body
> with
>
> try {
> ...
> }
> finally {
> inParams = null;
> outParam = null;
> result = null;
> strBuf.setLength(0);
> }
>
> and remove the existing else clause that has "strBuf.setLength(0)".
>
> Even better, if outParam and result were made local variables
> instead of fields of Worker, you could remove the second and third
> lines of the finally clause.
>
> Why does this matter? Because there is no guarantee that the
> execute() method will be called again any time soon, or ever. Since
> there is no size restriction on arguments and results in the XML-RPC
> spec, you can have arbitrarily large objects hanging around for
> arbitrarily long intervals, a classic Java memory leak.
>
> I saw this happen in practice: The service defined a method to
> return the state of the entire system (as XML-RPC structs and
> arrays). Huge chunks of memory that could have been freed up on
> return were left dangling uselessly until the Worker was next used.
> Performance noticeably improved when we made a local fix to the
> Worker class.
>
> I asked about this over a year ago, but I think it got lost in the
> cracks.