On Tue, 30 Mar 2021 23:51:41 GMT, Hui Shi <[email protected]> wrote: >> …ue to large TLAB size >> >> serviceability/jvmti/HeapMonitor tests intermittently fail when using >> PS/Serial GC, original test has implicit assumptions on TLAB size and >> depends on allocate fix amount of objects to consume TLAB and trigger object >> sampling. These tests will fail if TLAB is above 20M (this can easily happen >> when using PS/Serial GC and heap is large), when allocation can not consume >> current TLAB and _byte_until_sample. >> >> Fix in tests is adding an explicit GC to consume current TLAB. >> Running on 256G memory machine, make run-test CONF=release >> TEST="test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/" >> 'JTREG=JOBS=12;VM_OPTIONS=-XX:ActiveProcessorCount=1' >> >> before fix: 6 or 7 tests in 20 tests intermittently fail >> after fix: no failure in 100 runs release/fastdebug >> >> This might also fix https://bugs.openjdk.java.net/browse/JDK-8225313 > > Hui Shi has updated the pull request incrementally with one additional commit > since the last revision: > > update copyright year
Changes requested by cjplummer (Reviewer). test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java line 176: > 174: // and set _bytes_until_sample to 0. > 175: // initial _bytes_until_sample is geometric variable with the > specified mean > 176: // (512K by default), check > ThreadHeapSampler::pick_next_geometric_sample() I think there is too much detail about the hotspot implementation in these comments. I think for (1) it is enough to say that the current TLAB needs to be consumed, and that this can be accomplished with a System.gc(). For (2) I don't really understand what you are saying. If you can explaining without referring to specific fields in hotspot, that would be better. I also don't understand how you are making sure that (2) is triggered. Does that happen during the allocation loop? test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java line 180: > 178: // trigger GC to consume current TLAB in step1 > 179: // consume initial _bytes_until_sample in following loops, each > iteration consume > 180: // about 1600KB, 10 iterations can definitly consume intitial > _bytes_until_sample I think this comment should be after the System.gc() call. test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java line 215: > 213: enableSamplingEvents(); > 214: } > 215: // similar reason with sampleEverything, consume TLAB and trigger > sample How about: ` // Use System.gc() to consume TLAB and trigger sampling as described above in sampleEverything` test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.java line 49: > 47: > 48: HeapMonitor.enableSamplingEvents(); > 49: // retire TLAB with GC, ensure sample happens, not assume TLAB size // Instead of relying on the allocation loop to fill and retire the TLAB, which might not happen, // use System.gc() to retire the TLAB and ensure sampling happens ------------- PR: https://git.openjdk.java.net/jdk/pull/3265
