Primitive write barrier support

Thanks, looks like a nice addition!

I took a quick scan through the "rvm" part of the patch. It looked plausible to me, with the exception of the addition of the CHAR_ASTORE operator for the opt compiler. Since the sign/zero extension doesn't matter for stores, we uniformly collapse the signed/unsigned version of the store operations into a single operator. BURS still should be able to avoid sign extension for the store(load) pattern (perhaps there is just a missing rule somewhere??).

Steve/Daniel, could one of you take a look at the MMTk part of the patch and comment? I don't mind doing the leg work of committing it and changing the opt compiler part to drop CHAR_ASTORE, but I want one of you to sign off on the MMTk changes first.

thanks,

--dave

I think you can handle the CHAR vs. SHORT ASTORE by in ExpandRuntimeServices getting the store instructions LocationOperand (AStore.getLocation()) and then getting the arrayElementType from it to distinguish between shorts and chars to figure out what to call. A little more cumbersome in the expansion code, but it avoids introducing an operator and is more consistent with how we are doing other store operations (byte vs. ubyte, etc).

Following Dave's comments this patch removes the need for a CHAR_ASTORE operator by getting the element type for 16 bit array stores

Any feedback from the MMTk side on this patch?

Daniel and I have both had a look.

1. We don't want to commit it without a configuration from which we can build a straightforward test for it---we don't want it in the head untested. Laurence, you say above that you have a configuration that depends on this patch. Is it suitable as the basis for a regression test? Can you make this available to us please?

2. We'd like to clean up the API for barriers (into MMTk and MemoryManager). I'll try to do that over the next day or so .

3. Hopefully with the above fixed we can have this patch committed soon.

Hi Steve and Daniel,

Thanks for taking a look at this. In response to your comments:

1) Currently I have two patches that provide some sort of testing for primitive write barriers (although I don't believe either are suitable):

i) A GC that extends SemiSpace and declares the needsPrimitiveWriteBarriers constraint. The mutator class provides primitive write barriers which make a straight write to the heap. The rest of the GC is deferred to the parent classes. This test checks that the primitive write barrier code compiles but does not provide any assurances that the compiler has not optimised away parts of the barrier. There's no reason this test couldn't work with say the Immix collector and I'm happy to contribute the patch, but I think its only of very limited value.

ii) Steve, we discussed my second patch briefly at ISMM09 and is much more intrusive - making the default allocation space be READ_ONLY. All primitive and reference writes to the heap occur via a mmap'd page aliased to the heap. Unfortunately this impacts code in BumpPointer, MonotonePageResource and a whole load of other places that store metadata directly into the heap. This patch provides very strong reassurances that barriers are correctly compiled but execution speed is too slow to be useful as a regression test.

Having thought about this I think a more suitable test would be as follows:

• A simple Java application (PrimitiveWriteBarrierTestApplication) that declares a public volatile field for each primitive type
• Add a pickAllocator(byte[]) method to Plan that receives a byte array describing the type to be allocated, this method should return ALLOC_DEFAULT
• Extend MemoryManager.pickAllocator(RVMType) to call Plan.pickAllocator(byte[]) - giving individual GC plans a mechanism to control where allocation should happen
• Add a new PrimitiveWriteBarrierTest GC plan - this plan can extend the NoGC plan or any other GC plan, extending a Plan that actually does GC will allow this test to run normal benchmarks
• This plan should instantiate an additional space (PrimitiveWriteBarrierTestApplicationSpace)
• This plan should override pickAllocator(type[]) and alloc() such that the PrimitiveWriteBarrierTestApplication class is allocated into the dedicated PrimitiveWriteBarrierTestApplicationSpace, this space should only be for this type, all other objects should be allocated into their normal space
• The plan should declare the needsPrimitiveWriteBarriers constraint and provide a suitable implementation
• If the object is in any space other than the PrimitiveWriteBarrierTestApplicationSpace - perform the primitive write as normal
• If writing to the PrimitiveWriteBarrierTestApplicationSpace then manipulate the value before storing it (maybe add 1, maybe complement)
• The PrimitiveWriteBarrierTestApplication performs a series of writes to its primitive fields and then reads the fields back, the test expects to read back a different value to the value written (the fields need to be marked volatile to ensure the opt compiler doesn't just remove the read). If the read value does not match the expected value then either the test is being run under a different GC plan, or the barrier code is faulty.

This approach provides a stronger assurance that the primitive write barriers are being correctly compiled than the first suggested patch. It does not catch all primitive writes to the heap like the second patch, but does handle the desired case of putfield's.

I have not yet implemented this patch (although it's not a lot of work) - I thought I would suggest it first to make sure that it would be an acceptable test case. If you have any alternative suggestions then it would be great to discuss their relative merits and come to a consensus. I'm quite happy to write the test case once it's agreed on how we should test this.

2) You commented on tidying up the API for barriers - as best I remember I followed the current API for reference barriers when adding the necessary primitive write barrier code. Whilst I have no objections to refactoring the code I am just curious if I messed something up or if this patch provides a convenient point for some wider MMTk and MemoryManager refactoring?

3) Great!

