RE: 回复: 回复: 回复: [8u] RFR: 8255717 Fix JFR crash in WriteObjectSampleStacktrace with object has uninitialized klass(Internet mail)

[Hohensee, Paul]

Lgtm, except that threadLocalAllocBuffer.hpp needs a copyright date update. No need for a re-review for that.

Paul

On 11/11/20, 3:56 AM, "kalinshi(施慧)" <kalinshi at tencent.com> wrote:

    Thanks! New TLAB queries are added and unnecessary comment and assert removed.

    JBS: https://bugs.openjdk.java.net/browse/JDK-8255717
    Webrev: http://cr.openjdk.java.net/~hshi/8255717/webrev_v4/

    Regards
    Hui

    -----邮件原件-----
    发件人: Hohensee, Paul <hohensee at amazon.com>
    发送时间: 2020年11月11日 5:30
    收件人: kalinshi(施慧) <kalinshi at tencent.com>; Liu, Xin <xxinliu at amazon.com>; jdk8u-dev at openjdk.java.net
    主题: (Internet mail)Re: 回复: 回复: [8u] RFR: 8255717 Fix JFR crash in WriteObjectSampleStacktrace with object has uninitialized klass(Internet mail)

    I like this approach, but I'd rather see tlab queries added to threadLocalAllocationBuffer.hpp rather than constructing them in send_jfr_allocation_notification_event. E.g., add these methods to ThreadLocalAllocationBuffer and use them in send_jfr_allocation_event().

    bool is_first(HeapWord* obj) { return obj == start(); } bool is_in(HeapWord* obj) { return obj < start() || obj > end()); size_t hard_size() { return pointer_delta(hard_end(), start()); }

    No need for a comment on send_jfr_allocation_event(). The one that's there (i.e, "// invoked") is obvious info.

    No need for an assert in send_jfr_allocation_event(). If obj is NULL, it should never get there, and, looking at the jfr code, it doesn't look like a crash can happen even if it is NULL.

    Thanks,
    Paul

    On 11/5/20, 6:49 AM, "jdk8u-dev on behalf of kalinshi(施慧)" <jdk8u-dev-retn at openjdk.java.net on behalf of kalinshi at tencent.com> wrote:

        Thanks! Webrev is updated.

        JBS: https://bugs.openjdk.java.net/browse/JDK-8255717
        Webrev: http://cr.openjdk.java.net/~hshi/8255717/webrev_v3/

        Regards
        Hui

        -----邮件原件-----
        发件人: Liu, Xin <xxinliu at amazon.com>
        发送时间: 2020年11月5日 16:59
        收件人: kalinshi(施慧) <kalinshi at tencent.com>; jdk8u-dev at openjdk.java.net
        主题: Re: 回复: [8u] RFR: 8255717 Fix JFR crash in WriteObjectSampleStacktrace with object has uninitialized klass(Internet mail)

        Hi, Hui,

        Yes, I think post_allocation_notify is an ideal place to place send_jfr_allocation_event.  One nit is you probably need to extend the comment " // Support for jvmti and dtrace".
        There's a trailing whitespace after "// allocate in new TLA" in send_jfr_allocation_event.
        Update copyright header?

        Except above, your patch looks good to me, but we still need a reviewer to approve it.

        Thanks,
        --lx

        On 11/4/20, 10:13 PM, "kalinshi(施慧)" <kalinshi at tencent.com> wrote:

            Please help review updated fix again!

            Move JFR send allocation event code. From before object setup to post_allocation_notify.

            send_allocation_in_new_tlab_event is invoked if object is allocated at current thread's TLAB start.
            send_allocation_outside_tlab_event is invoked if object is not in current thread's TLAB.

            Not sure why AllocTracer::send_allocation_in_new_tlab_event and AllocTracer::send_allocation_outside_tlab_event not guarded with JFR_ONLY macro. Leave it same with current code.

            JBS: https://bugs.openjdk.java.net/browse/JDK-8255717
            Webrev: http://cr.openjdk.java.net/~hshi/8255717/webrev_v2/

            Regards
            Hui

            -----邮件原件-----
            发件人: Liu, Xin <xxinliu at amazon.com>
            发送时间: 2020年11月3日 11:12
            收件人: kalinshi(施慧) <kalinshi at tencent.com>; jdk8u-dev at openjdk.java.net
            主题: Re: [8u] RFR: Fix JFR crash in WriteObjectSampleStacktrace with object has uninitialized klass(Internet mail)

            Hello, Hui,

            Thank you for taking time to fix it.  The following is my personal option. We still need reviewer to make decision.

            I think the safest approach is to postpone those two events after proper obj initialization.  That aligns up the behavior of newer jdks and avoids redundant stores of klasses, doesn't it?
            The key is a variable to record if the allocation happens in or out of tlab.
            https://github.com/openjdk/jdk/blob/master/src/hotspot/share/gc/shared/memAllocator.cpp#L49

            It doesn't necessarily mean you have to backport the class MemAllocator::Allocation to jdk8u.  One idea is to extend common_mem_allocate_init  with another output argument.
            HeapWord* CollectedHeap::common_mem_allocate_init(KlassHandle klass, size_t size, TRAPS) {
              bool tlab = true;
              HeapWord* obj = common_mem_allocate_noinit(klass, size, tlab, CHECK_NULL);
              init_obj(obj, size);
              if (obj != NULL) {
                 if (tlab) send_allocation_in_new_tlab_event ()
                 else  send_allocation_outside_tlab_event()
              }
              return obj;
            }

            thanks,
            --lx

            On 11/1/20, 10:38 PM, "jdk8u-dev on behalf of kalinshi(施慧)" <jdk8u-dev-retn at openjdk.java.net on behalf of kalinshi at tencent.com> wrote:

                Hi All,

                Please help review crash fix in 8u JFR.

                JFR ObjectSample's _object field might have uninitialized klass when it is iterated in WriteObjectSampleStacktrace. Because ObjectSample is created before post_allocation_setup.
                This problem doesn't exist in 11 or master, as ObjectSample is created in MemAllocator::Allocation destruction method (notify_allocation_jfr_sampler), its klass is initialized.

                Detail reproduce and analysis in JBS. Before this patch, debug version crash 17 time sin 12 hours, with this patch, no crash reproduced.

                JBS: https://bugs.openjdk.java.net/browse/JDK-8255717
                Webrev http://cr.openjdk.java.net/~hshi/8255717/webrev/

                Regards
                Hui