[cp-patches] Re: RFC: Class Loader patch to record classwithinitiating class loader

[Robert Lougher]

Hi,

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

> 
> Mark Wielaard wrote:
> > On Wed, 2005-07-27 at 13:18 +0200, Jeroen Frijters wrote:
> > > While digging around the class loading issues, I discovered that we
> > > didn't record a class with the "initiating loader" [1]. This is
> > > necessary to maintain type safety in the face of buggy or malicious
> > > class loaders (or even when the contents of the directories in the
> > > classpath change).
> > 

First of all, I've been up all night finishing off a release, I'm
tired and my broken wrist is very sore, and I'm grouchy.  I'm getting
the excuses in early, because I'm really, really unhappy with this.

Yes, I realised this problem over a year ago, and fixed JamVM then.  While
I was waiting for the VM interface to support it I overlayed ClassLoader,
but stopped earlier this year because I was unable to support the latest
snapshot and CVS head doing this (for various reasons I felt unable to
propose a patch myself).

> > With this patch I am not completely sure what the semantics of
> > VMClassLoader.USE_VM_CACHE are. If it is set to true
> > registerInitiatingLoader() is called everywhere, but not for
> > ClassLoader.defineClass(). This seems to complicate the VM interface a
> > bit since it looks like the runtime can do this optimization of
> > registering the initiating class loader everywhere itself, 
> > not just with VMClassLoader.defineClass().
> 

I agree with Mark here.  I think the Class/VMClass/VMClassLoader
interface is already too complex and this makes it worse.  Class is
trying to do too much, leading to lots of micro-methods:

VMClassLoader.loadClass(...)
VMClass.loadArrayClass(...)
VMClass.initialize(...)
VMClass.forName(str)

and now:

VMClassLoader.USE_VM_CACHE
VMClassLoader.registerInitiatingLoader()

All this for basically Class.forName(str, res, cl)!

In my opinion, the logic for deciding whether to load via the bootstrap
or via a classloader, whether to load an array or a "normal" class, 
initialization, and the managing of the cache is best done in the runtime
for performance reasons.

At the least the logic should be pushed into the VM reference classes,
for VMs that want Classpath to do the work for them.  At present,
the runtime can't do it itself, without adding a measure of redundancy.

> It is already possible to keep the table completely in the VM (in fact,
> this is what IKVM does), but you cannot get rid of the call in
> Class.forName() because the VM is not involved in that process.
> 

And the reason you can't get rid of it is because the logic is in
Class.forName (namely the call to classloader.loadClass()).

Trying not to be partisan, in JamVM VMClassLoader.loadClass(...),
VMClass.loadArrayClass(...) and VMClass.forName(str) all map down
to the same native function (forName(s, r, cl)), which does the
logic of Class.forName(s, r, cl) itself, and records initiating
loaders.  It was done this way before the VM interface matured, and
I've preferred to keep it.

It has the advantage that loadClass on a class loader will not be
called if the VM can tell from its cache that the class loader has
already defined the class, or is marked as an initiating loader
for it.

Flame away,

Rob.

> Regards,
> Jeroen
>