RVM

Refactor reflection to use VM interface

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 2.9.3
  • Component/s: Runtime: Class Library
  • Labels:
    None
  • Number of attachments :
    1

Description

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.

  1. File
    rvm-385.diff
    06/Mar/08 6:10 PM
    285 kB
    Andrew John Hughes

Issue Links

is depended upon by

Improvement - An improvement or enhancement to an existing feature or task. RVM-268 Update JikesRVM copies of classes (e.g. Constructor) to match the 1.5/current Classpath API

  • Minor - Minor loss of function, or other problem where easy workaround is present.
  • In Progress - This issue is being actively worked on at the moment by the assignee.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andrew John Hughes added a comment -

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

Show
Andrew John Hughes added a comment - Patch just committed to Classpath. I'm working on refactoring JikesRVM's interface to match.
Hide
Permalink
Andrew John Hughes added a comment -

Committed as revision 14003.

Show
Andrew John Hughes added a comment - Committed as revision 14003.
Hide
Permalink
Ian Rogers added a comment -

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)

Show
Ian Rogers added a comment - 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)
Hide
Permalink
Ian Rogers added a comment -

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.

Show
Ian Rogers added a comment - 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.
Hide
Permalink
Ian Rogers added a comment -

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

Show
Ian Rogers added a comment - I have a work around but the SVN repository appears to be down.
Hide
Permalink
Ian Rogers added a comment -

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

Show
Ian Rogers added a comment - Ah no, sourceforge decided to expire my password :-/ Fix in r14004. Passes pre-commit tests on IA32 Linux with a 1.6 Sun JDK.
Hide
Permalink
Andrew John Hughes added a comment -

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

Show
Andrew John Hughes added a comment - 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).
Hide
Permalink
Ian Rogers added a comment -

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.

Show
Ian Rogers added a comment - 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.
Hide
Permalink
Andrew John Hughes added a comment -

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

Show
Andrew John Hughes added a comment - Maybe, although I tested the new getParameterAnnotations method manually and that worked. Sorry to cause so many problems.

People

  • Assignee:
    Andrew John Hughes
    Reporter:
    Ian Rogers
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: