October 2020 - hotspot-gc-dev

Work-in-progress: 8236485: Epoch synchronization protocol for G1 concurrent refinement
by Man Cao 19 Apr '21

19 Apr '21

Hi all, I have written up a description and challenges for implementing an epoch synchronization protocol. This protocol is necessary for removing the StoreLoad fence in G1's post-write barrier (JDK-8226731) Description: https://bugs.openjdk.java.net/browse/JDK-8236485 Work-in-progress webrev: https://cr.openjdk.java.net/~manc/8236485/webrev_wip0/ There are two main challenges that I'm not sure how to resolve: - Triggering a thread-local handshake is a blocking operation that can pass a safepoint. - There are native post-write barriers executed by threads in native/VM state. Discussions and suggestions are highly appreciated! -Man

4 7

RFR(S) : 8246494 : introduce vm.flagless at-requires property
by Igor Ignatyev 09 Mar '21

09 Mar '21

http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00 > 70 lines changed: 66 ins; 0 del; 4 mod Hi all, could you please review the patch which introduces a new @requires property to filter out the tests which ignore externally provided JVM flags? the idea behind this patch is to have a way to clearly mark tests which ignore flags, so a) it's obvious that they don't execute a flag-guarded code/feature, and extra care should be taken to use them to verify any flag-guarded changed; b) they can be easily excluded from runs w/ flags. @requires and VMProps allows us to achieve both, so it's been decided to add a new property `vm.flagless`. `vm.flagless` is set to false if there are any XX flags other than `-XX:MaxRAMPercentage` and `-XX:CreateCoredumpOnCrash` (which are known to be set almost always) or any X flags other `-Xmixed`; in other words any tests w/ `@requires vm.flagless` will be excluded from runs w/ any other X / XX flags passed via `-vmoption` / `-javaoption`. in rare cases, when one still wants to run the tests marked by `vm.flagless` w/ external flags, `vm.flagless` can be forcefully set to true by setting any value to `TEST_VM_FLAGLESS` env. variable. this patch adds necessary common changes and marks common tests, namely Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific tests will be marked separately by the corresponding subtasks of 8151707[1]. please note, the patch depends on CODETOOLS-7902336[2], which will be included in the next jtreg version, so this patch is to be integrated only after jtreg5.1 is promoted and we switch to use it by 8246387[3]. JBS: https://bugs.openjdk.java.net/browse/JDK-8246494 webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00 testing: marked tests w/ different XX and X flags w/ and w/o TEST_VM_FLAGLESS env. var, and w/o any flags [1] https://bugs.openjdk.java.net/browse/JDK-8151707 [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336 [3] https://bugs.openjdk.java.net/browse/JDK-8246387 Thanks, -- Igor

3 5

RFR: 8234930: Use MAP_JIT when allocating pages for code cache on macOS
by Anton Kozlov 15 Dec '20

15 Dec '20

Please review an updated RFR from https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041… On macOS, MAP_JIT cannot be used with MAP_FIXED[1]. So pd_reserve_memory have to provide MAP_JIT for mmap(NULL, PROT_NONE), the function was made aware of exec permissions. For executable and data regions, pd_commit_memory only unlocks the memory with mprotect, this should make no difference compared with old code. For data regions, pd_uncommit_memory still uses a new overlapping anonymous mmap which returns pages to the OS and immediately reflects this in diagnostic tools like ps. For executable regions it would require MAP_FIXED|MAP_JIT, so instead madvise(MADV_FREE)+mprotect(PROT_NONE) are used. They should also allow OS to reclaim pages, but apparently this does not happen immediately. In practice, it should not be a problem for executable regions, as codecache does not shrink (if I haven't missed anything, by the implementation and in principle). Tested: * local tier1 * jdk-submit * codesign[2] with hardened runtime and allow-jit but without allow-unsigned-executable-memory entitlements[3] produce a working bundle. (adding GC group as suggested by @dholmes-ora) [1] https://github.com/apple/darwin-xnu/blob/master/bsd/kern/kern_mman.c#L227 [2] codesign \ --sign - \ --options runtime \ --entitlements ents.plist \ --timestamp \ $J/bin/* $J/lib/server/*.dylib $J/lib/*.dylib [3] <?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> <plist version="1.0"> <dict> <key>com.apple.security.cs.allow-jit</key> <true/> <key>com.apple.security.cs.disable-library-validation</key> <true/> <key>com.apple.security.cs.allow-dyld-environment-variables</key> <true/> </dict> </plist> ------------- Commit messages: - Use MAP_JIT for CodeCache pages Changes: https://git.openjdk.java.net/jdk/pull/294/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=294&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8234930 Stats: 65 lines in 15 files changed: 27 ins; 0 del; 38 mod Patch: https://git.openjdk.java.net/jdk/pull/294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/294/head:pull/294 PR: https://git.openjdk.java.net/jdk/pull/294

5 42

Re: RFR: 8234930: Use MAP_JIT when allocating pages for code cache on macOS
by Stefan Karlsson 02 Dec '20

02 Dec '20

On Mon, 28 Sep 2020 11:16:46 GMT, Thomas Stuefe <stuefe(a)openjdk.org> wrote: >> Hi Anton, >> >> I am sorry, but I dislike the API changes to os::xxx(). As it is now, this API is quite convoluted, undocumented and >> inconsistent. There are ongoing efforts to clean this up (see eg JDK-8253650, JDK-8253638 and JDK-8253683). This mess >> carries over into the upper layers too, e.g. ReservedSpace. About the API changes: >> >> - Why does os::uncommit_memory() now require "exec" as parameter? I know why you do it, but semantically it has no >> meaning. I should be able to uncommit memory without knowing the exact flags the mapping had been established with. So >> now the user has to carry this information and provide it back to the API when uncommitting? Right now probably all >> uncommits happen in areas where the exec information is implicit by its usage, but who says this is always the case? >> >> - We now have to specify "exec" as parameter to os::reverve_memory(). This is another new restriction - before, I could >> theoretically reserve one mapping and commit various parts of it with and without exec flag. With this change, the >> whole mapping has to be either exec or !exec. >> Can we on MacOS not use mmap at os::commit_memory, like we do on other platforms? In os::commit_memory, the exec >> parameter exists but on MacOS it is ignored. It would be more consistent with other platforms if the exec parameter >> were honored in commit, not requiring a new one at reserve. And I see mmap is used in uncommit, so changing mappings >> via overlapped mmap seems to work? Thanks, Thomas > >> I am sorry, but I dislike the API changes to os::xxx(). As it is now, this API is quite convoluted, undocumented and >> inconsistent. > > Just to clarify that I did not mean your change makes the API convoluted and inconsistent but that it has been that way > already, for a long time. It is a constant headache to get right, especially in ports whose memory management is a bit > non-standard, e.g. on AIX. @tstuefe My patch to remove MAP_FIXED from the memory reservation path should make it possible to revert all the os::reserve_memory changes in this patch. ------------- PR: https://git.openjdk.java.net/jdk/pull/294

4 14

RFR: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
by Stefan Karlsson 02 Dec '20

02 Dec '20

This is an alternative version of the fix proposed in 900: https://github.com/openjdk/jdk/pull/900 Erik's description: > Today, when you crash, the GCLogPrecious::_lock is taken. This effectively limits you to only get clean crash reports if you crash or assert without holding a lock of rank tty or lower. It is arguably difficult to know what locks you are going to have when crashing. Therefore, I don't think the precious GC log should constrain possible crashing contexts in that fashion. As Erik mentioned in that PR, I'd like to retain the ability to easily dump the precious log when debugging. The proposed fix changes the Mutex to a Semaphore, and use trywait to safely access the buffer. In the unlikely event that another thread is holding the lock, the hs_err printer skips printing the log. This also makes it possible to call precious logging from within the stack watermark processing code. I think there's a possibility that we might call the following error logging, when we fail to commit memory for a ZPage, when relocating, during stack watermark processing: `log_error_p(gc)("Failed to commit memory (%s)", err.to_string());` ------------- Commit messages: - 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing Changes: https://git.openjdk.java.net/jdk/pull/903/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=903&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254877 Stats: 27 lines in 2 files changed: 11 ins; 4 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/903.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/903/head:pull/903 PR: https://git.openjdk.java.net/jdk/pull/903

4 9

RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap
by Lin Zang 05 Nov '20

05 Nov '20

- Parallel heap iteration support for PSS - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103 ------------- Commit messages: - 8252103: Parallel heap inspection for ParallelScavengeHeap Changes: https://git.openjdk.java.net/jdk/pull/25/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=25&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252103 Stats: 165 lines in 6 files changed: 165 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/25.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/25/head:pull/25 PR: https://git.openjdk.java.net/jdk/pull/25

5 70

RFR: 8188055: (ref) Add Reference::refersTo predicate
by Kim Barrett 04 Nov '20

04 Nov '20

Finally returning to this review that was started in April 2020. I've recast it as a github PR. I think the security concern raised by Gil has been adequately answered. https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.ht… https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.h… https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/03079… Please review a new function: java.lang.ref.Reference.refersTo. This function is needed to test the referent of a Reference object without artificially extending the lifetime of the referent object, as may happen when calling Reference.get. Some garbage collectors require extending the lifetime of a weak referent when accessed, in order to maintain collector invariants. Lifetime extension may occur with any collector when the Reference is a SoftReference, as calling get indicates recent access. This new function also allows testing the referent of a PhantomReference, which can't be accessed by calling get. The new function uses native methods whose implementations are in the VM so they can use the Access API. It is the intent that these methods will be intrinsified by optimizing compilers like C2 or graal, but that hasn't been implemented yet. Bear that in mind before rushing off to change existing uses of Reference.get. There are two native methods involved, one in Reference and an override in PhantomReference, both package private in java.lang.ref. The reason for this split is to simplify the intrinsification. This is a change from the version from April 2020; that version had a single native method in Reference, implemented using the ON_UNKNOWN_OOP_REF Access reference strength category. However, adding support for that category in the compilers adds significant implementation effort and complexity. Splitting avoids that complexity. Testing: mach5 tier1 Locally (linux-x64) verified the new test passes with various garbage collectors. ------------- Commit messages: - new function Changes: https://git.openjdk.java.net/jdk/pull/498/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=498&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8188055 Stats: 389 lines in 8 files changed: 387 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/498.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/498/head:pull/498 PR: https://git.openjdk.java.net/jdk/pull/498

14 73

RFR: 8254758: Change G1ServiceThread to be task based
by Stefan Johansson 03 Nov '20

03 Nov '20

Please review this enhancement to make `G1ServiceThread` task based. This thread has evolved from having a single responsibility to do more things. And there are plans to add more to it. To make this easier and to allow different tasks to run at different intervals this change adds a simple task mechanism to the service thread. The idea is that the service thread keeps a list of `G1ServiceTask`-objects that are ordered by when they should execute. When no tasks are ready to be run the service thread check how far into the future the next task is scheduled and sleeps/waits for that duration. If a new task is added while the thread is waiting it will be woken up to check for ready tasks. There are currently two tasks in the list. They have been extracted from the old service thread into the tasks: `G1PeriodicGCTask` and `G1RemSetSamplingTask`. These are more or less identical to the old code, but in the form of tasks. The only difference is that `G1PeriodicGCTask` no longer need to keep track of the last attempt, since it will be rescheduled with the correct interval. To reschedule a task is to update the time when it should be executed next and adding it back to the list. This insertion is O(n) and it should be fine since the expected number of tasks handled by the service thread is low. If a task don't want to be rescheduled it can set a negative timeout and that will cause the task to be dropped from the list. There are currently no such tasks and adding such tasks come with the responsibility of making sure resources are taken care of when the task is no longer active. I've added a gtest to do some basic VM-testing and apart from a lot of local testing I've run mach5 tier 1-4 a few times with good results. I have created a few follow up tasks to look at moving the current tasks to more appropriate locations and also updated a few old service thread related tasks with the label: [`gc-g1-service-thread`](https://bugs.openjdk.java.net/issues/?jql=labels%20%3D%20gc-g1-service-thread) ------------- Commit messages: - Self review - 8254758: Change G1ServiceThread to be task based Changes: https://git.openjdk.java.net/jdk/pull/734/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=734&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254758 Stats: 544 lines in 3 files changed: 448 ins; 29 del; 67 mod Patch: https://git.openjdk.java.net/jdk/pull/734.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/734/head:pull/734 PR: https://git.openjdk.java.net/jdk/pull/734

4 31

RFR: 8255237: ZGC: Bulk free garbage pages during relocation set selection
by Per Liden 02 Nov '20

02 Nov '20

During relocation set selection, we immediately free garbage (empty) pages. On very large heap, which has a very high number of garbage pages we can make a significant number of calls to free pages, which prolongs the "Concurrent Select Relocation Set" phase. Freeing a page involves grabbing a lock, inserting the page into the page cache, and then try to satisfy any stalled allocation. By freeing empty pages in bulk, we can amortize the some of the cost of freeing a page and speed up this phase. With this patch we free (up to) 64 pages in bulk. This PR depends on "8255234: ZGC: Bulk allocate forwarding data structures" and "8255235: ZGC: Allocate and initialize forwarding data structures in parallel", which are separate PRs (#804 and #805). This patch was joint work with Albert (ayang). ------------- Commit messages: - 8255237: ZGC: Bulk free garbage pages during relocation set selection - 8255235: ZGC: Allocate and initialize forwarding data structures in parallel - 8255234: ZGC: Bulk allocate forwarding data structures Changes: https://git.openjdk.java.net/jdk/pull/806/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=806&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255237 Stats: 667 lines in 24 files changed: 446 ins; 153 del; 68 mod Patch: https://git.openjdk.java.net/jdk/pull/806.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/806/head:pull/806 PR: https://git.openjdk.java.net/jdk/pull/806

3 9

RFR: 8255471: ZGC: Rework root iterators and closures
by Stefan Karlsson 02 Nov '20

02 Nov '20

Today we have the ZRootsIteratorClosure type to cater for the different type of roots and their containers (oop storage, threads, nmethods, runtime oop data structures). This closure is then passed down and applied to the different containers. We split the root sets into different sets: concurrent strong, concurrent weak, and non-concurrent weak. Not all of those sets contain all types of containers. This forces some of the code to implement, or at least consider, parts of the ZRootsIteratorClosure interface that it doesn't care about. It also moves some of the closure logic into shared code, even when it's only used by one of the closures. I propose that we turn this inside out, and use direct closures that match the visited containers (NMethodClosure for nmethods, et.c.). The intention is to make it a bit easier to see from the call sites what code is run and, hopefully, making localized changes stay more local in the code. Note: I started this code with a version building upon https://bugs.openjdk.java.net/browse/JDK-8212879, but I don't want to wait for that work to finish before sending this out, so the current patch contains handling of non-conurrent weak roots. As soon as that gets integrated, we can get rid of the ZWeakSerial and the associated non-concurrent weak roots handling. I've tested this on tier1-7 on linux x64. ------------- Commit messages: - 8255471: ZGC: Rework root iterators and closures Changes: https://git.openjdk.java.net/jdk/pull/928/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=928&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255471 Stats: 583 lines in 17 files changed: 231 ins; 202 del; 150 mod Patch: https://git.openjdk.java.net/jdk/pull/928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/928/head:pull/928 PR: https://git.openjdk.java.net/jdk/pull/928

3 10