Kind regards
Laurence

Hi Laurence,

w.r.t. 2), no you've done nothing wrong; your patch just highlights that the existing API is creaky.

w.r.t. 1), Daniel has built and tested a nice general system for testing barriers (based on fine grain protection offered by a virtualization layer), which I suggest we move to ASAP. However, he is not ready to commit this just yet (needs to be tidied, documented and properly integrated) and he's in the final throes of writing up his PhD, so it won't be ready in the near term. So I suggest that for the short term we go for your more basic (and as you note, more limited) test of a simple SemiSpace collector that happens to utilize these barriers.

As to the overall approach from here, what I want to do right now is:

a) I do some basic re-factoring of the underlying infrastructure into the head ASAP
b) Together we get your system working with the new head
c) We get a basic regression test working
d) We commit b) & c)
e) Daniel gets his system committed
f) We introduce a comprehensive regression test for all barriers

I want to get to d) ASAP. Hopefully we'll get to f) within a couple of months.

Hi Steve,

Please find attached a simplistic test collector (extending SS) that uses primitive write barriers to write into the heap

We need to decide what an appropriate regression test is - probably nothing more than checking that both the Base and Opt builds compile and maybe run _200_check correctly?

I look forward to you committing your refactoring. Once your happy with the API in trunk if you could give me a nod and I'll rework my patch.

Kind regards
Laurence

Here is a first rough stab at the refactoring I'd like to see.

There are really only two substantive changes:

1. Move barrier code out of MemoryManager into a new Barriers class.

2. Rationalize the naming of barriers, prefixing reference barriers with "reference" so that this naming convention can be trivially replicated for each of the other types, with the prefix suitably replaced by the type name.

This patch should be pretty close to ready to go. Aside from some minor cleanup, the only remaining task is to pad it out with stubs for all of the different types.

Laurence: feel free to take a look, but don't bother patching against this yet because it is not final, so you could end up wasting effort. I hope to have something finalized within a day or two.

Others: please let me know if you'd like to see a different refactoring (I discussed this with Daniel, but will be interested to hear if anyone else has alternative ideas).

A few minor changes and inclusion of the MMTk harness. I've not had much luck in testing the harness. Robin, if you're able to do so, it would be great if you could test this.

I would like to commit this (or something similar) very soon.

The next step is do the trivial cookie-cutter duplication of the code for reference types (included in this patch) to pad out for each of the primitive types. I want some consensus that I've got it basically right for reference types before doing that, though.

The MMTk harness works fine with this patch - I've committed a trivial change to test-mmtk to make it less platform sensitive.

There seems to be a bug/inconsistency in the VM.barriers.wordRead method - the barrier itself takes 5 parameters, src, slot, metaDataA, metaDataB, mode, but the wordRead method doesn't have the slot parameter and assumes that metadataA is the slot. Given that the metadata is a workaround, I think the slot parameter should be explicit, but in either case the Poisoned collector is broken.

One minor quibble is with the names of the constants INSTANCE_LOAD and INSTANCE_STORE. I'd prefer GETFIELD and PUTFIELD (consistent with GETSTATIC/PUTSTATIC), but i guess INSTANCE_FIELD_LOAD/STORE would be OK too.

THanks Robin.

