incubator-kato-spec mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bobrovsky, Konstantin S" <konstantin.s.bobrov...@intel.com>
Subject RE: Apache Kato release - comments please
Date Wed, 25 Nov 2009 13:55:47 GMT
Hi Stuart, all

As we are approaching the end of the public review, I once again reviewed the specification,
the results are below. These are some my older comments which have not yet been anyhow resolved
plus few new. I could volunteer to develop some of the suggestions further and prepare a complete
source updates for the parts which I suggest to change. BTW, I also entered these comments
into the JCP comments page for this JSR:
http://wiki.jcp.org/boards/index.php?t=2852

Thanks,
Konst
 
Intel Novosibirsk
Closed Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park, 
17 Krylatskaya Str., Bldg 4, Moscow 121614, 
Russian Federation

=== interface ImageSymbol ===
1)
Add 'public int getSize()' method which would return a number of bytes occupied by the symbol
in the image.

=== interface ImageStackFrame ===
1)
API also needs to support 'split' stack frames: e.g. on Itanium every frame consists of a
traditinal memory stack frame and additionally, a register stack frame where Register Stack
Engine (RSE) spills current register frame during a call (and restores from upon return).
The easiest way probably is to mention in the spec something like "implementations can extend
this interface to provide data specific to a particular platform, such as register stack frame
location on Itanium (r), or frame size").

=== interface JavaField ===
1)
During VM crash debugging it is sometimes important to know the offset of a particular field
from object start. So, something like
	/** @return for instance fields, offset of the field from object's beginning in
* the memory; -1 if unknown or for static fields */
public int getOffset()


=== interface JavaLocation ===
1)
The spec says that it "Represents a point of execution within a Java method", however it does
not provide access to bytecode index of currently executed Java instruction, which is the
only available (sensible) current Java code position description for interpreted frames, and
an invaluable complement for compiled method's current code address. So, I would suggest to
also have
	/** @return bytecode index corresponding to position in the code represented by
* this JavaLocation instance */
	public int getBytecodeIndex()

2)
getCompilationLevel() suits more to the piece of code which encloses this JavaLocation, and
does not seem to fit logically well here

3)
JavaMethod getMethod() does not provide easy way to identify piece of code this location belongs
to. Another method
	/** @return a description of a logical chunk of code this location belongs to */
	public CodeStub getContainingCodeStub()
would achieve this. Additionally, I think that a "piece of generated code" is important concept
within any JVM to dispense a specific interface for it - CodeStub. 

  interface CodeStub {
    String getName();
    ImageSection getCode();
  }
CodeStub which a compiled method would extend from it:

CompiledMethodCode extends CodeStub {
	JavaMethod getMethod();
	int getCompilationLevel();
	/** @return (for OSR compilations - bytecode index of the entry point into
 * this method) */
	int getEntryPointBytecodeIndex();
	// plus maybe GC maps (all implementaions) and other implementation-specific info
}

=== interface JavaMethod ===
1)
	/** Get the set of ImageSections containing the compiled code of this method. */
List getCompiledSections()
          

I would suggest to change to 
	/** Get the set of CompiledMethodCode objects containing the compiled code of
 *  this method. */
List getCompiledSections()


=== interface JavaStackFrame ===
1)
Usually, Java stack frames contain information about Java monitors held in this frame. This
is necessary because Java spec requires "forced" release of all monitors held by a method
in case it "completes abruptly". So something like
	/** @return list of JavaObject instances representing monitors held by
 * this method activation */
List getHeldMonitors()
would be really useful.

2)
Important case of JavaStackFrame is interpreted frames - in this case current position in
the machine code is not that important (this position is not unique to a method) as current
position in the bytecode being executed. So it would be good to distinguish compiled frames
from interpreted ones (IMO, this is implementable for any JVM)
	public boolean isCompiled();
	public boolean isInterpreted();
In case of interpreted frames, more information might be available.

3)
It would be of great help to an engineer investigating a crash in compiled code or some Java-level
runtime error to know the JVM virtual state (in JVMS sense) for all Java stack frames, i.e.:
-	held monitors
-	current bytecode index
-	values of all local variables
-	content of the expression stack

and some JVMs can provide such info even for compiled frames. I think it is desirable to add
something like (removing getHeldMonitors):

	public JVMState getCurrentJVMState();


=== interface JavaReference ===
1)
HEAP_ROOT_*, REFERENCE_* and other constants are omitted from the PDF document 

