On Sat, Feb 12, 2005 at 04:45:43PM +0100, Philippe Elie wrote:

> On Mon, 07 Feb 2005 at 17:04 +0000, Scott T Jones wrote:
>
> > This patch adds two new functions to OProfile.  First, it adds the ability
> > to resolve addresses in anonymous processes, by integrating libopjdl.so
> > into OProfile.  This builds upon the patch previously provided by Will
> > Cohen.  Second, it provides a Java profiler, libopjan.so, which resolves
> > the names of Java jitted methods, when combined with the first feature.
> > This implements the following proposal on the oprofile-list:
> >
> > http://sourceforge.net/mailarchive/forum.php?thread_id=5641225&forum_id=3252
>
> There is some minor things to change before applying it.
>
> - replace all msdos CR/LF in newly created file (e.g. op_jdl_bfd.c)
>
> - many s/return (xx);/return xx;/


Both of these items have been fixed.

>
> - patch contains some chunk diffing only by space, I prefer we avoid that.
>
> - some global var in new .c can be made static afaics

Could you give me one example of each of these?

>
> - Will Cohen patch must be updated and submitted to lkml:
> I get a look to the our current code and I don't think running a kernel
> with Will patch and an old oprofile version can hurt, cookie lookup for
> NO_COOKIE will fail but the cookie will be hashed and no other lookup
> will occur, sample file open for cookie == 0 will fail silently but samples
> lost for these mapping will be accounted.
>
>
> The following can be addressed after applying the patch unless John disagree
>
> - configure.in :
> +if ! ( test -f libopjan/jvmpi.h ) ; then
> +  if test -f $JDKDIR/include/jvmpi.h ; then
> +    cp -p $JDKDIR/include/jvmpi.h libopjan
> +  else
> +    echo You must copy jvmpi.h from your Java include directory
> +    echo to the libopjan directory to use Java profiling.
> +  fi
> +fi
>
> this must be done only if --enable-opjan is given on command line

This check for jvmpi.h will be moved to libopjan/Makefile.am,
which already checks the --enable-opjan flag.
 
>
> - use of dso like:
> oprofile-0.8.1/daemon/Makefile.am
> +   ../libopjdl/.libs/libopjdl.so \
> +   /usr/lib/libbfd.so \
>
> must be done conditionnaly or by building an empty dso and link with it
> unconditionnaly.


I do not understand this.  These DSOs are always used, they are not
conditional.  They provide support for identifying anonymous modules
even if --enable-opjan is not specified.

>
> - I don't understand why libopjdl.so must be linked with pp tools

I can understand that you do not want to possibly taint your pp tools
with regressions.  We only called libopjdl.so from opreport to minimize
the window in which new addresses could be identified in anonymous code.
We will create a stand-alone routine that can call libopjdl.so immediately
before invoking opreport.  We had only hoped to eliminate the possibility
that the user would omit this call.
 
>
> - I dunno if we want to create file under /tmp/oprofile or
> /var/lib/oprofile/sample/vm/java/, I prefer the second option.

At John Levon's request, the file will be created under
/var/lib/oprofile/samples/{anon}/.
 
>
> - later we will need to support multiple jitted source by getting get the
> interface from the tgid

This will be done.

>
> A question about unload method vs unload class. Can a class be unloaded
> w/o all of these methods unloaded first ?

I do not know.  It may be JVM specific.  However, our code will automatically
unload all of the methods associated with a class when the class is unloaded.
 
>
> What's the rationale behind the valid flag vs the removal of class and
> method ? Is it to allow reuse a class or method after an unload then a
> load event ?

It was implemented that way for ease of debugging.  We can easily change the

code to delete these control blocks when classes and methods are unloaded.
 
>
> John, I think we need to release oprofile 0.8.2 before appyling this patch.
>
>
> > A compatible level of jvmpi.h (IBM or Sun 1.3 or above) must be copied to
> > the libopjan directory before libopjan.so can be successfully built.  The
> > new configure script will copy this file from $JDKDIR/include if the
> > JDKDIR
> > environment variable has been set to the directory of the active JDK.
> > Note that libopjan.so is built using libtool, the presence of which is
> > tested at configure time.
>
> Looking at the code I see we need to compile oprofile for a specific jvm,
> either sun or ibm. People will prefer probably we support both of them and
> to figure out the right at runtime. Something to think about, not a stopper
> for the present patch

It was never our intention to require any specific JVM.  Our current code will
successfully build with the jvmpi.h from either IBM or Sun and the version built
with the Sun jvmpi.h will work with either JVM.  However, it is true that a
version built with the IBM jvmpi.h will only work with an IBM JVM.  Therefore,
we will modify our code to ensure that all versions work on any JVM.
 
>
> regards,
> Philippe Elie


Thank you for your helpful comments.

Scott T Jones

WBI Performance II
IBM Corp, Austin, TX
Reply to:  stjones@us.ibm.com
Phone:  (512) 838-4758, T/L:  678-4758