I don't think the methods are inconsistent, although they did need cleaning up. In the case of the harness, we use one of the available metadata parameters to pass the slot, which is not required in the Jikes RVM case.

One thing that is perhaps slightly mysterious is that we no longer have writes to statics as a special case of the other writes, but rather we have a separate API "nonHeap**" which is used for such read and writes, and more generally for any reads or writes to non-heap regions that needs to be barriered (perhaps a stack barrier, for example). This API takes a slot rather than a reference as the argument.

The underlying theme above is to move MMTk away from strong binding to Java, so we no longer use "putfield/getfield" etc on the MMTk end, though we do on the Jikes RVM end.

Furthermore, on the Jikes RVM end we separate out the possibilty of generic VM barriers from GC-required barriers. There are cases where the VM may require a barrier for purposes that don't relate to the GC. This is now pretty cleanly expressed.

Your comment about the GETSTATIC etc variables is valid, it was an oversight. Fixed now.

I've gone ahead and committed, but if anyone is unhappy or can see clear points for improvement, just shout and we can fix it up.

Once everyone is content, I'll do the process of expanding the API out to all the primitive types, which will now be trivial cookie cutter work.

Laurence, I believe that I have finally finished all the re-working of the plumbing (see RVM-589).

If you're able, could you please post a new patch relative to the head?

Hi,

Please find attached a new version of my patch designed to apply to trunk.

Steve: I have since thought about the BulkCopy changes we discussed and have taken the liberty of implementing one possible refactoring (see details below). If this change is completely inappropriate then I can revert it from the patch before it is applied - otherwise the patch could be applied to trunk and then any small tweaks committed later.

Begin possible commit message:

This patch completes compiler support for primitive write barriers (see RVM-845 and RVM-859) and adds a simplistic MMTk collector that uses primitive write barriers:

• Add Baseline and Opt compiler support for write barriers on primitive putfields and primitive array stores (ppc and ia32)
• Add a new MMTk collector that extends SemiSpace and uses primitive write barriers - this collector provides some simplistic assurances that the primitve write barrier code is correct
• Add locationMetadata to various Magic methods and update code to pass this metadata when known
• Object cloning, field reflection, sun.misc.Unsafe.* and Synchrornization code has been updated to use primitive write barriers when required
• Support for atomic exchange of int, long and Word fields when either field stores or reads requires a barrier
• Fix some Javadoc errors introduced during the recent MMTk refactoring

The final change that this patch makes is the handling of <type>BulkCopy. Previously objectReferenceBulkCopy was only called by RVMArray.arrayCopy if object array store barriers were required and array load barriers were not required. In a conversation with Steve Blackburn and Daniel Frampton it was agreed that this optimisation was not necessarily intuative and it was agreed that BulkCopy should be unconditionally called if either a array load or store barrier was required. This patch refactors all BulkCopy methods so that they are called during an array copy if a store or load barrier is required. The refactored BulkCopy methods use a enum to encode the return value and encode how the array copy should be made. Current possible return values are:

BulkCopyState.COPIED - the mutator BulkCopy method made a copy of the array (hopefully in an optimised fashion), no further work is required
BulkCopyState.FAST_COPY_STATE - no further barriers need to be triggered during the actual copy, it is safe to attempt an memcopy of the target to destination array (if conditions about memcopy are met)
BulkCopyState.MUST_SLOW_COPY - array copy must be by made reading and writing each array element, it is not safe to attempt a memcopy

Thanks for the patch Laurence.

As I discussed with you in response to your prior email (should have been here rather than email), I am planning to refactor bulk copy, and I will still do so---I established what I think is the right solution and will update the code as soon as I can.

What I plan to do now is to make mm.Barriers.<type>BulkCopy be called unconditionally whenever the VM needs read or write barriers. The optimization (as I described below) will then conditionally occur within mm.Barriers.<type>BulkCopy if the GC supports the optimization. Otherwise the naive (and always correct) element-by-element copy will occur. However that conditional logic is not visible to the VM code (which happens to only arise in one place: RVMArray).

I hope to do that over the next few days if possible (had hoped to do it today but am hopelessly behind on other things). However, you should feel free to just go and refactor your patch against the existing code. I'm happy to do my fixes after your patch comes in if you beat me to it (ie I'll "fix" your code for you if I've not done the refactoring before you get yours done).

