Re: Latest submission package to run commercial applications and work loads

[Mark Wielaard]

Hi,

On Mon, 2002-07-29 at 03:25, Wu, Gansha wrote:
>   We have GNU Classpath submissions that make ORP/GNU Classpath run typical
> commercial 
> applications and workloads, this submission is dedicated to bring up ECPerf
> and Java Petstore 
> benchmarks on JBoss/Tomcat with ORP/GNU Classpath as the runtime, and get
> performance comparable 
> to other JVMs. The submission includes bug fixes, performance enhancement,
> refactoring codes for 
> better maintenance, and some new files.

Thanks very much!
I quickly scanned the code and most of it looks very, very useful.

Do you plan to publish ECPerf results? If I remember correctly the
ECPerf benchmark specification is freely available but the actual
implementation has a nasty non-free license that even prevented you from
publishing results before submitting them to a committee first that
could veto any publication. It looked highly unfree which makes it not
something we would like to use. But maybe I am wrong.

Getting JBoss/Tomcat working with Orp is a major achievement! You
probably had to use some non-free libraries to get it all running
though. If you have a list of external (non-free) libraries that were
needed then that could maybe be used by the GNU ClasspathX (extensions)
project <http://www.gnu.org/software/classpathx> to decide what to work
on first.

> We have diff result with Changelog
> and a separate package 
> for original files. The diff result is generated by: 
>    cvs diff ¨CN ¨Cc ¨Cw; and the baseline is GNU Classpath CVS snapshot of
> Jul. 24, 2002.

Thanks. It would be nice if you could separating the patch a bit and put
it in the patch manager so we can add it in smaller pieces when reviewed
<http://savannah.gnu.org/patch/?group=classpath>. I have some first
observations below but it might take a while before everything is merged
into CVS.
Brian, I believe the last time the ORP team submitted patches all legal
issues were cleared so there is nothing preventing us from added the
code, correct?

>   Welcome to try it! We'll come up instructions for setup and configurations
> in future if necessary.

That would be highly appreciated since that would give us a better way
of evaluating some of the improvements.

Some general remarks. Scott Ananian already made some remarks that I
agree with. The following makes it easier to work together as a team and
is absolutely not meant to discourage future submissions. It just makes
integration easier. 
- Submit in smaller bits and pieces that makes discussing the changes
easier.
- A ChangeLog entry should discuss what changed not why it changed (that
should be clear from the code and comments in the code). That way one
can just look at the ChangeLog file to see where precisely something
changed without having to know why it changed which makes bug tracking
easier. See <http://www.gnu.org/prep/standards.html#SEC40>. A very good
guide can be found at <http://www.red-bean.com/cvs2cl/changelogs.html>.
- It is a matter of taste, but most people find unidiffs (diff -u)
easier to read then context diffs (diff -c).
- Please don't forget to add the standard copyright license boilerplate
to the top of new files (javax.rmi and javax.rmi.corba especially).
- New public classes and methods really should have some basic javadoc.
It is boring work but if it isn't done immediately when the code is
created it might never get done...
- If you have some example code for bugs you fixed please send it, we
can turn it into a Mauve test and make sure we don't create regressions.
Or you can create Mauve tests yourself of course... Do you use Mauve to
test Orp?

Here are some short comments about the patches. I hope to make some more
time in the next weeks to review the changes a bit better. If someone
else with CVS write access has more time please add the patches to CVS
if they look right to you.

- A couple of places StringBuffer seems to be a bottleneck for
performance. Initializing the StringBuffer to something like at least
half the expected size seems good practice. I saw that GCJ has
unsynchronized version of StringBuffer that can be used by library code
that knows it is save since it has the only reference to the buffer.
Another thing to look at it the following idea of reusing StringBuffers:
http://h21007.www2.hp.com/dspp/tech/tech_TechDocumentDetailPage_IDX/0,1701,2488,00.html
(The article claims a speed improvement of 65%)

The StringBuffer optimization of URLEncoder can probably be more
specific by using s.length() instead of INITIAL_MAX_PATH.

- The gnu/java/io/encode/Encoder.java patch looks like a "How did
anything ever work before this patch!" thing :) Should probably go in
immediately.

- SystemClassLoader pathinfo caching looks like a real improvement. I
was thinking about adding jar INDEX.LIST support
<http://java.sun.com/j2se/1.4/docs/guide/jar/jar.html#JAR%20Index>. But
this is probably more generic.
Some more caching is done in URLClassLoader. Would it make sense to make
SystemClassLoader a subclass of URLClassLoader and do all caching in
URLClassLoader or would that make VM ClassLoader bootstrapping a
nightmare?

- The gnu/java/net/protocol bug fixes/additions looks like they should
be applied as is. Thanks for the improvements.

- I think you are the only real user of the RMI code. Do we have any
other RMI expert on the list who could review these changes? (The
original implementation was donated by Transvirtual after they abandoned
the code but nobody has been maintaining it since :{
Thanks for your many improvements and additions.

- java/io/Object*Stream.java. We had these classes merged with libgcj
but they are diverging again (partly my fault, when fixing a bug
recently in libgcj I didn't have the time to merge it with Classpath
again). After adding these new additions we should really look at how
they are different. One thing I like about the libgcj version is that it
has a lot less native methods and does more with reflection. That makes
that code much more portable. Is there a (performance?) reason for using
so many native calls?

- ThreadLocal changes. I have to look closer at the changes but one of
the design decisions was to have all that inside the ThreadLocal classes
since Thread is a VM class that might be (very) different between VMs.
Maybe we should split Thread into a generic java.lang.Thread class and a
VM specific java.lang.VMThread helper class?

- The change in java/util/zip/InflaterInputStream.java suggests that you
use the native code implementation. Have you tried to pure java
implementation? Is it to slow in practice?

- I couldn't find the native code implementation for
vm/reference/java/lang/PlatformProcess.java.

Cheers and thanks again,

Mark