[#RVM-385] Refactor reflection to use VM interface

RVM

Refactor reflection to use VM interface

Created: 02/Mar/08 10:45 AM Updated: 09/Mar/08 09:11 PM

Component/s:

Runtime: Class Library

Affects Version/s:

None

Fix Version/s:

2.9.3

Time Tracking:

Not Specified

File Attachments:

rvm-385.diff (285 kB)

Issue Links:

dependent

This issue is depended upon by:

RVM-268 Update JikesRVM copies of classes (e.g. Constructor) to match the 1.5/current Classpath API

Description

« Hide

Currently at the RFC stage, Andrew Hughes has separated out the reflection code into VM and general purpose. Here's his post from classpath-patches:

This patch separates out common methods from the reflection VM
interfaces (Field, Constructor, Method) so that they are provided
in Classpath itself, and only native methods now appear in
new VMField, VMMethod and VMConstructor classes in the VM interface.
There is a lot of duplicate code in these classes that is currently
left to the VM to maintain, resulting in large parts of these files
in the VM being verbatim copies of the reference implementation and
dependent on package-private Classpath methods.

and the patch that was the original motivation:

In doing the work on CPStringBuilder, I noticed that many of our VMs
use code verbatim from the
java.lang.reflect.{Field,Constructor,Method} classes when they all
broke in the same way. This suggests we should have a VM separation
here and I propose to add this in this evening.

For now, the patch to make a VM work is fairly trivial. I've attached
ones for CACAO, JikesRVM, JamVM and CACAO.

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Andrew John Hughes added a comment - 03/Mar/08 01:15 PM

Patch just committed to Classpath. I'm working on refactoring JikesRVM's interface to match.

[ Permlink | « Hide ]

Andrew John Hughes added a comment - 06/Mar/08 06:10 PM

Committed as revision 14003.

[ Permlink | « Hide ]

Ian Rogers added a comment - 07/Mar/08 02:57 AM

I'm looking into regressions across the board since r14003:

http://jikesrvm.anu.edu.au/cattrack/results/rvmx86lnx32.anu.edu.au/perf/3016/regression_report

here is part of an example stack trace:

Compiler failure during compilation of < SystemAppCL, LEDU/purdue/cs/bloat/util/Graph$4; >.hasNext ()Z
java.lang.NullPointerException
at java.lang.reflect.VMConstructor.getModifiersInternal(VMConstructor.java:66)
at java.lang.reflect.Constructor.getModifiers(Constructor.java:135)
at java.lang.reflect.Constructor.toString(Constructor.java:223)
at java.lang.StringBuilder.append(StringBuilder.java:319)
at org.jikesrvm.compilers.opt.driver.CompilerPhase.newExecution(CompilerPhase.java:137)

I believe this could be a boot strap problem (ie we don't have VMConstructors for Constructors that are in the boot image)

[ Permlink | « Hide ]

Ian Rogers added a comment - 07/Mar/08 04:23 AM

Blergh, this has broken things in a nasty way for the opt compiler. Our previous java.lang.Constructor had a reference to a VM_Method which was the constructor. In the opt compiler we have Constructor classes to build the atomic elements of the optimization plan (I'm not a fan of this structure but it was a necessary evil to get rid of a bigger evil of using .clone that was causing the opt compiler not to be multi-thread-able). I used the oracle methods in the boot image writer, in this case copyKnownClasspathInstanceField at line 2168, to patch up the constructor/VM_Method field of the java.lang.Constructor as it got written out to the boot image. The problem now is that instead of a VM_Method the constructor holds a VMConstructor reference and making VMConstructors within the boot image writer is non-trivial as they don't exist in the boot image writer's java.lang and are also package protected. I may be able to fix these problems but it's going to depend on how nicely reflection wants to play. What's certain at the moment is that we fail pre-commit tests.

I'm somewhat concerned that this changes are going to give a performance regression. In particular the references can't be chased from Constructor to VMConstructor.

I'm worried that fixing this problem is going to cause a delay in which we get no regression results and therefore have to freeze code being added.

[ Permlink | « Hide ]

Ian Rogers added a comment - 07/Mar/08 06:27 AM

I have a work around but the SVN repository appears to be down.

[ Permlink | « Hide ]

Ian Rogers added a comment - 07/Mar/08 06:31 AM

Ah no, sourceforge decided to expire my password :-/ Fix in r14004. Passes pre-commit tests on IA32 Linux with a 1.6 Sun JDK.

[ Permlink | « Hide ]

Andrew John Hughes added a comment - 07/Mar/08 08:29 AM

I ran the pre-commit tests before committing and they passed.

VMConstructor is now the equivalent of the old Constructor. It should largely be a case of swapping Constructor for VMConstructor (i.e. it contains the VM_Method).

[ Permlink | « Hide ]

Ian Rogers added a comment - 07/Mar/08 08:32 AM

There must have been something funny in your world, the tests would only have passed for the opt compiler if the java.lang.Constructor you were actually testing were the old one.

[ Permlink | « Hide ]

Andrew John Hughes added a comment - 07/Mar/08 11:13 AM

Maybe, although I tested the new getParameterAnnotations method manually and that worked.
Sorry to cause so many problems.