Also, I want to keep that particular change out of your patch because it is a separate concern. I hope to have it done in the next few days.

This patch removes my proposed bulkCopy refactoring from my previous patch but otherwise provides the same functionality. RVM-861 (bulkCopy refactoring) will need to ensure that RVMArray.arrayCopy calls the correct bulkCopy methods when appropriate and define these methods in UsePrimitiveWriteBarrierMutator

Begin possible commit message:

This patch completes compiler support for primitive write barriers (see RVM-845 and RVM-859) and adds a simplistic MMTk collector that uses primitive write barriers:

• Add Baseline and Opt compiler support for write barriers on primitive putfields and primitive array stores (ppc and ia32)
• Add a new MMTk collector that extends SemiSpace and uses primitive write barriers - this collector provides some simplistic assurances that the primitve write barrier code is correct
• Add locationMetadata to various Magic methods and update code to pass this metadata when known
• Object cloning, field reflection, sun.misc.Unsafe.* and Synchrornization code has been updated to use primitive write barriers when required
• Support for atomic exchange of int, long and Word fields when either field stores or reads requires a barrier
• Fix some Javadoc errors introduced during the recent MMTk refactoring

The patch defines 2 new build configs i) ExtremeAssertionsBaseBaseUsePrimitiveWriteBarriers and ii) ExtremeAssertionsOptAdaptiveUsePrimitiveWriteBarriers both use the simplistic collector based on SemiSpace that merely uses primitive write barriers to complete the writes to the heap.

Hopefully the patch is now suitable for inclusion in trunk

Kind regards
Laurence

I had a shot at applying the latest patches, but I'm not entirely happy with it. Can you please help me with the following?

You have introduced barriers on magical types (Word). Why? I don't understand this and think it is wrong. A related change I bumped into disturbed me:

   public static Address fetchAndSubAddressWithBound(Object base, Offset offset, int decrement, Address bound) {
-    Address oldValue, newValue;
+    Word oldValue, newValue;
     if (VM.VerifyAssertions) VM._assert(decrement > 0);
     do {
-      oldValue = Magic.prepareAddress(base, offset);
-      newValue = oldValue.minus(decrement);
-      if (newValue.LT(bound)) return Address.max();
-    } while (!Magic.attemptAddress(base, offset, oldValue, newValue));
-    return oldValue;
+      if (Barriers.NEEDS_WORD_GETFIELD_BARRIER) {
+        oldValue = Barriers.wordFieldRead(base, offset, 0);
+      } else {
+        oldValue = Magic.getWordAtOffset(base, offset);
+      }
+      newValue = oldValue.minus(Word.fromIntZeroExtend(decrement));
+      if (newValue.toAddress().LT(bound)) return Address.max();
+    } while (!tryCompareAndSwap(base, offset, oldValue, newValue));
+    return oldValue.toAddress();
   }

Perhaps a few things are going on here....

1) Perhaps what you want is a use barrier, which is your motivation for including magical types (hard otherwise to understand why you would barrier the magical types).

I think that if what you want is a use barrier, then ultimately we should try to do that properly, not this way. However, I'm fine with doing it this way in the intrum, but it would be good to have that motivation clear. Otherwise it is strange to be putting barriers on the magical types.

2) Ultimately the word width magical types (except for ObjectReference) are equivalent, so you can use them interchangeably. However, this is not in the spirit of their design (you could in the limit get rid of them all and just have Word and ObjectReference).

So I think the right thing to do is to have explicit barriers for the necessary types and try to preserve the distinctions between the word-width magical types as far as possible (ie avoid changes like the one quoted above, and instead have appropriate ones for the types, just as we have explicit separate barriers for short and char which are at some level equivalent).

I am happy to push through with adding Address and Word to the API and plumbing through the barriers. I will try to do that over the next few days.

Added Word and Address stubs in r15795.

Hi Steve,

(Sorry for the delay in replying)

> 1) Perhaps what you want is a use barrier, which is your motivation for including magical types (hard otherwise to understand why you would barrier the magical types).

Yes absolutely, this was our motivation, the potential to barrier the use (read or write) of any field type stored in the heap which includes the unboxed types Word, Address et al

