RFR(XS): 8214235: assertion in collectedHeap.cpp: attempt to clean empty remainder

[Hohensee, Paul]

Yes, please proceed with the fix to tlab_alloc_reserve(). The original assert was doing its job of detecting a bug.

You're correct, I was looking at http://cr.openjdk.java.net/~bulasevich/8214235/webrev.01/. In that, chunks less than min_fill_size() could be left unfilled. Unlikely, but possible given a bug somewhere else.

In http://cr.openjdk.java.net/~bulasevich/8214235/webrev.00/, if the space available between top() and hard_end() is less than min_fill_size(), fill_with_dummy_object() should assert.

Thanks,

Paul

On 12/29/18, 6:10 AM, "Boris Ulasevich" <boris.ulasevich at bell-sw.com> wrote:

    Hello Paul,
    
    Thank for your input! I do not see how my final fix can introduce 
    non-object gap (may be you had a look to webrev.01 which was rejected?):
    
     > http://cr.openjdk.java.net/~bulasevich/8214235/webrev.00
     > void ThreadLocalAllocBuffer::insert_filler() {
     >   assert(end() != NULL, "Must not be retired");
     > + if (top() < hard_end()) {
     >     Universe::heap()->fill_with_dummy_object(top(), hard_end(), true);
     > + }
     > }
    
    I checked ">=" change with my crasher. It works OK (assertion check does 
    not fire because TLAB alignment_reserve grows 0->2 and therefore top 
    never hits hard_end). Do you think I should proceed (test, review) with 
    this fix instead of mine?
    
    thanks,
    Boris
    
    On 27.12.2018 20:13, Hohensee, Paul wrote:
    > Hi Boris,
    > 
    > I don't think this is the actual bug, rather it's in collectedHeap.cpp
    > 
    > size_t CollectedHeap::tlab_alloc_reserve() const {
    >    size_t min_size = min_dummy_object_size();
    >    return min_size > (size_t)MinObjAlignment ? align_object_size(min_size) : 0;
    > }
    > 
    > ">" should be ">=". Would you try it out on your crasher please?
    > 
    > During GC, an invariant is that eden (and other spaces) be  "parseable", i.e., it's filled with legal objects from bottom to top. Your patch can introduce a non-object gap in eden at arbitrary places.
    > 
    > Thanks,
    > 
    > Paul
    > 
    > On 12/18/18, 7:14 AM, "hotspot-gc-dev on behalf of Per Liden" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of per.liden at oracle.com> wrote:
    > 
    >      Hi Boris,
    >      
    >      On 12/11/18 3:13 PM, Boris Ulasevich wrote:
    >      > Hi Per,
    >      >
    >      >    You asked if I have reproducer for this on x86. I do have! The
    >      > configuration [--with-jvm-variants=client --with-debug-level=fastdebug]
    >      > on 32bit linux-x86 (alignment_reserve is empty for this configuration)
    >      > fires the assertion even on build time:
    >      > #  Internal Error
    >      > (/ws/boris/jdk-jdk/src/hotspot/share/gc/shared/collectedHeap.cpp:373),
    >      > pid=4338, tid=4339
    >      > #  assert(words >= min_fill_size()) failed: too small to fill
    >      >
    >      > My fix for the issue works Ok:
    >      >    http://cr.openjdk.java.net/~bulasevich/8214235/webrev.00
    >      
    >      Thanks for the reproducer, I think the above patch looks good.
    >      
    >      cheers,
    >      Per
    >      
    >      >    http://bugs.openjdk.java.net/browse/JDK-8214235
    >      >
    >      > Boris
    >      >
    >      > On 29.11.2018 18:13, Boris Ulasevich wrote:
    >      >> Hi Per,
    >      >>
    >      >> On 29.11.2018 16:30, Per Liden wrote:
    >      >>> Hi,
    >      >>>
    >      >>> On 11/29/18 12:02 PM, Boris Ulasevich wrote:
    >      >>>> Hi Per,
    >      >>>>
    >      >>>> On 28.11.2018 17:35, Per Liden wrote:
    >      >>>>> Hi,
    >      >>>>>
    >      >>>>> On 11/28/18 1:39 PM, Boris Ulasevich wrote:
    >      >>>>>> Hi all,
    >      >>>>>>
    >      >>>>>> Please review a simple fix: do not fill remaining tlab memory
    >      >>>>>> when the remainder is empty.
    >      >>>>>
    >      >>>>> Hmm I get the feeling something else is wrong here. Like a
    >      >>>>> misaligned object or a misaligned object size was allocated in the
    >      >>>>> TLAB.
    >      >>>>
    >      >>>> Actually assertion check catches the case when top() = end().
    >      >>>> I do not think it is something criminal. May be it is better to
    >      >>>> add check (top() < hard_end()) instead:
    >      >>>> http://cr.openjdk.java.net/~bulasevich/8214235/webrev.00
    >      >>>
    >      >>> There's still something fishy here. The TLAB alignment_reserve should
    >      >>> make sure there's always enough space left for a filler object.
    >      >>
    >      >> My case is different! alignment_reserve() returns 0:
    >      >> alignment_reserve() = MAX2(tlab_alloc_reserve(),
    >      >> _reserve_for_allocation_prefetch) =
    >      >> MAX2(min_dummy_object_size() > MinObjAlignment ?
    >      >> min_dummy_object_size() : 0, is_server_compilation_mode_vm() ? a+b*c/d
    >      >> : 0) =
    >      >> MAX2(2 > 2 ? 2 : 0, 0 ? a+b*c/d : 0) = MAX2(0, 0) = 0
    >      >>
    >      >>> Do you have a reproducer for this on x86 or does this only happen on
    >      >>> Arm?
    >      >>>
    >      >>> /Per
    >      >>>
    >      >>>>
    >      >>>>> When you crash here it would be interesting to see what the tlab
    >      >>>>> start/end/top/etc is and what MinObjAlignement is being used.
    >      >>>>
    >      >>>> TLAB: start()=0x938b0b28, end()=0x938dcdc8, top()=0x938dcdc8
    >      >>>> MinObjAlignment: 2
    >      >>>>
    >      >>>>> cheers,
    >      >>>>> Per
    >      >>>>>
    >      >>>>>>
    >      >>>>>> https://bugs.openjdk.java.net/browse/JDK-8214235
    >      >>>>>> http://cr.openjdk.java.net/~bulasevich/8214235/webrev.01
    >      >>>>>>
    >      >>>>>> Thanks,
    >      >>>>>> Boris
    >      
    >