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

[Jean Christophe Beyler]

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