incubator-kato-spec mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steve Poole <spoole...@googlemail.com>
Subject Re: Apache Kato release - comments please
Date Mon, 30 Nov 2009 11:32:01 GMT
Konst - by replies to you comments below.


On Wed, Nov 25, 2009 at 1:55 PM, Bobrovsky, Konstantin S <
konstantin.s.bobrovsky@intel.com> wrote:

> 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.
>
> That does make sense -  perhaps we should also consider adding in a
getType()  method ?

=== 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").
>
> Thanks - you've given as an easy way out :-)


> === 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()
>
> This makes sense too -  though I think it would be better to  have
something like this instead :

public ImagePointer getOffset(JavaObject )

Gives you more flexibility in the implementation and returns something
you're going to need anyway.   The ImagePointer can then be used to look at
the contents of the object directly.


> === 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()
>
> Agreed -  perhaps its should be called  getProgramCounter()  or something
similar?

2)
> getCompilationLevel() suits more to the piece of code which encloses this
> JavaLocation, and does not seem to fit logically well here
>
> Agreed -  see comments later about this.


> 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
> }
>
> Konst - I'm not sure I really understand the benefit of this item over what
already exists.  Could you start a separate thread and provide some more
info about sort of usecase you had in mind?


> === 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()
>
> Understood -  this is tied up with the item above so we can resolve them
> together.
>


> === 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.
>
> Agreed - this would be very 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.
>
> Thats a good idea. Do we need both?   What about levels of compilation.
You suggested moving getCompilationLevel() to here so can't we use that with
perhaps 0 = interpreted?



> 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
>
> ok - thanks for spotting that.


> 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();
>
> I understand why you want this  - is it only restricted to references?
Are there other places where knowing which register was used is important?

   /** @return  a location within this image where the reference's target is
>     * located or null if such information is not available */
>    public ImagePointer.getImageLocation();
>
> Agree with this idea  -  I would probably go with something a little more
generic though :

     public ImagePointer getAddress();



> 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/...'
>
> Understood


> 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.
>
> Understood

> 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
>
> Agreed

> 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
>
> Agreed
>


> >-----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/<http://people.apache.org/%7Emonteith/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/
>
>


-- 
Steve

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message