[Nice summary from Dave D email]

Here is the weirdness I was alluding to in the HS meeting.

Consider, for example, in collectorPolicy.cpp:

  TwoGenerationCollectorPolicy::mem_allocate_work

Here we have

   {
      MutexLocker ml(Heap_lock);
      ...
      // Read the gc count while the heap lock is held.
      gc_count_before = Universe::heap()->total_collections();
    }


    VM_GenCollectForAllocation op(size,
                                  is_large_noref,
                                  is_tlab,
                                  gc_count_before);
    VMThread::execute(&op);

Note that the heap lock is not held when VMThread::execute is called,
Now, "VM_GenCollectForAllocation" is a VM_GC_Operation:

  class VM_GenCollectForAllocation: public VM_GC_Operation {

The constructor for VM_GenCollectForAllocation passes
"gc_count_before" to its supertype:

  VM_GenCollectForAllocation(size_t size,
                             bool large_noref,
                             bool tlab,
                             int gc_count_before)
    : VM_GC_Operation(gc_count_before),

which uses it to initialze the field _gc_count_before:

  VM_GC_Operation(int gc_count_before) {
    _prologue_succeeded = false;
    _gc_count_before = gc_count_before;
  }

Now VMThread::execute(&op) does:

    void VMThread::execute(VM_Operation* op) {
      if (!t->is_VM_thread()) {
// New request from Java thread, evaluate prologue
if (!op->doit_prologue()) {
return; // op was cancelled
}

And VM_GenCollectForAllocation inherits VM_GC_Operation::doit_prologue,
which does:

bool VM_GC_Operation::doit_prologue() {
  assert(Thread::current()->is_Java_thread(), "just checking");

  // *******
  _gc_count_before = Universe::heap()->total_collections();

  acquire_pending_list_lock();
  // If the GC count has changed someone beat us to the collection
  // Get the Heap_lock after the pending_list_lock.
  Heap_lock->lock();
  // Check invocations
  if (gc_count_changed()) {

I added the newlines and ****'s. This line overwrites the gc_count
value with which the VM_GC_Operation was initialized, and puts in a
value of total_collections read outside the heap lock. So we could
have:

(I'm going to write "N" for "total_collections".)

  Thread A Thread B

  lock(Heap_lock)
  gc_count_before = N
  unlock(Heap_lock)

lock(Heap_lock)
gc_count_before = N
unlock(Heap_lock)

  op._gc_count_before = gc_count_before;
op._gc_count_before = gc_count_before;

  VMThread.execute(op);
VMThread.execute(op);
                                          op._gc_count_before = N;
                                          lock(Heap_lock)
                                          is N == op._gc_count_before? yes.
do collection.
                                          N++;
unlock(Heap_lock);

  op.gc_count_before = N;
  lock(Heap_lock)
  is N == op._gc_count_before? yes.
  do collection.
  N++;
  unlock(Heap_lock);


This is bad because we wanted thread A to fail, to go back out and
retry the allocation, since the collection that happened probably made
it possible. The count should only be observed when holding the heap
lock.

Hope this is clear; let me know if not...