From: Philippe E. <ph...@wa...> - 2005-02-19 02:28:13
|
On Wed, 16 Feb 2005 at 15:42 +0000, Scott T Jones wrote: > > - 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? --- oprofile-0.8.1/configure.in 2004-09-11 12:45:09.000000000 -0500 [snip] @@ -24,7 +24,7 @@ AC_SUBST(DATE) dnl needed for module build OPROFILE_DIR=`pwd` AC_SUBST(OPROFILE_DIR) - + AC_PROG_CC AC_PROG_CPP AC_PROG_CXX @@ -48,7 +48,7 @@ AC_SUBST(EXTRA_CFLAGS_MODULE) This is a minor problem anyway. op_jan.c, most (all ?) global variables at start of file seems static, some functions are probably static too. > > 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. make shouldn't fail after a ./configure, the check must be done in configure imho as you do actually. Btw can't we use the jvmpi.h provided by gcc or provide our own jvmpi.h by deriving a gpl'ed jvmpi.h from the the gcc one ? I don't know if jvmpi.h is a part of some sort of official java ABI and if jdk developers take care about backward binary compatiblity for the interface exposed in jvmpi.h > > - 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. embedded people will not like a link to libfd.so, we need probably two ./configure options, one to allow anonymous samples and one depending on the first for java. > > > > - 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 missed the call from pp tools to flush image file to disk, I though it was done by the daemon through an opcontrol --dump. John what about this ? > > > > 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. if throwing events to unload all functions of a class before throwing an unload class event is not a part of java ABI your way is right. It's something to check later, if it's clearly documented we can avoid the linked list of method in struct class_node but for now it's better to prefer robust code. > > 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. Perhaps we can use it, let say daemon receive a unload event class.foo() but the oprofile per cpu buffer contains sample to this function we can perhaps accept samples to deleted function avoiding to lost some sample especially at end of a jitted application. Afaics actually we lost all this samples ? We had something roughly similar in 2.4 daemon, we delayed death of some data struct, iirc the trick was to mark as really deleted a data struct for which we didn't receive any samples during two consecutives flush of the kernel buffers, see daemon/liblegacy/opd_proc.c:opd_age_proc(), proc->accessed is incremented once by dump if this opd_proc struct receive any sample so the opd_proc * was deleted after a dump w/o any sample to this proc) > 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. this raise again the use or the copy in oprofile source of gcc's jvmpi.h, does it make sense ? I've not yet tested this patch but: libutil/op_file.c: +/* generate anonymous image if possible */ +void op_write_jit_mapping_file(char const * file) +{ + op_jit_write_mapping_file(file); + return; +} libopjdl/op_jdl.h: +void op_jit_write_mapping_file(char * name); the compiler doesn't complain about the const pointer casted implicitely to a non-const pointer ? +++ oprofile-new/libopjdl/op_jdl_internal.h 2005-02-07 14:29:57.000000000 -0600 @@ -0,0 +1,128 @@ +/** + * @file libjdl/op_jdl.h typo, op_jdl_internal.h regards, Philippe Elie |