RFR (L): Add TSAN instrumentation for allocation and garbage collection

[Man Cao]

All done. Thanks for the reviews.

-Man


On Mon, Jul 1, 2019 at 10:39 AM Arthur Eubanks <aeubanks at google.com> wrote:

>           // Update source and target ranges.
>           source_low = MIN2(source_low, (char *)source_obj);
>           source_high = MAX2(source_high, (char *)source_obj +
> obj_size_bytes);
>           target_low = MIN2(target_low, (char *)target_obj);
>           target_high = MAX2(target_high, (char *)target_obj +
> obj_size_bytes);
>           if (target_obj < source_obj) {
>             ++(*n_downward_moves);
>           }
>           // Append to the moves list.
>           PendingMove move = {(char *)source_obj, (char *)target_obj,
>                               obj_size_bytes};
>
> (source_obj + obj_size_bytes) and (target_obj + obj_size_bytes) can be
> replaced with move.source_end()/target_end(), right? Then might as well
> replace (source_obj) with move.source_begin() and replace (target_obj) with
> move.target_begin().
>
> Otherwise LGTM, no need for new webrev.
>
> On Mon, Jul 1, 2019 at 9:09 AM Jean Christophe Beyler <jcbeyler at google.com>
> wrote:
>
>> Hi Man,
>>
>> Small nits:
>>
>> buckets -> should be _buckets
>> +    TsanOopSizeMap* newMap = new TsanOopSizeMap(map_size / 2); -> should
>> be new_map
>>
>> No need for a new webrev for me, it looks good,
>> Jc
>>
>> On Fri, Jun 28, 2019 at 7:00 PM Man Cao <manc at google.com> wrote:
>>
>>> I've addressed all cleanup/refactoring comments except this one:
>>>
>>> > Should this instead be an assert and we do the check for
>>> ThreadSanitizer in the caller?
>>> >  456   if (!ThreadSanitizer) return;
>>> It would be ugly to make WeakProcessor conditionally run
>>> TsanOopMap::weak_oops_do(), due to its use of function pointers.
>>> JvmtiExport::weak_oops_do and Jfr::weak_oops_do have similar checks that
>>> guards their entire execution.
>>>
>>> I also updated comment at the top of tsanOopMap.hpp, as it tracks all
>>> Java objects instead of only lock objects, and fixed a few compiler
>>> warnings.
>>> Also removed indention for "private:" / "public:" keywords, per latest
>>> OpenJDK style.
>>>
>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00-01.diff/
>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.01/
>>>
>>> -Man
>>>
>>>
>>> On Fri, Jun 28, 2019 at 12:33 PM Man Cao <manc at google.com> wrote:
>>>
>>>> I don't think it's worth it to change the algorithm for
>>>> TsanOopSizeMap::rebuild_oops_map at this point. If want to improve
>>>> performance, we should first think for better ways for tracking
>>>> allocations, or parallelize TsanOopSizeMap::rebuild_oops_map. I'm happy to
>>>> do other cleanup and refactoring, though.
>>>>
>>>> -Man
>>>>
>>>>
>>>> On Fri, Jun 28, 2019 at 11:34 AM Arthur Eubanks <aeubanks at google.com>
>>>> wrote:
>>>>
>>>>> I wonder if for the part in TsanOopSizeMap::rebuild_oops_map where we
>>>>> tell TSAN about moved objects, instead of repeatedly doing passes until
>>>>> everything is moved (which as mentioned in the comments has a worst case of
>>>>> O(N^2)), we create a new fake heap area, for each oop we pretend to move it
>>>>> to the new fake heap area in a contiguous chunk of memory, then pretend to
>>>>> move each oop from the new fake area to the final location after the GC
>>>>> move. That would be O(N) and would (I think) simplify the code.
>>>>> TSAN <https://clang.llvm.org/docs/ThreadSanitizer.html> already has
>>>>> 5-10x memory overhead.
>>>>>
>>>>> On Thu, Jun 27, 2019 at 5:45 PM Arthur Eubanks <aeubanks at google.com>
>>>>> wrote:
>>>>>
>>>>>> Could you change the comment in tsanOopMap.hpp
>>>>>>   39 // 1. add() is only passed a live oop.
>>>>>>   40 // 2. add() must be thread-safe wrt itself.
>>>>>> "add()" -> "add_*()"
>>>>>>
>>>>>>
>>>>>> Another comment in tsanExternalDecls.hpp which seems inaccurate
>>>>>> +  // Called lazily when an oop should start to be tracked by Tsan.
>>>>>> It seems like we're calling __tsan_java_alloc() eagerly, not lazily.
>>>>>>
>>>>>>
>>>>>> tsanOopMap.cpp:
>>>>>>
>>>>>> We fixed this, comment is wrong:
>>>>>>  448 // TODO: We never call __tsan_java_fini; we should.
>>>>>> And right above, maybe we could deallocate where we call
>>>>>> __tsan_java_fini?
>>>>>>  447 // TODO: We never deallocate the oopMap; we should.
>>>>>>
>>>>>> Should this instead be an assert and we do the check for
>>>>>> ThreadSanitizer in the caller?
>>>>>>  456   if (!ThreadSanitizer) return;
>>>>>>
>>>>>>
>>>>>> I also need more time to review rebuild_oops_map() :).
>>>>>>
>>>>>> On Thu, Jun 27, 2019 at 2:49 PM Jean Christophe Beyler <
>>>>>> jcbeyler at google.com> wrote:
>>>>>>
>>>>>>> Hi Man,
>>>>>>>
>>>>>>>
>>>>>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00/src/hotspot/share/runtime/sharedRuntime.cpp.udiff.html
>>>>>>> // Note Well.
>>>>>>> ->
>>>>>>> // NOTE:
>>>>>>>
>>>>>>>
>>>>>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00/test/hotspot/jtreg/tsan/NonRacyGarbageCollectionLoopTest.java.html
>>>>>>> -> I would put a note somewhere that if you change the 40 for
>>>>>>> HEAP_SIZE you
>>>>>>> should change the Xms/Xmx... (I imagined we could create the Xms/Xmx
>>>>>>> with
>>>>>>> string concatenation but that seems overkill)
>>>>>>>
>>>>>>>
>>>>>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00/src/hotspot/share/tsan/tsanOopMap.cpp.html
>>>>>>>    - I find TsanOopBucket is not the right name, it might be TsanOop
>>>>>>>
>>>>>>>   -  120       do {
>>>>>>>  121         h = hash(h);
>>>>>>>  122         bucket = &buckets[h % _size];
>>>>>>>  123       } while (bucket->has_oop() && bucket->get_oop() != o);
>>>>>>>
>>>>>>> This could be put in a helper method because done twice, no?
>>>>>>>
>>>>>>> - print_map is not used, is it?
>>>>>>> - oopMap should be oop_map no?
>>>>>>>
>>>>>>> I admit I have not finished looking at rebuild_oops_map, it is a bit
>>>>>>> big
>>>>>>> ;-) could we break it up?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jc
>>>>>>>
>>>>>>
>>
>> --
>>
>> Thanks,
>> Jc
>>
>