[cp-patches] Re: Some VMStackWalker cleanups/simplifications

[Robert Lougher]

Hi All,

Jeroen Frijters <jeroen <at> sumatra.nl> writes:

> 
> Mark Wielaard wrote:
> > I think that is a little strong.
> 
> I apologize if I come across too strong, it's just that I really care
> about doing the right thing (a little too much sometimes), so when
> someone proposes a IMNSHO bad idea, it sometimes affects me a little too
> much.
> 

I've been offline for a few days and so have missed this wonderful discussion.

Without casting stones, I would like to point out that JamVM and all the other 
VMs have endured quite a few changes to the VM interface recently that seem to 
have been for IKVMs benefit.  This may or may not be a bad patch, but the VM 
interface has to be a compromise between efficiency, robustness and simplicity 
even if it is not ideal for each and every VM.

> > I think it makes the interface a lot
> > easier, robust and hopefully more efficient to implement.
> 
> I have so many different problems with it that it is hard to discuss it
> for me. Let me start by saying that I'm pretty much convinced that
> passing the caller class isn't actually solving any problem, so I would
> appreciate it if you could try to explain exactly what problem it is
> that you are addressing. Second, I object to the notion that this makes
> the interface easier and more robust, because it allows a caller to pass
> in arbitrary classes, what is the defined behavior in such a case?
> 

OK, I think I know where the change comes from.  Last weekend I was working on 
getting JamVM to work with CVS and 0.13 at the same time.  Why you might ask?  
Because Classpath developers want a new release which they don't have to patch 
(4 patches are currently needed), but it also needs to work with the latest 
snapshot for "ordinary" users.  Perhaps I should just release 2 versions, but 
it seemed to be possible to have just one version apart for one problem...

The change to getClassContext() when it was moved from VMSecurityManager to 
VMStackWalker.  The old version was responsible for popping off TWO frames (in 
addition to those for reflection).  The one for VMSecurityManager AND the one 
for SecurityManager itself.  The new one only pops off the one for itself, i.e. 
VMStackWalker.

This is the kind of change that Marks' proposal was designed to fix.  Implicit 
stack depth assumptions, which will break if the calling order changes.  And as 
for efficiency, take a look at the implementation of 
SecurityManager.getClassContext() using VMStackWalker.  Because 
VMStackWalker.getClassContext() no longer pops off the frame for 
SecurityManager it has to do an array copy into a new array.  Admittedly, this 
method has mostly been replaced by the more efficient ones, but it indicates 
the interface is not optimal.

Any easy way to fix this problem is to replace the Class argument with a count, 
2 frames to pop for SecurityManager, 1 everywhere else.  This could be a stupid 
suggestion, I haven't given it a lot of thought.

I would like to have an agreement reached soon, as I have JamVM 1.2.4 ready to 
release which currently uses the new interface.  I'll just release two versions 
(one for 0.13 and one for CVS) if it's dropped.

Regards,

Rob.

> > > Of course it adds overhead, especially with a 1.4 compiler.
> > 
> > Aha. OK, I see where you are comming from. I assumed SomeClass.class
> > would be compiled efficiently, and I think at least jikes 
> > gets it right by caching the result, but I see gcj doesn't do that and
> 
> > actually calls Class.forName() each time (eep).
> 
> That's not really what I meant. My main concern is that it is very hard
> to statically determine the class that is passed and thus it's hard to
> optimize it away. With a 1.5 compiler at least that would be easier.
> 
> > So I should actually change the patch
> > so that it really is a fixed parameter at each callsite by
> > precalculating it. Will do that.
> 
> Please don't.
> 
> Regards,
> Jeroen
>