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

[Hohensee, Paul]

GC folks, I'd like comment on my suggestion too, please.

Thanks,

Paul

On 12/31/18, 7:27 AM, "hotspot-gc-dev on behalf of Hohensee, Paul" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of hohensee at amazon.com> wrote:

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