[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8055845 - Add trace event for promoted objects
From:       Thomas Schatzl <thomas.schatzl () oracle ! com>
Date:       2014-09-16 13:41:30
Message-ID: 1410874890.2755.66.camel () cirrus
[Download RAW message or body]

Hi,

On Mon, 2014-09-15 at 14:44 -0700, Staffan Friberg wrote:
> Hi,

> > The latter messes up the method signatures, and in any case (when using
> > option one or three) this code is slightly racy as we might report too
> > many events as another thread might have claimed the object. (Parallel
> > Scavenge has the same issue, in addition to the possibility of sending
> > two events as Bengt describes).
> Good catch with the assert had missed that, will read it similar to how 
> it is done G1ParScanThreadState::copy_to_survivor_space instead, which 
> avoids the assert.
> 

Maybe the change could introduce a method without the assert and use it
instead of repeating this all the time? Reading the age from a mark oop
without caring about the assert seems to be done in a few places now.

> I think the best way would be to read the age and then check 
> is_forwarded, like now but without the assert issue. If is_forwarded is 
> false the read age will be OK to use, if it is true it might be a valid 
> age or overwritten by the forward, but I won't be using it so that is 
> fine. It would still have the risk of not actually doing the copying 
> later, but at least all the data about the object should be correct at 
> this point.
> 
> 
>    if (result != NULL && _gc_tracer_stw->should_report_promotion_event()) {
>      markOop m = old->mark();
>      uint age = m->has_displaced_mark_helper() ? 
> m->displaced_mark_helper()->age()
>                                                : m->age();
>      // Check if object has already been promoted by another thread
>      if (!old->is_forwarded()) {
>        _gc_tracer_stw->report_promotion_event(old, age,
>                !heap_region_containing_raw(result)->is_young(), word_size);
>      }
>    }
> 
> 
> I don't follow why you would like the age to be read after the 
> is_forwarded. Trusting the age is my main concern here and reading it 
> prior to the is_forward check I think would solve that. Could you expand 
> on this further?

Instead of first getting the markOop like the code does in this latest
version, this would have avoided the assertion failure too.

The latest change looks okay now if the tradeoffs are acceptable.

Thanks,
  Thomas


[prev in list] [next in list] [prev in thread] [next in thread]