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

[Man Cao]

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