On 5/23/2012 9:26 AM, Mark Wielaard wrote:
On Wed, 2012-05-23 at 09:14 -0400, Keith McGuigan wrote:
Just a couple of quick comments/questions on the code:
Why did we get rid of a couple of the declarations in jni.cpp? Why
aren't they needed?
Could you say which ones you think we got rid of? It was not my
intention to get rid of anything. Maybe you are referring to the first
patch? That came with the following description:
The first patch is just a consistency cleanup patch. The JNI Set and
SetStatic Field methods used HS_DTRACE_PROBE_CDECL_N and HS_DTRACE_PROBE_N
directly instead of just using DTRACE_PROBE[N] like all other JNI methods.
This doesn't matter for the Solaris macros, but on GNU/Linux the macros
don't use direct function declarations (which is introduced in the second
patch).
Ok, might be that it's fine. I don't remember if there was a reason
that SetStatic was different from the other JNI calls, but it certainly
could just have been an omission in refactoring or something. We just
need to make sure that it still works (solaris-dtrace-wise). It's
certainly better this way!
dtrace.hpp should be refactored (if possible) into the os-specific
subdirectories, instead of using #ifdef<OS> macros in the header.
I.e., you might have to make a src/solaris/vm/dtrace_solaris.hpp and
src/linux/vm/dtrace_linux.hpp file and include it from here.
I rather not, the code should be identical except for two small details
between solaris and gnu/linux as the patch shows (and one is just a hack
for a solaris tailcall bug). The apple/macos part looks very different
and could be split out, but that was already there.
I refer to the redefinition blocks of HS_DTRACE_PROBEx, which I don't
consider a small detail (though the solaris tailcall thing certainly
is). It's (of course) just a style thing, but traditionally in hotspot
we've wanted the os or arch specific code in os or arch specific
directories, instead of littering the code with #ifdefs. I know the OSX
stuff started violated this some, but I hope we're going to resolve that
rather than continue down that path.
Has this been built on solaris, and is the dtrace functionality unchanged?
I don't have access to solaris, the dtrace functionality should of
course be unchanged.
Understood. We'll just have to make sure that gets done somehow.
--
- Keith
