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

[Arthur Eubanks]

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