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

[Arthur Eubanks]

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