Serguei -
I think that all sounds fine.
--
- Keith
On 5/23/2012 1:25 PM, [email protected] wrote:
Keith,
I agree that it'd be nice to follow the rules.
But we also can do it in two stages:
- first integration to keep close to the original fix that was already
tested on Linux platform (as Mark tells)
- separate refactoring to follow the platform separation rules (will
need to file another CR)
Not sure I'll be able to work on this in the near future (a month) as I
have to switch to the urgent security issue.
But it is still can be possible to do in a background with a low priority.
Other action items for this integration:
1. Test that the HS DTrace are not broken on Solaris
2. Find or setup a Linux machine with the systemTap
3. Check that the fix is built Ok
4. Test that the fix works on Linux
Need a unit test for the fix (Mark, do you have any?)
Thanks,
Serguei
On 5/23/12 9:27 AM, Keith McGuigan wrote:
Hi Mark -
I'd prefer that it's done the "right" way (based on *my* definition of
"right", of course :) ), but I won't put up a fuss if whomever
shepherds this through agrees with you and wants to keep it in it's
current form. I expect that will be Serguei, or someone else from the
serviceability team? (Serguei?)
--
- Keith
On 5/23/2012 12:19 PM, Mark Wielaard wrote:
On Wed, 2012-05-23 at 10:23 -0400, Keith McGuigan wrote:
On 5/23/2012 9:56 AM, Mark Wielaard wrote:
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.
I wouldn't have mind if the OSX stuff was split out this way, but
now I
don't have any example to follow here. What/How do you suggest it is
done?
My first thought would be to create a dtrace_solaris.hpp file in
src/os/solaris/vm and a dtrace_linux.hpp file in src/os/linux/vm (and
maybe the same for bsd and windows?) for any OS-specific stuff, like
the
macro definitions and maybe even that solaris workaround. Leave
anything common in the original file (like the default empty
definitions
and such). Then you include the specific-file based on the target OS,
similar to what's done here:
http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/03d61caacd1e/src/share/vm/runtime/interfaceSupport.hpp
Yeah... it's a bit of a pain, and maybe in this case it's a little
overkill. But it's definitely nice to have a location for the
OS-specific stuff and it keeps the code rather neat.
So, do you require this change or not? I don't particularly like that
style (you loose the overview/a central place where all the magic macros
are defined), but I can certainly do it if you require it now. But then
I want to do it as additional patches on top of the current patches to
make sure they are separate. Since I will then have to touch code for at
least two architectures to which I don't have access (I also don't have
access to windows nor bsd, but I hope I don't have to touch those too).
So I want such changes to be testable separately by the architecture
maintainers.
Thanks,
Mark