2)
This API defines heap roots returned by getHeapRoots queries (e.g. JavaStackFrame.getHeapRoots)
to be JavaReferences. Sometimes it is very helpful to know where the heap root actually resides
- the corresponding CPU register and/or memory address (in a memory stack frame or register
stack frame), and this info is usually available in Java runtimes. Something like

    /** @return a CPU register name where the reference's target is located or
     * null if such information is not available */
    public ImageRegister getRegister();

    /** @return  a location within this image where the reference's target is
     * located or null if such information is not available */
    public ImagePointer.getImageLocation();

would be helpful.

=== interface JavaRuntime ===

1) It should be possible in the API to iterate trough generated code sequences other than
managed methods - so-called 'stubs'. Each stub is similar to a method: it has a name, (possibly,
multiple) code sections. This interface (JavaRuntime) seems an appropriate place for this
API:
	/** @return  a list of CodeStub objects representing code sequences
 * generated internally by the JVM */
	public List getJVMCodeStubs();

(2) Not all methods within a Java runtime are necessarily compiled. Many may remain interpreted,
others might not even have a bytecode. So I would suggest to replace 'getCompiledMethods'
with
	/** @return a list of JavaMethod objects representing managed methods known
 * to the system. Each JavaMethod may be either interpreted or compiled or
   native. Method's code for compiled methods can be quired via @see
  #JavaMethod.getCompiledSections() */
public List getJavaMethods();

=== Common Register Names ===
1) 
'Long' or 'Integer' is too vague for specifying register type. It is better to specify the
width of the register - 256 (future Intel architectures) / 128 (SSE registers) / 80 (FP registers
in ia32, Intel64, IA64)/ 64 / 32 bit - plus an additional characteristic such as 'general
purpose/floating point/application/flags/...'

2)
The table title "Table A.2. IA64 Register Names" is wrong, it should read either "Intel64
Register Names" or "AMD64 Register Names", as "IA64" is Itanium architecture.

3)
For the complete list of recommended register names and their specifications for ia32 and
Intel64 architectures I suggest add a link to

Intel(r) 64 and IA-32 Architectures Software Developer's Manual (September 2009), Volume 1:Basic
Architecture, Sections "3.4 BASIC PROGRAM EXECUTION REGISTERS" and "3.5 INSTRUCTION POINTER"
http://www.intel.com/Assets/PDF/manual/253665.pdf

4)
I suggest to add a new section "IA64 Register Names" with a link to the official Itanium(r)
2 specification for the recommended IA64 register names and their specifications, which is

Intel(r) Itanium(r) Architecture Software Developer's Manual (rev 2.2), Volume 1: Application
Architecture, Section 3.1 "Application Register State"
http://download.intel.com/design/Itanium/manuals/24531705.pdf


>-----Original Message-----
>From: Stuart Monteith [mailto:stukato@stoo.me.uk]
>Sent: Friday, November 20, 2009 11:29 PM
>To: kato-dev@incubator.apache.org
>Subject: Apache Kato release - comments please
>
>Hello all,
>     Steve and I have been working on getting the codebase ready for a
>release. This is a non-trivial activity with lots of things to get right.
>
>Given the state of the project as it is just now, we can't claim to be
>doing a 1.0 release, instead I'm following the Milestone convention.
>
>I'm not proposing that what I've assembled is submitted as a release
>candidate just yet, as there is still work to do on it.
>
>If you look here, you'll find the files that would make up a release:
>     http://people.apache.org/~monteith/kato/apache-kato-M1-incubating-RC1/
>
>I've also created a tag:
>
>https://svn.apache.org/repos/asf/incubator/kato/tags/apache-kato-M1-
>incubating-RC1/
>
>
>Obvious things that are wrong:
>     apache-kato-M1-incubating-tomcat.demos - packages are undocumented
>and missing some files.
>     The documentation - it needs more work, but I'd be interested in
>some cursory feedback.
>     The CJVMTI agent needs work under Linux - it's missing some
>information.
>     The TCK is still under development.
>     Some filtering needs to be applied to the rat reports.
>     There are some spurious files, like readme.html.
>
>I have found this a very useful exercise, as this is the first that
>we've packaged the projects contents and seen what it actually includes,
>so this should give everyone a good opportunity to have a look and see.
>
>
>Regards,
>     Stuart
>
>--
>Stuart Monteith
>http://blog.stoo.me.uk/


Mime
View raw message