> 2) Ultimately the word width magical types (except for ObjectReference) are equivalent, so you can use them interchangeably. However, this is not in the spirit of their design (you could in the limit get rid of them all and just have Word and ObjectReference).

I agree that the code you flagged up as "disturbing" was a nasty hack on my part, this code can now be rewritten much more cleanly based on your addition of barriers for Address

> So I think the right thing to do is to have explicit barriers for the necessary types and try to preserve the distinctions between the word-width magical types as far as possible (ie avoid changes like the one quoted above, and instead have appropriate ones for the types, just as we have explicit separate barriers for short and char which are at some level equivalent).

Agreed, over the next few days I shall modify TypeReference and the compilers such that we can distinguish putfields and getfields to Word, Address, Offset and Extent fields. The addition of Offset and Extent barriers should then ensure that all types have their own barrier.

Are their any other changes that you believe are necessary? I aim to rework my patch to incorporate all of the above points.

Kind regards
Laurence

Hi Steve,

Based on your barrier padding out work please find two patches attached that I hope will complete RVM-845:

1) RVM-845-20091211.patch.0

This patch is based on r15796 and completes the work of adding primitive write barrier support to MMTk and the RVM. On the MMTk side of things:

• Add stub implementations to MutatorContext for reading, writing and bulk copying Extent and Offset fields or array's
• Add constraints to PlanContraints to support the above
• Add stub implementations to MutatorContext for atomicially updating a int, long, Address or Word fields in the presence of read or write barriers
• Various Javadoc changes including capitalising types (e.g. address -> Address) and fixing existing cut and paste errors

On the MMTk / RVM interface side of things:

• Add support for MMTk calling back to the RVM to actually perform the Extent / Offset read or writes
• Where known MMTk now passes FieldReference location metadata to the RVM to assist in stores
• Implementations of atomic int, long, Address and Word updates
• Various Javadoc changes including capitalising types (e.g. address -> Address) and fixing existing cut and paste errors

On the RVM side of things:

• Replicate Magic.set* methods to provide support for storing all types (primitive, Magic and Object) without having to resort to type abuse
• Provide alternative versions of Magic.set* methods that also take location metadata (which the opt compiler can use)
• Provide methods in TypeReference to distingish if a type is a Word, Address, Extent or Offset
  • This means that the existing TypeReference.isWordType() method which returned true if the type was any unboxed scalar type had to be renamed isWordLikeType
• Modify the ia32-base, ppc-base and opt compilers to check if primitive or unboxed write barriers are required on putfields and array stores and call the appropirate barrier if required
• Modify sun.misc.Unsafe methods, RVMField reflection code and object cloning code to use primitive write barriers if appropirate to perform stores
• Modified org.jikesrvm.mm.mminterface.Barriers:
  • Src and dst parameters were the wrong way round for most primitive bulkCopy barriers
  • charArrayWrite shifted index by LOG_BYTES_IN_ADDRESS rather than LOG_BYTES_IN_CHAR
  • Removed Word and Address array load, store and bulkCopy support (added in r15795) - Word[] and Address[] are types that should not be used within the RVM see org.vmmagic.unboxed.WordArray etc for alternatives. This does not remove any functionality from MMTk, nor is it due to any MMTk limitations, it simply removes glue code from the RVM that would never be called
  • Glue code needed to support Extent and Offset field writes (as called by the compiler)
  • Glue code to support int, long, Address and Word atomic updates (as called by Sychronization.java)
• RVMArray arrayCopy methods should pass the number of bytes to be copied (not the number of elements) to the bulkCopy barriers
• Modify org.jikesrvm.scheduler.Synchronization
  • To use primitive write barriers if needed when modifying a field (the cause of needing to add int, long, Address and Word atomic barriers)
  • Removed several methods such as fetchAndAddAddress, these methods were a) Unused, but b) More importantly could not be modified to be safe with primitive write barriers (due to not recieving an Object parameter)
• Various Javadoc changes including capitalising types (e.g. address -> Address) and fixing existing cut and paste errors

2) RVM-845-20091211.patch.1

This is an updated version of my prior usePrimitiveWriteBarriers.patch to include write barriers for Word, Address, Extent and Offset types. This test GC is only suitable for testing that the primitive write barrier code compiles - it will not necessarily catch the opt compiler optimising away parts of the barrier.

Laurence, thanks for your patience. I'm working on it now. Just slowly walking through the 227KB patch.

However, I noticed that it failed the pre-commit test. Error message below. I will continue going through the patch, but if you get the chance to look into the error below before I do, that would be nice.

—

===================== Results =====================
Total Success Rate 122/124

Subversion Revision: 15804M

Result Configuration Group Test Run Reason
FAILURE prototype.default basic TestFieldReflection Unexpected exit code.
FAILURE development.Opt_1 basic TestFieldReflection Unexpected exit code.

===================================================

—

Testing on getFields
Values: true 127 1.0 1.0 1 1 1 true 127 1.0 1.0 1 1 1
Set booleans to false !!!!!!!!!!!!
Values: false 127 1.0 1.0 1 1 1 false 127 1.0 1.0 1 1 1
Set bytes to 12 Unable to render embedded object: File (++++++) not found.+vm internal error at:
Thread #12
– Stack –
at [0x67d85c80, 0x5b336bfa] Lorg/jikesrvm/VM; sysFail(Ljava/lang/String;)V at line 2275
at [0x67d85ca0, 0x5b336cff] Lorg/jikesrvm/VM; _assertionFailure(Ljava/lang/String;Ljava/lang/String;)V at line 613
at [0x67d85cc8, 0x5b336d8d] Lorg/jikesrvm/VM; _assert(ZLjava/lang/String;Ljava/lang/String;)V at line 596
at [0x67d85cec, 0x5b24a73b] Lorg/jikesrvm/VM; _assert(Z)V at line 574
at [0x67d85d0c, 0x5b01a4bb] Lorg/jikesrvm/runtime/Statics; setSlotContents(Lorg/vmmagic/unboxed/Offset;Ljava/lang/Object;)V at line 625
at [0x67d85d38, 0x5b23c80d] Lorg/jikesrvm/classloader/RVMField; setDoubleValueUnchecked(Ljava/lang/Object;D)V at line 573
at [0x67d85d6c, 0x5b23cd3f] Ljava/lang/reflect/VMCommonLibrarySupport; setByteInternal(Ljava/lang/Object;BLorg/jikesrvm/classloader/RVMField;)V at line 795
at [0x67d85da0, 0x5b0a21bd] Ljava/lang/reflect/VMCommonLibrarySupport; setByte(Ljava/lang/Object;BLorg/jikesrvm/classloader/RVMField;Ljava/lang/reflect/Field;Lorg/jikesrvm/classloader/RVMClass;)V at line 726
at [0x67d85dd8, 0x5b0a0e5c] Ljava/lang/reflect/VMField; setByte(Ljava/lang/reflect/Field;Ljava/lang/Object;B)V at line 113
at [0x67d85e08, 0x6783e581] Ljava/lang/reflect/Field; setByte(Ljava/lang/Object;B)V at line 540
at [0x67d85e44, 0x678350cd] Ltest/org/jikesrvm/basic/core/reflect/TestFieldReflection; testByte(Ljava/lang/Object;[Ljava/lang/reflect/Field;)V at line 154
at [0x67d85e6c, 0x6782c193] Ltest/org/jikesrvm/basic/core/reflect/TestFieldReflection; testFieldReflection(Ljava/lang/Object;Ljava/lang/Class;[Ljava/lang/reflect/Field;)V at line 80
at [0x67d85e90, 0x678310f9] Ltest/org/jikesrvm/basic/core/reflect/TestFieldReflection; testFieldReflection(Ljava/lang/Class;)V at line 66
at [0x67d85eac, 0x5b2d6a36] Ltest/org/jikesrvm/basic/core/reflect/TestFieldReflection; main([Ljava/lang/String;)V at line 60
at [0x67d85ec4, 0x5b267ddc] <invisible method>
at [0x67d85f38, 0x5b268240] Lorg/jikesrvm/runtime/Reflection; outOfLineInvoke(Lorg/jikesrvm/classloader/RVMMethod;Ljava/lang/Object;[Ljava/lang/Object;Z)Ljava/lang/Object; at line 194
at [0x67d85f70, 0x5b1ae7e9] Lorg/jikesrvm/runtime/Reflection; invoke(Lorg/jikesrvm/classloader/RVMMethod;Lorg/jikesrvm/runtime/ReflectionBase;Ljava/lang/Object;[Ljava/lang/Object;Z)Ljava/lang/Object; at line 76
at [0x67d85fb0, 0x5b29df07] Lorg/jikesrvm/scheduler/MainThread; run()V at line 201
at [0x67d85fd8, 0x5b322039] Lorg/jikesrvm/scheduler/RVMThread; run()V at line 2491
at [0x67d85ff8, 0x080501e5] Lorg/jikesrvm/scheduler/RVMThread; startoff()V at line 2535
********************************************************************************

• Abnormal termination of Jikes RVM *
• Jikes RVM terminated abnormally indicating a problem in the virtual machine. *
• Jikes RVM relies on community support to get debug information. Help improve *
• Jikes RVM for everybody by reporting this error. Please see: *
• http://jikesrvm.org/Reporting+Bugs *
  ********************************************************************************

Laurence,

It looks good. I'm going to go ahead and performance test it.

I only saw one significant issue (trivial to fix). You have applied this pattern of change to Barriers.java:

-    if (NEEDS_BOOLEAN_GC_WRITE_BARRIER) {
+    if (NEEDS_BOOLEAN_PUTFIELD_BARRIER) {

That change is an error; it breaks some intentional abstraction:

• NEEDS_BOOLEAN_PUTFIELD_BARRIER encapsulates the idea that the VM is required to insert a barrier. It intentionally abstracts over the reason for that barrier being required. The VM (compilers, classloader, etc etc) should only ever refer to the constants of this form when deciding whether a barrier is required. The nomenclature here is Java-specific.
• NEEDS_BOOLEAN_GC_WRITE_BARRIER is private to Barriers.java and exposes a (conditional) reason for the requirement to have a barrier. In the current code base, this is the only reason why a VM barrier might be required, but this is not generally the case. There exist important non-GC rationales for implementing barriers (something like arraylets is one example). These other cases need not be mutually exclusive nor one-to-one with GC requirements. The nomenclature here is language-neutral.

So the code above deals separately with each reason for including a barrier. I anticipate the addition of other non-GC cases in the near term (the code exists...). So we would then see as set of variables in the form of NEEDS_BOOLEAN_FOOBAR_WRITE_BARRIER, etc. I realize that without the other cases in place the code probably looks strange

One other minor trivial issue: you should probably change Magic_Store64_MD etc to Magic_Store64MD. AFAIK, the Magic_ prefix is a special one to denote a particular set of classes, but otherwise we're supposed to avoid underbars in class names.

Hi Steve,

Apologies for the preCommit failure (didn't notice the error in the logs)... Working on a fix now hope to have a patch shortly.

Kind regards
Laurence

The attached patch (RVM-845-20091222.patch.0) fixes the pre-commit failure identified by Steve. Floats and doubles were missing their own setSlotContents(...) methods in org.jikesrvm.runtime.Statics and were being autoboxed and written via the Object setSlotContents method. All pre-commit tests now pass on x86 and ppc32.

With respect to refactoring of org.jikervm.mm.mminterface.Barriers - ah ok. I see how you intend the code to look in the future with the possibility of non-GC related barriers.

One other minor trivial issue: you should probably change Magic_Store64_MD etc to Magic_Store64MD. AFAIK, the Magic_ prefix is a special one to denote a particular set of classes, but otherwise we're supposed to avoid underbars in class names.

I tried hard to follow the coding conventions used by each of the compilers, it so happens that classes such as Magic_Store32_MD already existed in the ia32 baseline compiler so I followed the existing naming convention when adding Magic_Store_64_MD et al. If there is a strong feeling that the existing code should also be refactored then perhaps this can be done as a separate issue.

Thanks for taking the time to review the patch, it would be a very nice Christmas present to get this into trunk!

Kind regards
Laurence

Thanks Laurence.

I committed your patches in r15805 and 15806 and added them to the sanity regressions in 15809.

Thanks for your patience on this!