Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v47]
On Fri, 8 Nov 2024 10:42:15 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused imports from VMProps I'm worked through the make + src changes and don't have any issues. I skimmed tests some didn't study closely. You will need to bump the copyright header of several jlink src files before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2465008109
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v4]
On Fri, 11 Oct 2024 15:49:33 GMT, Chen Liang wrote: >> Please review this patch that: >> 1. Implemented `forEach` to optimize for 1 or 2 element collections. >> 2. Implemented `spliterator` to optimize for a single element. >> >> The default implementations for multiple-element immutable collections are >> fine as-is, specializing implementation doesn't provide much benefit. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 18 commits: > > - s'marks requests > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Add test to ensure reproducible iteration order > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Use the improved form in forEach > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Null checks should probably be in the beginning... > - mark implicit null checks > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - ... and 8 more: https://git.openjdk.org/jdk/compare/7276a1be...4f1f4f1b Keep alive; maybe we can look forward to 25. - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2465816966
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v9]
On Tue, 29 Oct 2024 17:06:08 GMT, Harshitha Onkar wrote:
>> src/java.desktop/share/classes/java/awt/MouseInfo.java line 68:
>>
>>> 66: * @throws SecurityException if a security manager exists and its
>>> 67: *{@code checkPermission} method doesn't allow the
>>> operation
>>> 68: * @see GraphicsConfiguration
>>
>> Is `GraphicsConfiguration` removed from the list because it's already
>> mentioned and linked in the description above? Is it intentional?
>
>> Is it intentional?
>
> It was probably by mistake. but you are right, I see it mentioned already in
> the doc. I don't think we need to mention it again?
It has a value… when it's mentioned with `@see`, the link is present in the
*See Also* section, as you can see in the the specification of
[`MouseInfo.getPointerInfo()`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/MouseInfo.html#getPointerInfo()).
Without the `@see` tag, one has to read the entire description to find the link.
It looks subtle. We can restore the `@see`-link later if deemed necessary.
Does anyone else have an opinion?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1826266398
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v33]
On Fri, 8 Nov 2024 11:31:40 GMT, Magnus Ihse Bursie wrote:
>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86
>> Port_](https://openjdk.org/jeps/479).
>>
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port.
>>> This port was [deprecated for removal in JDK
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a
>>> future release.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Inline buildJniFunctionName
src/hotspot/os/posix/include/jvm_md.h line 41:
> 39:
> 40: #define JNI_ONLOAD_SYMBOLS {"JNI_OnLoad"}
> 41: #define JNI_ONUNLOAD_SYMBOLS {"JNI_OnUnload"}
are these changes related to the Windows 32-bit x86 port?
src/hotspot/os/posix/os_posix.cpp line 699:
> 697: }
> 698:
> 699: void os::print_jni_name_prefix_on(outputStream* st, int args_size) {
are these changes related to the Windows 32-bit x86 port?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1834878288
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1834878195
Re: RFR: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs) [v4]
> Please review this patch which removes unnecessary `@SuppressWarnings` > annotations. Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision: Put back @SuppressWarnings annotations to be fixed by JDK-8343286. - Changes: - all: https://git.openjdk.org/jdk/pull/21852/files - new: https://git.openjdk.org/jdk/pull/21852/files/a64d5594..9137373f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21852&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21852&range=02-03 Stats: 3 lines in 2 files changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21852.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21852/head:pull/21852 PR: https://git.openjdk.org/jdk/pull/21852
Re: RFR: 8343793: Test java/foreign/TestMemorySession.java is timing out [v2]
On Fri, 8 Nov 2024 16:59:48 GMT, Quan Anh Mai wrote:
>> Hi,
>>
>> This patch fixes the deadlock in `TestMemorySession#testAcquireCloseRace`.
>> The lock-step spin lock is implemented as with `lock` being an
>> `AtomicInteger`:
>>
>> // Keep the 2 threads operating on the same scope
>> int k = lock.getAndAdd(1) + 1;
>> while (k != i * 2) {
>> Thread.onSpinWait();
>> k = lock.get();
>> }
>>
>> Given the initial condition:
>>
>> Thread 1: i = 0
>> Thread 2: i = 0
>> lock: -2
>>
>> The `lock` then undergoes the following operations:
>>
>>
>>
>> Thread 1 Thread 2 lock value
>> getAndAdd(1)-1
>> getAndAdd(1) 0 -> Thread 2 then continues
>> its next iteration, its i value is now 1
>> getAndAdd(1) 1 -> Thread 2 reaches the
>> next iteration before thread 1 has a chance to read the value 0
>> get()1 -> Thread 1 now cannot
>> proceed because it missed the value 0
>>get() 1 -> Thread 2 now cannot
>> proceed because lock can never reach 2
>>
>>
>> The solution is to not rely on the exact value of the lock but instead
>> whether the lock has passed the expected value.
>>
>> Testing: I have run this test several hundreds times and got no failure
>> while without this patch I encountered a timeout every approximately 30
>> times.
>>
>> Please take a look, thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> refactor the test
Thanks!
-
Marked as reviewed by mcimadamore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21976#pullrequestreview-2425021686
Re: RFR: 8343819: Link Float.NaN and Double.NaN to equivalence discussion in Double
On Fri, 8 Nov 2024 10:10:28 GMT, Eirik Bjørsnøs wrote:
> Please review this doc-only enhancement which links the word _equivalent_ in
> `Float.NaN` and `Double.NaN` constant field descriptions to the
> floating-point equivalence discussion in `Double`.
>
>> It is equivalent to the value returned by{@code
>> Float.intBitsToFloat(0x7fc0)}.
>
> For readers not well-versed in floating point, it may not be immediatly clear
> what _equivalent to_ means here.
>
> The smallest improvement I could think of is to simply make the word
> `equivalent` a plain link to the floating-point equivalence discussion in the
> class description of `java.lang.Double`.
>
> Verification: This is a doc-only change. I ran `make docs` and verified that
> the links resolve.
Looks fine; thanks.
-
Marked as reviewed by darcy (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21972#pullrequestreview-2425021526
Re: RFR: 8343793: Test java/foreign/TestMemorySession.java is timing out
On Fri, 8 Nov 2024 12:22:54 GMT, Maurizio Cimadamore
wrote:
>> Hi,
>>
>> This patch fixes the deadlock in `TestMemorySession#testAcquireCloseRace`.
>> The lock-step spin lock is implemented as with `lock` being an
>> `AtomicInteger`:
>>
>> // Keep the 2 threads operating on the same scope
>> int k = lock.getAndAdd(1) + 1;
>> while (k != i * 2) {
>> Thread.onSpinWait();
>> k = lock.get();
>> }
>>
>> Given the initial condition:
>>
>> Thread 1: i = 0
>> Thread 2: i = 0
>> lock: -2
>>
>> The `lock` then undergoes the following operations:
>>
>>
>>
>> Thread 1 Thread 2 lock value
>> getAndAdd(1)-1
>> getAndAdd(1) 0 -> Thread 2 then continues
>> its next iteration, its i value is now 1
>> getAndAdd(1) 1 -> Thread 2 reaches the
>> next iteration before thread 1 has a chance to read the value 0
>> get()1 -> Thread 1 now cannot
>> proceed because it missed the value 0
>>get() 1 -> Thread 2 now cannot
>> proceed because lock can never reach 2
>>
>>
>> The solution is to not rely on the exact value of the lock but instead
>> whether the lock has passed the expected value.
>>
>> Testing: I have run this test several hundreds times and got no failure
>> while without this patch I encountered a timeout every approximately 30
>> times.
>>
>> Please take a look, thanks a lot.
>
> I'm playing with something like this:
>
>
> @Test
> public void testAcquireCloseRace() throws InterruptedException {
> int MAX_ITERATIONS = 1000;
> AtomicInteger iteration = new AtomicInteger();
> boolean[] result = new boolean[1];
> MemorySessionImpl[] scopes = new MemorySessionImpl[MAX_ITERATIONS];
> for (int i = 0; i < MAX_ITERATIONS; i++) {
> scopes[i] = MemorySessionImpl.toMemorySession(Arena.ofShared());
> }
>
> // This thread tries to close the scopes
> Thread t1 = new Thread(() -> {
> int it = iteration.get();
> while (it < MAX_ITERATIONS) {
> while (true) {
> try {
> scopes[it].close();
> // if we close successfully, the iteration is now
> completed
> break;
> } catch (IllegalStateException e) {
> // scope is acquired, wait and try again
> Thread.onSpinWait();
> }
> }
> // wait for other thread to complete its iteration
> int prev = it;
> while (prev == it) {
> Thread.onSpinWait();
> it = iteration.get();
> }
> }
> });
>
> // This thread tries to acquire the scopes, then check if it is alive
> after an acquire failure
> Thread t2 = new Thread(() -> {
> int it = iteration.get();
> while (it < MAX_ITERATIONS) {
> while (true) {
> try {
> scopes[it].acquire0();
> } catch (IllegalStateException e) {
> // we failed to acquire, meaning the other thread has
> closed this scope
> if (scopes[it].isAlive()) {
> result[0] = true;
> }
> // this iteration is now completed
> break;
> }
> // if we get here, acquire was successful, so we need to
> release...
> scopes[it].release0();
> // ... and then try again
> }
> // advance to next iteration
> it = iteration.addAndGet(1);
> }
> });
>
> t1.start();
> t2.start();
> t1.join();
> t2.join();
> assertFalse(result[0]);
> }
>
>
> I think I find this logic a bit more dir...
> @mcimadamore I think your approach is more or less similar. Although it is a
> little bit clearer in terms of what index we are operating on, it is a bit
> less clear in terms of the synchronization mechanism, as it is spread out to
> a wider scope.
IMHO both synchronization mechanisms are spread out in scope, as no matter what
you'll have an action at a distance. FWIW, I find the logic with index updating
particularly hard to read -- and a very indirect way to coordinate the two
threads, which is why I tried to reach for something a little more explicit.
-
PR Comment: https://git.openjdk.org/jdk/pull/21976#issuecomment-2465099538
Re: RFR: 8317542: Specjvm::xml have scalability issue for high vCPU numbers [v3]
On Thu, 7 Nov 2024 18:30:22 GMT, Vladimir Ivanov wrote: >> The synchronization block may be substituted by the 'volatile' variable >> smaller synchronization block. >> It reduce the total blocking time for the specjvm2008::xml.validation >> workload and improve the reported score. >> Scores for the 112vCPU on the with 28GB heap increased from 17915.83 to >> 22943.2. >> Unit tests was not affected: >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jaxp 516 516 0 0 >>jtreg:test/jdk/javax/xml 7070 0 0 >> == >> TEST SUCCESS >> >> The tier1 is OK too. > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8317542: Specjvm::xml have scalability issue for high vCPU numbers Thanks for the great work! Nice improvement on systems with high vCPUs. There are other areas worth checking. But this is a good step forward. - Marked as reviewed by joehw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21815#pullrequestreview-2424531410
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v13]
On Wed, 30 Oct 2024 11:05:17 GMT, Magnus Ihse Bursie wrote: >> make/scripts/compare.sh line 1457: >> >>> 1455: THIS_SEC_BIN="$THIS_SEC_DIR/sec-bin.zip" >>> 1456: if [ "$OPENJDK_TARGET_OS" = "windows" ]; then >>> 1457: JGSS_WINDOWS_BIN="jgss-windows-x64-bin.zip" >> >> This is now being defined for windows-aarch64 too, when it previously >> wasn't. That seems wrong, >> given the "x64" suffix. > > Well... this was broken on windows-aarch64 before, too, since then it would > have looked for `jgss-windows-i586-bin.zip`. > > I'm going to leave this as it is. Obviously there is a lot more work needed > to get the compare script running on windows-aarch64, and I seriously doubt > anyone care about that platform enough to spend that time (Microsoft > themselves seems to have all but abandoned the windows-aarch64 port...). @magicus @kimbarrett @shipilev Thanks for catching this. We want to get this working on Windows AArch64. I have filed https://bugs.openjdk.org/browse/JDK-8343857. - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1834839335
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v3]
On Thu, 7 Nov 2024 22:31:21 GMT, Sergey Chernyshev wrote: >> Cgroup V1 subsustem fails to initialize mounted controllers properly in >> certain cases, that may lead to controllers left undetected/inactive. We >> observed the behavior in CloudFoundry deployments, it affects also host >> systems. >> >> The relevant /proc/self/mountinfo line is >> >> >> 2207 2196 0:43 >> /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c >> /sys/fs/cgroup/cpu,cpuacct ro,nosuid,nodev,noexec,relatime master:25 - >> cgroup cgroup rw,cpu,cpuacct >> >> >> /proc/self/cgroup: >> >> >> 11:cpu,cpuacct:/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >> >> >> Here, Java runs inside containerized process that is being moved cgroups due >> to load balancing. >> >> Let's examine the condition at line 64 here >> https://github.com/openjdk/jdk/blob/55a7cf14453b6cd1de91362927b2fa63cba400a1/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp#L59-L72 >> It is always FALSE and the branch is never taken. The issue was spotted >> earlier by @jerboaa in >> [JDK-8288019](https://bugs.openjdk.org/browse/JDK-8288019). >> >> The original logic was intended to find the common prefix of `_root`and >> `cgroup_path` and concatenate the remaining suffix to the `_mount_point` >> (lines 67-68). That could lead to the following results: >> >> Example input >> >> _root = "/a" >> cgroup_path = "/a/b" >> _mount_point = "/sys/fs/cgroup/cpu,cpuacct" >> >> >> result _path >> >> "/sys/fs/cgroup/cpu,cpuacct/b" >> >> >> Here, cgroup_path comes from /proc/self/cgroup 3rd column. The man page >> (https://man7.org/linux/man-pages/man7/cgroups.7.html#NOTES) for control >> groups states: >> >> >> ... >>/proc/pid/cgroup (since Linux 2.6.24) >> This file describes control groups to which the process >> with the corresponding PID belongs. The displayed >> information differs for cgroups version 1 and version 2 >> hierarchies. >> For each cgroup hierarchy of which the process is a >> member, there is one entry containing three colon- >> separated fields: >> >> hierarchy-ID:controller-list:cgroup-path >> >> For example: >> >> 5:cpuacct,cpu,cpuset:/daemons >> ... >> [3] This field contains the pathname of the control group >>in the hierarchy to which the process belongs. This >>pathname is relative to the mount point of the >>hierarchy. >> >> >> This explicitly states the "pathname is relative t... > > Sergey Chernyshev has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' into JDK-8343191 > - patch reimplemented > - fix the logic that skips duplicate controller's mount points > - 8343191: Cgroup v1 subsystem fails to set subsystem path Hi Severin, thanks for this question. I didn't check cg v2 because the issue (NPE) was observed in v1 hosts only. I believe it's because v2 uses --cgroupns=private by default, in which cgroup is mounted at hierarchy leaf, so both `_root` and `cgroup_path` are `/`. It's an open question what happens if a process is moved between cgroups in v2 mode. I will look into it and file an issue if there are problems in v2. - PR Comment: https://git.openjdk.org/jdk/pull/21808#issuecomment-2465146149
Re: RFR: 8342707: Prepare Gatherers for graduation from Preview [v4]
On Fri, 8 Nov 2024 13:56:38 GMT, Viktor Klang wrote: >> Make final adjustments to drop PreviewFeature and updating the @ since >> markers. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Updating the copyright year of the Gatherer benchmarks The removal of preview toggles look good. Confirmed that since in stream packages and removal of preview toggles tests/micro look good. Side comment: With the `jvmArgs` cleared on benchmark `@Fork`, you can just make it `@Fork(1)` instead of `@Fork(value = 1)`. - Marked as reviewed by liach (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21686#pullrequestreview-2424229448
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v9]
On Fri, 8 Nov 2024 13:49:55 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 224 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Move remaining JEP 486 failing tests into correct groups. > - Move JEP 486 failing tests into hotspot_runtime group. > - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - ... and 214 more: https://git.openjdk.org/jdk/compare/d0077eec...6ad9192e Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2424915104
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v3]
On Fri, 1 Nov 2024 18:06:47 GMT, Alexey Ivanov wrote: > > I'd not looked at this test before but when I do the thing I noticed is > > that createPrivateValue is no longer used. But I don't see a problem with > > keeping the rest of the test. > > @prrace Do I understand correctly that _“`createPrivateValue` is no longer > used”_ means `MultiUIDefaults` is never used with `ProxyLazyValue`? > > The [`MultiUIDefaults` > class](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/MultiUIDefaults.java) > is used in `UIManager`: > > https://github.com/openjdk/jdk/blob/c82ad845e101bf5d97c0744377d68002907d4a0e/src/java.desktop/share/classes/javax/swing/UIManager.java#L198 I think I was just saying there appeared to be dead code in the test. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1835053637
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v10]
> This is an implementation of [JEP 483: Ahead-of-Time Class Loading & > Linking](https://openjdk.org/jeps/483). > > > Note: this is a combined PR of the following individual PRs > - https://github.com/openjdk/jdk/pull/20516 > - https://github.com/openjdk/jdk/pull/20517 > - https://github.com/openjdk/jdk/pull/20843 > - https://github.com/openjdk/jdk/pull/20958 > - https://github.com/openjdk/jdk/pull/20959 > - https://github.com/openjdk/jdk/pull/21049 > - https://github.com/openjdk/jdk/pull/21143 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 177 commits: - fixed merge with UseCompactObjectHeaders - Merge branch 'master' into jep-483-candidate - fixed merge - Merge branch 'master' of https://github.com/openjdk/jdk into merge-mainline - @DanHeidinga comment -- exit VM when runtimeSetup() fails - fixed merge - Merge branch 'master' into jep-483-candidate - 8343493: Perform module checks during MetaspaceShared::map_archives() - reverted changes in modules.cpp to make it easy to merge with mainline - Fixed whitespace; fixed minimal build - ... and 167 more: https://git.openjdk.org/jdk/compare/44ec501a...ce4c93ee - Changes: https://git.openjdk.org/jdk/pull/21642/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21642&range=09 Stats: 7193 lines in 107 files changed: 6358 ins; 513 del; 322 mod Patch: https://git.openjdk.org/jdk/pull/21642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21642/head:pull/21642 PR: https://git.openjdk.org/jdk/pull/21642
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v3]
On Thu, 7 Nov 2024 22:31:21 GMT, Sergey Chernyshev wrote: >> Cgroup V1 subsustem fails to initialize mounted controllers properly in >> certain cases, that may lead to controllers left undetected/inactive. We >> observed the behavior in CloudFoundry deployments, it affects also host >> systems. >> >> The relevant /proc/self/mountinfo line is >> >> >> 2207 2196 0:43 >> /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c >> /sys/fs/cgroup/cpu,cpuacct ro,nosuid,nodev,noexec,relatime master:25 - >> cgroup cgroup rw,cpu,cpuacct >> >> >> /proc/self/cgroup: >> >> >> 11:cpu,cpuacct:/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >> >> >> Here, Java runs inside containerized process that is being moved cgroups due >> to load balancing. >> >> Let's examine the condition at line 64 here >> https://github.com/openjdk/jdk/blob/55a7cf14453b6cd1de91362927b2fa63cba400a1/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp#L59-L72 >> It is always FALSE and the branch is never taken. The issue was spotted >> earlier by @jerboaa in >> [JDK-8288019](https://bugs.openjdk.org/browse/JDK-8288019). >> >> The original logic was intended to find the common prefix of `_root`and >> `cgroup_path` and concatenate the remaining suffix to the `_mount_point` >> (lines 67-68). That could lead to the following results: >> >> Example input >> >> _root = "/a" >> cgroup_path = "/a/b" >> _mount_point = "/sys/fs/cgroup/cpu,cpuacct" >> >> >> result _path >> >> "/sys/fs/cgroup/cpu,cpuacct/b" >> >> >> Here, cgroup_path comes from /proc/self/cgroup 3rd column. The man page >> (https://man7.org/linux/man-pages/man7/cgroups.7.html#NOTES) for control >> groups states: >> >> >> ... >>/proc/pid/cgroup (since Linux 2.6.24) >> This file describes control groups to which the process >> with the corresponding PID belongs. The displayed >> information differs for cgroups version 1 and version 2 >> hierarchies. >> For each cgroup hierarchy of which the process is a >> member, there is one entry containing three colon- >> separated fields: >> >> hierarchy-ID:controller-list:cgroup-path >> >> For example: >> >> 5:cpuacct,cpu,cpuset:/daemons >> ... >> [3] This field contains the pathname of the control group >>in the hierarchy to which the process belongs. This >>pathname is relative to the mount point of the >>hierarchy. >> >> >> This explicitly states the "pathname is relative t... > > Sergey Chernyshev has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' into JDK-8343191 > - patch reimplemented > - fix the logic that skips duplicate controller's mount points > - 8343191: Cgroup v1 subsystem fails to set subsystem path It looks to me that v2 mode is not affected, at least the way it is in v1. In v2 mode, cgroup is mounted either at leaf node (private namespace), or the complete hierarchy at /sys/fs/cgroup (host namespace). In host mode it works right away, as the full hierarchy is accessible. With a cgroup v2 created like this: sudo mkdir -p /sys/fs/cgroup/test echo 2 | sudo tee /sys/fs/cgroup/test/memory.max ``` The result would be [0.000s][debug][os,container] Detected optional pids controller entry in /proc/cgroups [0.001s][debug][os,container] Detected cgroups v2 unified hierarchy [0.001s][trace][os,container] Adjusting controller path for memory: /sys/fs/cgroup/test [0.001s][trace][os,container] Path to /memory.max is /sys/fs/cgroup/test/memory.max [0.001s][trace][os,container] Memory Limit is: 19488 In the private namespace (it's a default setting in v2 hosts), it may fail migrating the process between cgroups (a docker issue?). It may look like the cgroup files are not mapped at all, while `cgroup_path` appears to be set relative to the old cgroup (the old cgroup isn't mapped though). [0.000s][debug][os,container] Detected optional pids controller entry in /proc/cgroups [0.001s][debug][os,container] Detected cgroups v2 unified hierarchy [0.001s][trace][os,container] Adjusting controller path for memory: /sys/fs/cgroup/../../test [0.001s][trace][os,container] Path to /memory.max is /sys/fs/cgroup/../../test/memory.max [0.001s][debug][os,container] Open of file /sys/fs/cgroup/../../test/memory.max failed, No such file or directory [0.001s][trace][os,container] Memory Limit failed: -2 [0.001s][trace][os,container] Memory Limit is: -2 [0.001s][debug][os,container] container memory limit failed: -2, using host value 4105613312 [0.001s][trace][os,container] Path to /memory.max is /sys/fs/cgrou
Re: RFR: 8343804: Show the default time zone with -XshowSettings option
On Fri, 8 Nov 2024 04:40:15 GMT, Jaikiran Pai wrote: > Hello Naoto, this looks good to me. The test itself could assert that the > printed timezone is indeed the default timezone, but if you prefer it in its > current form, that's fine too. Thank you, Jai. I considererd that too, i.e. affirming the actual tz value. I decided not to do it aligning with other tests that do not affirm their values either. - PR Comment: https://git.openjdk.org/jdk/pull/21965#issuecomment-2465370971
Re: RFR: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs) [v3]
> Please review this patch which removes unnecessary `@SuppressWarnings` > annotations. Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' into SuppressWarningsCleanup-core-libs - Update copyright years. - Remove a few more @SuppressWarnings annotations. - Remove a few more @SuppressWarnings annotations. - Update copyright years. - Merge branch 'master' into SuppressWarningsCleanup-core-libs - Merge branch 'master' into SuppressWarningsCleanup-core-libs - Remove unnecessary @SuppressWarnings annotations. - Changes: - all: https://git.openjdk.org/jdk/pull/21852/files - new: https://git.openjdk.org/jdk/pull/21852/files/7842dfea..a64d5594 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21852&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21852&range=01-02 Stats: 131613 lines in 758 files changed: 103986 ins; 9698 del; 17929 mod Patch: https://git.openjdk.org/jdk/pull/21852.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21852/head:pull/21852 PR: https://git.openjdk.org/jdk/pull/21852
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v33]
On Fri, 8 Nov 2024 11:31:40 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Inline buildJniFunctionName Still looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2424652542
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v33]
On Fri, 8 Nov 2024 18:26:25 GMT, Saint Wesonga wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Inline buildJniFunctionName
>
> src/hotspot/os/posix/include/jvm_md.h line 41:
>
>> 39:
>> 40: #define JNI_ONLOAD_SYMBOLS {"JNI_OnLoad"}
>> 41: #define JNI_ONUNLOAD_SYMBOLS {"JNI_OnUnload"}
>
> are these changes related to the Windows 32-bit x86 port?
After removal of Windows 32-bit x86 port, all definitions of these macros are
identical, so are merged into jvm.h.
There is additional followup work involving these: see
https://bugs.openjdk.org/browse/JDK-8343703.
> src/hotspot/os/posix/os_posix.cpp line 699:
>
>> 697: }
>> 698:
>> 699: void os::print_jni_name_prefix_on(outputStream* st, int args_size) {
>
> are these changes related to the Windows 32-bit x86 port?
As part of removal of Windows 32-bit x86 port, these functions are no longer
needed nor called, and all
definitions removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1834900456
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1834900337
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v33]
On Fri, 8 Nov 2024 11:31:40 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Inline buildJniFunctionName Marked as reviewed by sweso...@github.com (no known OpenJDK username). - PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2424836232
Integrated: 8343804: Show the default time zone with -XshowSettings option
On Thu, 7 Nov 2024 23:36:39 GMT, Naoto Sato wrote: > A small enhancement in the Java launcher. For diagnostic purposes, display > the default time zone ID with the `-XshowSettings` option. This pull request has now been integrated. Changeset: 03298558 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/0329855831102a48abf14b5befc933f84dfd3460 Stats: 9 lines in 2 files changed: 7 ins; 0 del; 2 mod 8343804: Show the default time zone with -XshowSettings option Reviewed-by: iris, jpai - PR: https://git.openjdk.org/jdk/pull/21965
Re: RFR: 8343804: Show the default time zone with -XshowSettings option
On Thu, 7 Nov 2024 23:36:39 GMT, Naoto Sato wrote: > A small enhancement in the Java launcher. For diagnostic purposes, display > the default time zone ID with the `-XshowSettings` option. Thank you for your reviews! - PR Comment: https://git.openjdk.org/jdk/pull/21965#issuecomment-2465689586
Re: RFR: 8341137: Optimize long vector multiplication using x86 VPMUL[U]DQ instruction [v2]
On Fri, 8 Nov 2024 08:15:32 GMT, Jatin Bhateja wrote:
>> This patch optimizes LongVector multiplication by inferring VPMUL[U]DQ
>> instruction for following IR pallets.
>>
>>
>>MulVL ( AndV SRC1, 0x) ( AndV SRC2, 0x)
>>MulVL (URShiftVL SRC1 , 32) (URShiftVL SRC2, 32)
>>MulVL (URShiftVL SRC1 , 32) ( AndV SRC2, 0x)
>>MulVL ( AndV SRC1, 0x) (URShiftVL SRC2 , 32)
>>MulVL (VectorCastI2X SRC1) (VectorCastI2X SRC2)
>>MulVL (RShiftVL SRC1 , 32) (RShiftVL SRC2, 32)
>>
>>
>>
>> A 64x64 bit multiplication produces 128 bit result, and can be performed
>> by individually multiplying upper and lower double word of multiplier with
>> multiplicand and assembling the partial products to compute full width
>> result. Targets supporting vector quadword multiplication have separate
>> instructions to compute upper and lower quadwords for 128 bit result.
>> Therefore existing VectorAPI multiplication operator expects shape
>> conformance between source and result vectors.
>>
>> If upper 32 bits of quadword multiplier and multiplicand is always set to
>> zero then result of multiplication is only dependent on the partial product
>> of their lower double words and can be performed using unsigned 32 bit
>> multiplication instruction with quadword saturation. Patch matches this
>> pattern in a target dependent manner without introducing new IR node.
>>
>> VPMUL[U]DQ instruction performs [unsigned] multiplication between even
>> numbered doubleword lanes of two long vectors and produces 64 bit result.
>> It has much lower latency compared to full 64 bit multiplication instruction
>> "VPMULLQ", in addition non-AVX512DQ targets does not support direct quadword
>> multiplication, thus we can save redundant partial product for zeroed out
>> upper 32 bits. This results into throughput improvements on both P and E
>> core Xeons.
>>
>> Please find below the performance of [XXH3 hashing benchmark
>> ](https://mail.openjdk.org/pipermail/panama-dev/2024-July/020557.html)included
>> with the patch:-
>>
>>
>> Sierra Forest :-
>>
>> Baseline:-
>> Benchmark (SIZE) Mode CntScore
>> Error Units
>> VectorXXH3HashingBenchmark.hashingKernel1024 thrpt2 806.228
>> ops/ms
>> VectorXXH3HashingBenchmark.hashingKernel2048 thrpt2 403.044
>> ops/ms
>> VectorXXH3HashingBenchmark.hashingKernel4096 thrpt2 200.641
>> ops/ms
>> VectorXXH3HashingBenchmark.hashingKernel8192 thrpt2 100.664
>> ops/ms
>>
>> With Optimizati...
>
> Jatin Bhateja has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Creating specialized IR to shield pattern from subsequent transforms in
> optimization pipeline
In the latest version you added new Ideal nodes (`MulIVL` and `MulUIVL`). I
don't see a compelling reason to do so. IMO matcher functionality is more than
enough to cover `VPMULDQ` case. `MulIVL` is equivalent to `MulVL` +
`has_int_inputs()` predicate. For `MulUIVL` you additionally do input rewiring
(using `forward_masked_input`), but (1) `AndV src (Replicate 0x))`
operands can be easily detected on matcher side (with an extra AD instruction);
and (2) such optimization is limited because it is valid only for `0x`
case while `has_uint_inputs() == true` for `C <= 0x`.
So, IMO `MulIVL` and `MulUIVL` nodes just add noise in Ideal graph without
improving situation during matching.
src/hotspot/share/opto/vectornode.cpp line 2132:
> 2130: // Directly forward masked inputs if
> 2131: if (n->Opcode() == Op_AndV) {
> 2132: return n->in(1)->Opcode() == Op_Replicate ? n->in(2) : n->in(1);
This particular check should ensure that Replicate constant is `0x`.
-
PR Review: https://git.openjdk.org/jdk/pull/21244#pullrequestreview-2424864897
PR Review Comment: https://git.openjdk.org/jdk/pull/21244#discussion_r1835023354
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v48]
On Fri, 8 Nov 2024 17:07:55 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright headers Marked as reviewed by mchung (Reviewer). Thanks for the update and skimmed through the tests. Looks good. I also agree that better to fail early when detecting patched modules as described in JDK-8343839. - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2425219797 PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2466054872
RFR: 8331467: ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider
Use the built-in file system provider rather than the custom file system provider. Add "public static FileSystemProvider create" method in DefaultFileSystemProvider which is from java8API to be compatible against runtime. - Commit messages: - 8331467: Fix JDK-8331467 ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider Changes: https://git.openjdk.org/jdk/pull/21556/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21556&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331467 Stats: 33 lines in 5 files changed: 32 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21556.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21556/head:pull/21556 PR: https://git.openjdk.org/jdk/pull/21556
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v6]
> After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Copyright in VersionInfo.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/21616/files - new: https://git.openjdk.org/jdk/pull/21616/files/68767cbe..5e9039fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v2]
On Thu, 31 Oct 2024 19:07:42 GMT, Chris Plummer wrote: >>> I do wonder if mutex support can be implemented for Windows with >>> Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it >>> would be nice to have. Shame threads.h is not available with some Visual >>> Studio versions we support, or at all with gcc. mtx_lock/unlock would be a >>> nice solution to this problem >> >> I'll also start looking into this in the meantime, unless you explicitly >> want me not to do so > >> > I do wonder if mutex support can be implemented for Windows with >> > Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it >> > would be nice to have. Shame threads.h is not available with some Visual >> > Studio versions we support, or at all with gcc. mtx_lock/unlock would be a >> > nice solution to this problem >> >> I'll also start looking into this in the meantime, unless you explicitly >> want me not to do so > > Feel free to if you'd like to. Personally I don't consider it to be that > important. @plummercj @alexeysemenyukoracle @sashamatveev reawaiting review - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2466039622
Withdrawn: 8331467: ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider
On Thu, 17 Oct 2024 03:37:38 GMT, jyxzwd wrote: > Use the built-in file system provider rather than the custom file system > provider. > Add "public static FileSystemProvider create" method in > DefaultFileSystemProvider which is from java8API to be compatible against > runtime. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/21556
RFR: 8331467: ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider
Use the built-in file system provider rather than the custom file system provider. Add "public static FileSystemProvider create" method in DefaultFileSystemProvider which is from java8API to be compatible against runtime. - Commit messages: - 8331467: Fix JDK-8331467 ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider Changes: https://git.openjdk.org/jdk/pull/21997/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21997&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331467 Stats: 33 lines in 5 files changed: 32 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21997.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21997/head:pull/21997 PR: https://git.openjdk.org/jdk/pull/21997
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v6]
On Sat, 9 Nov 2024 04:26:55 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Copyright in VersionInfo.cpp Marked as reviewed by asemenyuk (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21616#pullrequestreview-2425219022
RFR: 8343780: Add since checker tests to the Tools area modules and add missing @since to jdk.jfr.Recording
Can I please get a review for this PR that add tests to verify the value of `@since` tags to the Tools area modules. The test is described in this [email](https://mail.openjdk.org/pipermail/jdk-dev/2024-October/009474.html). The benefit from this is helping API authors and reviewer validate the accuracy of `@since` in their source code (and subsequently, in the generated documentation). And lessen the burden on Reviewers. The test has been added for `java.base` and a few other modules and has helped catch some bugs before they make it to the JDK. Notes: - I have also added an `@since` tag that was missing. - You will notice there is no test for the `jdk.jfr` module, it will be added in a future PR because it requires special handling. (JFR used to be a commercial feature and this requires special handling to be added for it in the test) TIA - Commit messages: - remove backticks - 8343780 Changes: https://git.openjdk.org/jdk/pull/21982/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21982&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8343780 Stats: 16 lines in 6 files changed: 1 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/21982.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21982/head:pull/21982 PR: https://git.openjdk.org/jdk/pull/21982
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v47]
On Fri, 8 Nov 2024 10:42:15 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused imports from VMProps Thanks for the review! > You will need to bump the copyright header of several jlink src files before > integrating. OK. Let me work on that. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2465228449
Integrated: 8343844: Add benchmarks for superword/autovectorization in FFM BulkOperations
On Fri, 8 Nov 2024 16:13:28 GMT, Per Minborg wrote: > This PR proposed to add a few benchmarks using superword/autovectorization This pull request has now been integrated. Changeset: 2fb0c1dd Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/2fb0c1dd62f1c690cf6b78f5cdfe18b10c991886 Stats: 36 lines in 1 file changed: 36 ins; 0 del; 0 mod 8343844: Add benchmarks for superword/autovectorization in FFM BulkOperations Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/21988
Re: RFR: 8343844: Add benchmarks for superword/autovectorization in FFM BulkOperations
On Fri, 8 Nov 2024 16:13:28 GMT, Per Minborg wrote: > This PR proposed to add a few benchmarks using superword/autovectorization Marked as reviewed by mcimadamore (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21988#pullrequestreview-2424305115
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v48]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Update copyright headers - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/0bf4843d..2042fc34 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=47 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=46-47 Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v48]
On Fri, 8 Nov 2024 17:07:55 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright headers Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2424410814
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v33]
On Fri, 8 Nov 2024 11:31:40 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Inline buildJniFunctionName Re-approve. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2424696022
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v47]
On Fri, 8 Nov 2024 10:42:15 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused imports from VMProps Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2424083294
Re: RFR: 8343793: Test java/foreign/TestMemorySession.java is timing out
On Fri, 8 Nov 2024 11:56:08 GMT, Quan Anh Mai wrote:
> Hi,
>
> This patch fixes the deadlock in `TestMemorySession#testAcquireCloseRace`.
> The lock-step spin lock is implemented as with `lock` being an
> `AtomicInteger`:
>
> // Keep the 2 threads operating on the same scope
> int k = lock.getAndAdd(1) + 1;
> while (k != i * 2) {
> Thread.onSpinWait();
> k = lock.get();
> }
>
> Given the initial condition:
>
> Thread 1: i = 0
> Thread 2: i = 0
> lock: -2
>
> The `lock` then undergoes the following operations:
>
>
>
> Thread 1 Thread 2 lock value
> getAndAdd(1)-1
> getAndAdd(1) 0 -> Thread 2 then continues
> its next iteration, its i value is now 1
> getAndAdd(1) 1 -> Thread 2 reaches the next
> iteration before thread 1 has a chance to read the value 0
> get()1 -> Thread 1 now cannot
> proceed because it missed the value 0
>get() 1 -> Thread 2 now cannot
> proceed because lock can never reach 2
>
>
> The solution is to not rely on the exact value of the lock but instead
> whether the lock has passed the expected value.
>
> Testing: I have run this test several hundreds times and got no failure while
> without this patch I encountered a timeout every approximately 30 times.
>
> Please take a look, thanks a lot.
I think I also prefer Maurizio's version.
-
PR Comment: https://git.openjdk.org/jdk/pull/21976#issuecomment-2465114542
Re: RFR: 8343793: Test java/foreign/TestMemorySession.java is timing out
On Fri, 8 Nov 2024 11:56:08 GMT, Quan Anh Mai wrote:
> Hi,
>
> This patch fixes the deadlock in `TestMemorySession#testAcquireCloseRace`.
> The lock-step spin lock is implemented as with `lock` being an
> `AtomicInteger`:
>
> // Keep the 2 threads operating on the same scope
> int k = lock.getAndAdd(1) + 1;
> while (k != i * 2) {
> Thread.onSpinWait();
> k = lock.get();
> }
>
> Given the initial condition:
>
> Thread 1: i = 0
> Thread 2: i = 0
> lock: -2
>
> The `lock` then undergoes the following operations:
>
>
>
> Thread 1 Thread 2 lock value
> getAndAdd(1)-1
> getAndAdd(1) 0 -> Thread 2 then continues
> its next iteration, its i value is now 1
> getAndAdd(1) 1 -> Thread 2 reaches the next
> iteration before thread 1 has a chance to read the value 0
> get()1 -> Thread 1 now cannot
> proceed because it missed the value 0
>get() 1 -> Thread 2 now cannot
> proceed because lock can never reach 2
>
>
> The solution is to not rely on the exact value of the lock but instead
> whether the lock has passed the expected value.
>
> Testing: I have run this test several hundreds times and got no failure while
> without this patch I encountered a timeout every approximately 30 times.
>
> Please take a look, thanks a lot.
Sure, I have refactored the test, no failure after 200 runs.
-
PR Comment: https://git.openjdk.org/jdk/pull/21976#issuecomment-2465276187
Re: RFR: 8342707: Prepare Gatherers for graduation from Preview [v4]
On Fri, 8 Nov 2024 13:56:38 GMT, Viktor Klang wrote: >> Make final adjustments to drop PreviewFeature and updating the @ since >> markers. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Updating the copyright year of the Gatherer benchmarks The current form makes it easy to add jvm args in the future, too. It's totally fine. - PR Comment: https://git.openjdk.org/jdk/pull/21686#issuecomment-2465277946
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v9]
On Fri, 1 Nov 2024 20:26:45 GMT, Alexey Ivanov wrote: >>> Is it intentional? >> >> It was probably by mistake. but you are right, I see it mentioned already in >> the doc. I don't think we need to mention it again? > > It has a value… when it's mentioned with `@see`, the link is present in the > *See Also* section, as you can see in the the specification of > [`MouseInfo.getPointerInfo()`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/MouseInfo.html#getPointerInfo()). > > Without the `@see` tag, one has to read the entire description to find the > link. > > It looks subtle. We can restore the `@see`-link later if deemed necessary. > > Does anyone else have an opinion? I can add it back if it is more convenient and readable to have the `@see` tag. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1834883137
RFR: 8343844: Add benchmarks for superword/autovectorization in FFM BulkOperations
This PR proposed to add a few benchmarks using superword/autovectorization - Commit messages: - Add benchmarks Changes: https://git.openjdk.org/jdk/pull/21988/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21988&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8343844 Stats: 36 lines in 1 file changed: 36 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21988.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21988/head:pull/21988 PR: https://git.openjdk.org/jdk/pull/21988
Re: RFR: 8342707: Prepare Gatherers for graduation from Preview [v4]
On Fri, 8 Nov 2024 16:17:39 GMT, Chen Liang wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updating the copyright year of the Gatherer benchmarks > > The removal of preview toggles look good. Confirmed that since in stream > packages and removal of preview toggles tests/micro look good. > > Side comment: With the `jvmArgs` cleared on benchmark `@Fork`, you can just > make it `@Fork(1)` instead of `@Fork(value = 1)`. @liach >Side comment: With the jvmArgs cleared on benchmark @Fork, you can just make >it @Fork(1) instead of @Fork(value = 1). Yeah, I opted to keep it as-is to make the delta between old-and-new low (and to make adding additional params slightly faster). I'd be happy to change it if you insist, tho! :) - PR Comment: https://git.openjdk.org/jdk/pull/21686#issuecomment-2465208982
Re: RFR: 8343793: Test java/foreign/TestMemorySession.java is timing out [v2]
> Hi,
>
> This patch fixes the deadlock in `TestMemorySession#testAcquireCloseRace`.
> The lock-step spin lock is implemented as with `lock` being an
> `AtomicInteger`:
>
> // Keep the 2 threads operating on the same scope
> int k = lock.getAndAdd(1) + 1;
> while (k != i * 2) {
> Thread.onSpinWait();
> k = lock.get();
> }
>
> Given the initial condition:
>
> Thread 1: i = 0
> Thread 2: i = 0
> lock: -2
>
> The `lock` then undergoes the following operations:
>
>
>
> Thread 1 Thread 2 lock value
> getAndAdd(1)-1
> getAndAdd(1) 0 -> Thread 2 then continues
> its next iteration, its i value is now 1
> getAndAdd(1) 1 -> Thread 2 reaches the next
> iteration before thread 1 has a chance to read the value 0
> get()1 -> Thread 1 now cannot
> proceed because it missed the value 0
>get() 1 -> Thread 2 now cannot
> proceed because lock can never reach 2
>
>
> The solution is to not rely on the exact value of the lock but instead
> whether the lock has passed the expected value.
>
> Testing: I have run this test several hundreds times and got no failure while
> without this patch I encountered a timeout every approximately 30 times.
>
> Please take a look, thanks a lot.
Quan Anh Mai has updated the pull request incrementally with one additional
commit since the last revision:
refactor the test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/21976/files
- new: https://git.openjdk.org/jdk/pull/21976/files/940b7d04..e77ce9e8
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=21976&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=21976&range=00-01
Stats: 24 lines in 1 file changed: 10 ins; 6 del; 8 mod
Patch: https://git.openjdk.org/jdk/pull/21976.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/21976/head:pull/21976
PR: https://git.openjdk.org/jdk/pull/21976
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v5]
On Fri, 8 Nov 2024 13:39:08 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove neutralLangId in VersionInfo.cpp src/jdk.jpackage/windows/native/libjpackage/VersionInfo.cpp line 1: > 1: /* Copyright year needs to be updated. "2020," -> "2020, 2024,". - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1835031208
Re: RFR: 8310691: [REDO] [vectorapi] Refactor VectorShuffle implementation [v2]
On Sun, 6 Oct 2024 10:24:53 GMT, Quan Anh Mai wrote: >> Quan Anh Mai has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains one commit: >> >> [vectorapi] Refactor VectorShuffle implementation > > I have adapted the patch in accordance with > https://github.com/openjdk/jdk/pull/20634, I moved the index wrapping into C2 > instead of making it a separate step as I think it seems clearer. Also, I > think in the future we can eliminate this step so putting it in C2 would make > the progress easier. > > Please take a look, thanks a lot. @merykitty Could you please merge with the latest and resolve conflicts? - PR Comment: https://git.openjdk.org/jdk/pull/21042#issuecomment-2465889165
Re: RFR: 8341137: Optimize long vector multiplication using x86 VPMUL[U]DQ instruction [v2]
On Fri, 8 Nov 2024 20:25:10 GMT, Vladimir Ivanov wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Creating specialized IR to shield pattern from subsequent transforms in
>> optimization pipeline
>
> src/hotspot/share/opto/vectornode.cpp line 2132:
>
>> 2130: // Directly forward masked inputs if
>> 2131: if (n->Opcode() == Op_AndV) {
>> 2132: return n->in(1)->Opcode() == Op_Replicate ? n->in(2) : n->in(1);
>
> This particular check should ensure that Replicate constant is `0x`.
Yes, this should ensure 0x.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21244#discussion_r1835148834
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Thu, 7 Nov 2024 13:27:41 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with six > additional commits since the last revision: > > - Mandy's feedback > - Remove "jlink.runtime.linkable" property from VMProps > >It's no longer used in any tests. > - Fix JLinkDedupTestBatchSizeOne.java > - Fix plugins/IncludeLocalesPluginTest.java > >Similar to GenerateJLIClassesPluginTest.java, the default module path >doesn't need to be explicitly specified. > - Fix plugins/GenerateJLIClassesPluginTest.java test > >The test doesn't need the default module path and default jmods >generated from the helper. Using this approach makes it work for >linkable run-time images as well. > - Simplify runtimeImage tests test/jtreg-ext/requires/VMProps.java line 28: > 26: import java.lang.module.ModuleFinder; > 27: import java.lang.module.ModuleReader; > 28: import java.lang.module.ModuleReference; I assume they aren't needed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834029491
Integrated: 8341399: Add since checker tests to the langtools modules
On Wed, 16 Oct 2024 16:23:41 GMT, Nizar Benalla wrote: > Can I get a review for this patch that adds `@since` checker tests to the > following modules: java.compiler, jdk.compiler, jdk.javadoc and jdk.jdeps. > The initial test for `java.base` has been integrated in > [JDK-8331051](https://bugs.openjdk.org/browse/JDK-8331051). > > The jtreg comments are almost the same as the ones for the `java.base` test, > only the Bug ID and the module name being passed to the test are different. > > I've made a small change to `test/jdk/TEST.groups` to include the new tests. > > Here are the emails > [[1](https://mail.openjdk.org/pipermail/jdk-dev/2024-June/009160.html)] > [[2](https://mail.openjdk.org/pipermail/jdk-dev/2024-October/009474.html)] in > `jdk-dev` describing how the tests work and how to run them. > > TIA This pull request has now been integrated. Changeset: 2e58ede1 Author:Nizar Benalla URL: https://git.openjdk.org/jdk/commit/2e58ede18c7cfe7364a8d6a630989b0ff2ea6447 Stats: 219 lines in 9 files changed: 184 ins; 34 del; 1 mod 8341399: Add since checker tests to the langtools modules Reviewed-by: vromero - PR: https://git.openjdk.org/jdk/pull/21542
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Fri, 8 Nov 2024 09:44:01 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request incrementally with six >> additional commits since the last revision: >> >> - Mandy's feedback >> - Remove "jlink.runtime.linkable" property from VMProps >> >>It's no longer used in any tests. >> - Fix JLinkDedupTestBatchSizeOne.java >> - Fix plugins/IncludeLocalesPluginTest.java >> >>Similar to GenerateJLIClassesPluginTest.java, the default module path >>doesn't need to be explicitly specified. >> - Fix plugins/GenerateJLIClassesPluginTest.java test >> >>The test doesn't need the default module path and default jmods >>generated from the helper. Using this approach makes it work for >>linkable run-time images as well. >> - Simplify runtimeImage tests > > test/jtreg-ext/requires/VMProps.java line 28: > >> 26: import java.lang.module.ModuleFinder; >> 27: import java.lang.module.ModuleReader; >> 28: import java.lang.module.ModuleReference; > > I assume they aren't needed now. True. > test/langtools/tools/javac/plugin/AutostartPlugins.java line 33: > >> 31: * jdk.jlink >> 32: * @build toolbox.ToolBox toolbox.JavacTask toolbox.JarTask >> 33: * @run main/othervm AutostartPlugins > > Why do the two javac plugin tests changed to /othervm? I assume they fail > with a JDK built with --generate-linkable-runtime but I can't immediately see > why. jtreg patches the `java.base` module and those langtools tests *link* from the run-time image (in such a config). Having a patched JDK module is not supported (and I don't think we ever should). Running in a separate JVM doesn't have this problem. See https://github.com/openjdk/jdk/pull/14787/commits/d8e1e834508589725adb8d70acb862a1270678ca. Does that make sense? - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834057484 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834056602
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v31]
On Thu, 7 Nov 2024 12:16:23 GMT, Aleksey Shipilev wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove FIXME > > I really wish we did not mess with `_stdcall` and `_cdecl` in this PR. A > future me chasing a bug would be surprised if we broke 64-bit Windows with > this "cleanup" PR. I think the PR like this should only carry the changes > that are provably, uncontroversially, visibly safe. Everything else that has > any chance to do semantic changes, should be done in follow-up PRs, IMO. > @shipilev Could you consider accepting the existing `__stdcall` changes in > this PR? That seems like the easiest way out of satisfying everyones opinions > here.. Sure, fine. - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2464299781
Re: RFR: 8335989: JEP 494: Implement Module Import Declarations (Second Preview) [v6]
> This is a current patch for module imports declarations, second preview. At > least the JEP number and preview revision will need to be updated in > `jdk.internal.javac.PreviewFeature.Feature`, but otherwise I believe this is > ready to receive feedback. > > The main changes are: > - `requires transitive java.base;` is permitted, under the preview flag. Both > javac and the runtime module system are updated to accept this directive when > preview is enabled. > - the `java.se` module is using `requires transitive java.base;`, and is > deemed to be participating in preview, so its classfile version is not > tainted. Runtime is updated to access `requires transitive java.base;` in any > `java.*`, considering all of them to be participating in preview. Can be > tighten up to only `java.se` is desired. > - the types imported through module imports can be shadowed using on-demand > imports. So, for example, having: > > import module java.base; > import module java.desktop; > ... > List l;//ambigous > > but: > > import module java.base; > import module java.desktop; > import java.util.*; > ... > List l;//not ambigous, reference to java.util.List Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits: - Merge branch 'master' into JDK-8335989 - Moving operators to the beginning of line, as suggested. - Updating PreviewFeature metadata - Cleanup. - Merge branch 'master' into JDK-8335989 - Merge branch 'master' into JDK-8335989 - Reflecting review feedback. - Cleanup. - Cleanup. - Fixing tests - ... and 10 more: https://git.openjdk.org/jdk/compare/a10b1ccd...21c835ca - Changes: https://git.openjdk.org/jdk/pull/21431/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21431&range=05 Stats: 762 lines in 28 files changed: 580 ins; 46 del; 136 mod Patch: https://git.openjdk.org/jdk/pull/21431.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21431/head:pull/21431 PR: https://git.openjdk.org/jdk/pull/21431
Re: RFR: 8341137: Optimize long vector multiplication using x86 VPMUL[U]DQ instruction [v2]
> This patch optimizes LongVector multiplication by inferring VPMUL[U]DQ > instruction for following IR pallets. > > >MulVL ( AndV SRC1, 0x) ( AndV SRC2, 0x) >MulVL (URShiftVL SRC1 , 32) (URShiftVL SRC2, 32) >MulVL (URShiftVL SRC1 , 32) ( AndV SRC2, 0x) >MulVL ( AndV SRC1, 0x) (URShiftVL SRC2 , 32) >MulVL (VectorCastI2X SRC1) (VectorCastI2X SRC2) >MulVL (RShiftVL SRC1 , 32) (RShiftVL SRC2, 32) > > > > A 64x64 bit multiplication produces 128 bit result, and can be performed by > individually multiplying upper and lower double word of multiplier with > multiplicand and assembling the partial products to compute full width > result. Targets supporting vector quadword multiplication have separate > instructions to compute upper and lower quadwords for 128 bit result. > Therefore existing VectorAPI multiplication operator expects shape > conformance between source and result vectors. > > If upper 32 bits of quadword multiplier and multiplicand is always set to > zero then result of multiplication is only dependent on the partial product > of their lower double words and can be performed using unsigned 32 bit > multiplication instruction with quadword saturation. Patch matches this > pattern in a target dependent manner without introducing new IR node. > > VPMUL[U]DQ instruction performs [unsigned] multiplication between even > numbered doubleword lanes of two long vectors and produces 64 bit result. It > has much lower latency compared to full 64 bit multiplication instruction > "VPMULLQ", in addition non-AVX512DQ targets does not support direct quadword > multiplication, thus we can save redundant partial product for zeroed out > upper 32 bits. This results into throughput improvements on both P and E core > Xeons. > > Please find below the performance of [XXH3 hashing benchmark > ](https://mail.openjdk.org/pipermail/panama-dev/2024-July/020557.html)included > with the patch:- > > > Sierra Forest :- > > Baseline:- > Benchmark (SIZE) Mode CntScore Error > Units > VectorXXH3HashingBenchmark.hashingKernel1024 thrpt2 806.228 > ops/ms > VectorXXH3HashingBenchmark.hashingKernel2048 thrpt2 403.044 > ops/ms > VectorXXH3HashingBenchmark.hashingKernel4096 thrpt2 200.641 > ops/ms > VectorXXH3HashingBenchmark.hashingKernel8192 thrpt2 100.664 > ops/ms > > With Optimization:- > Benchmark (SIZE) Mode ... Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision: Creating specialized IR to shield pattern from subsequent transforms in optimization pipeline - Changes: - all: https://git.openjdk.org/jdk/pull/21244/files - new: https://git.openjdk.org/jdk/pull/21244/files/43320063..613f491b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21244&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21244&range=00-01 Stats: 69 lines in 7 files changed: 57 ins; 3 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/21244.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21244/head:pull/21244 PR: https://git.openjdk.org/jdk/pull/21244
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 23 Aug 2024 21:02:25 GMT, Gerard Ziemski wrote:
> Is the plan to check-in the fix with both paths? Or are we going to remove
> the linked-list based one after the review?
The plan is to have both versions available at run-time. In this plan, we will
add JVM options to let the user to select either of them. This can be used for:
1️⃣ stepping back if new version fails in some use-cases, and also for 2️⃣
running benchmarks and comparing the results of both.
> I still haven't run the benchmarks to verify the speed increase promise.
> Ideally, I would like to do this with my mechanism, but will only do it, if I
> can manage it without getting sucked into working on the mechanism itself.
>
> I will definitively want to run the provided microbenchmarks though.
In `test_vmt_with_tree.cpp`, two versions are compared. One of the tests
(`PerformanceComparison`) is comparing the time they take for the same
operation. This test is skipped for now, because its expectation of
time-improvement fails. Until the time we still did not merge all other related
changes (like the `copy_flag` or `use_tag_inplace`,
[#20330](https://github.com/openjdk/jdk/pull/20330)) to this PR we cannot
expect the time to be improved.
> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 279:
>
>> 277: "Cannot commit verification bitmap
>> memory");
>> 278: }
>> 279: MemTracker::record_virtual_memory_type(verify_bitmap.base(),
>> verify_bitmap..size(), mtGC);
>
> Does this even compile?
No it doesn't, you're right. Shenandoah is not in the main build.
I thought that GHA builds do this but seems not.
Fixed. Thanks.
> src/hotspot/share/nmt/memReporter.cpp line 440:
>
>> 438: if (all_committed) {
>> 439: if (MemTracker::is_using_sorted_link_list()) {
>> 440: CommittedRegionIterator itr =
>> reserved_rgn->iterate_committed_regions();
>
> Indentation.
Done.
> src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 94:
>
>> 92: if (si._stack_index < 0 || si._stack_index >= _stacks.length()) {
>> 93: return _fake_stack;
>> 94: }
>
> Is that a leftover from debugging?
>
> Shouldn't this be an `assert` in final code?
You are right. Since the next lines check the `-1` as special case; this check
should be either an `assert` or logged. I wait for @jdksjolen, as this is his
code.
> src/hotspot/share/nmt/nmtTreap.hpp line 219:
>
>> 217: }
>> 218: last_seen = node;
>> 219: return true;
>
> Why are we returning a `bool` value in `void` function?
That is returning from the lambda function.
In `visit_in_order(F func)`, when function `func` (in our case the lambda)
returns `false` it means the iteration should stop. Otherwise, if it returns
`true` it means to continue iterating.
> src/hotspot/share/nmt/nmtTreap.hpp line 320:
>
>> 318: }
>> 319: head = to_visit.pop();
>> 320: if(!f(head))
>
> Needs space `if (!f(head))`
Fixed.
> src/hotspot/share/nmt/nmtTreap.hpp line 348:
>
>> 346: const int cmp_to = COMPARATOR::cmp(head->key(), to);
>> 347: if (cmp_from >= 0 && cmp_to < 0) {
>> 348: if(!f(head))
>
> Needs space `if (!f(head))`
Done.
> src/hotspot/share/nmt/nmtTreap.hpp line 356:
>
>> 354: head = nullptr;
>> 355: }
>> 356:
>
> Remove.
Done
-
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2309507439
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2378923225
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778342931
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713348422
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722829439
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778361486
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722821927
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722822141
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722822343
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Thu, 1 Aug 2024 15:44:32 GMT, Afshin Zafari wrote:
> - `VMATree` is used instead of `SortedLinkList` in new class
> `VirtualMemoryTrackerWithTree`.
> - A wrapper/helper `RegionTree` is made around VMATree to make some calls
> easier.
> - Both old and new versions exist in the code and can be selected via
> `MemTracker::set_version()`
> - `find_reserved_region()` is used in 4 cases, it will be removed in further
> PRs.
> - All tier1 tests pass except one ~that expects a 50% increase in committed
> memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167.
> - Adding a runtime flag for selecting the old or new version can be added
> later.
> - Some performance tests are added for new version, VMATree and Treap, to
> show the idea and should be improved later. Based on the results of comparing
> speed of VMATree and VMT, VMATree shows ~40x faster response time.
Changes requested by gziemski (Reviewer).
Changes requested by gziemski (Reviewer).
More review feedback...
Changes requested by gziemski (Reviewer).
Are we keeping both the linked list based and VMATree based implementations
just for the review process, and we will drop the linked list one, once we are
done with the review, or are they both going to be checked in?
Is there a concrete advantage here for using lambda expression when iterating
committed regions? I'm asking because personally I find
while ((committed_rgn = itr.next()) != nullptr) {
print_committed_rgn(committed_rgn);
}
simpler and more compact, hence easier to understand, than
CommittedMemoryRegion cmr;
VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn,
&cmr, [&](CommittedMemoryRegion* crgn) {
print_committed_rgn(crgn);
return true;
});
Same here:
if (MemTracker::is_using_tree()) {
CommittedMemoryRegion cmr;
bool reserved_and_committed = false;
VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn,
&cmr,
[&](CommittedMemoryRegion* committed_rgn) {
if (committed_rgn->size() == reserved_rgn->size() &&
committed_rgn->call_stack()->equals(*stack)) {
// One region spanning the entire reserved region, with the same
stack trace.
// Don't print this regions because the "reserved and committed" line
above
// already indicates that the region is committed.
reserved_and_committed = true;
return false;
}
return true;
});
if (reserved_and_committed)
return;
}
}
We are setting `reserved_and_committed` inside lambda, then we need to check it
outside. It's messy and harder to follow which `return` belongs to which
context.
> > Is there a concrete advantage here for using lambda expression when
> > iterating committed regions? I'm asking because personally I find
> > ```c++
> > while ((committed_rgn = itr.next()) != nullptr) {
> > print_committed_rgn(committed_rgn);
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > simpler and more compact, hence easier to understand, than
> > ```c++
> > CommittedMemoryRegion cmr;
> >
> > VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn,
> > &cmr, [&](CommittedMemoryRegion* crgn) {
> > print_committed_rgn(crgn);
> > return true;
> > });
> > ```
>
> Hi Gerard, I'm the one who prefers passing in a lambda and introduced that
> style, sorry :-).
>
> First off, I think the lambda code should be simplified, it should be:
>
> ```c++
> VMWT::tree().visit_committed_regions_of(*reserved_rgn, [](const
> CommittedMemoryRegion& crgn) {
> print_committed_region(crgn));
> return true;
> });
> ```
>
> I don't think that's too bad, right? The `return true` is a bit unfortunate.
> It's for being able to stop early, which I agree that regular iterators do
> better by just performing a `break;`.
>
> I'll go ahead and say one nice thing too: If the body of the lambda is a bit
> more complicated, then we can look at the capture list (the `[]` above) to
> see what the lambda can alter in the outside scope. With a while-loop, you
> really have to read the whole thing.
>
> **The reason** that I write these kinds of iterators is that they're much
> simpler to implement and maintain and, _to me_, STL-style iterators aren't
> easier to read, it's about the same level of complexity.
>
> I'll admit that your style of iterators (which are _not_ STL) is about the
> same complexity, though I find it unfortunate that we have to write an entire
> class for each iterator.
>
> With the simplifications I made, is this style of iterator acceptable?
hi Johan,
Yes, this does look better. I looked at
https://www.geeksforgeeks.org/when-to-use-lambda-expressions-inste
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 9 Aug 2024 16:05:00 GMT, Gerard Ziemski wrote:
> > > Is there a concrete advantage here for using lambda expression when
> > > iterating committed regions? I'm asking because personally I find
> > > ```c++
> > > while ((committed_rgn = itr.next()) != nullptr) {
> > > print_committed_rgn(committed_rgn);
> > > }
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > simpler and more compact, hence easier to understand, than
> > > ```c++
> > > CommittedMemoryRegion cmr;
> > >
> > > VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn,
> > > &cmr, [&](CommittedMemoryRegion* crgn) {
> > > print_committed_rgn(crgn);
> > > return true;
> > > });
> > > ```
> >
> >
> > Hi Gerard, I'm the one who prefers passing in a lambda and introduced that
> > style, sorry :-).
> > First off, I think the lambda code should be simplified, it should be:
> > ```c++
> > VMWT::tree().visit_committed_regions_of(*reserved_rgn, [](const
> > CommittedMemoryRegion& crgn) {
> > print_committed_region(crgn));
> > return true;
> > });
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I don't think that's too bad, right? The `return true` is a bit
> > unfortunate. It's for being able to stop early, which I agree that regular
> > iterators do better by just performing a `break;`.
> > I'll go ahead and say one nice thing too: If the body of the lambda is a
> > bit more complicated, then we can look at the capture list (the `[]` above)
> > to see what the lambda can alter in the outside scope. With a while-loop,
> > you really have to read the whole thing.
> > **The reason** that I write these kinds of iterators is that they're much
> > simpler to implement and maintain and, _to me_, STL-style iterators aren't
> > easier to read, it's about the same level of complexity.
> > I'll admit that your style of iterators (which are _not_ STL) is about the
> > same complexity, though I find it unfortunate that we have to write an
> > entire class for each iterator.
> > With the simplifications I made, is this style of iterator acceptable?
>
> hi Johan,
>
> Yes, this does look better. I looked at
> https://www.geeksforgeeks.org/when-to-use-lambda-expressions-instead-of-functions-in-cpp
> to see when one should consider using lambda and I like one particular
> scenario - improving readability by implementing the code locally, so one can
> see exactly what is going on without having to look inside a function. I
> think this is our use case scenario here.
>
> If only we didn't need all those `return` statements, I'd have clearly
> preferred lambda here in fact.
It's tedious to have to write `return true;` if you never `return false;` in
the function. I looked into fixing this with some metaprogramming, and I found
two C++14 solutions and one C++17 one. Here's the Godbolt:
https://godbolt.org/z/xGzoGYhf8
And here's the code, reproduced for posterity:
```c++
#include
#include
// C++14 solution
// Here we take extra runtime cost for the void returning function
#define ENABLE_IF(...) \
std::enable_if_t = 0
template ::type>::value)>
void iterate(std::vector& arr, F fun) {
for (int i = 0; i < arr.size(); i++) {
if (!fun(arr[i])) {
return;
}
}
}
template::type>::value)>
void iterate(std::vector& arr, F fun) {
iterate(arr, [&fun](int& i) {
fun(i);
return true;
});
}
// C++17 solution, notice that this compiles two functions: one for when
early_return is true and when for when it's not.
/*
template
void iterate(std::vector& arr, F fun) {
using ret_type = decltype(fun(arr[0])); // This doesn't actually execute
fun(arr[0]);
constexpr bool early_return = std::is_same::value;
for (int i = 0; i < arr.size(); i++) {
if constexpr (early_return) {
if (!fun(arr[i])) {
return;
}
} else {
fun(arr[i]);
}
}
}
*/
// C++14 solution
// Here we have to make the two implementations ourselves.
/*
#define ENABLE_IF(...) \
std::enable_if_t = 0
template ::type>::value)>
void iterate(std::vector& arr, F fun) {
for (int i = 0; i < arr.size(); i++) {
if (!fun(arr[i])) {
return;
}
}
}
template::type>::value)>
void iterate(std::vector& arr, F fun) {
for (int i = 0; i < arr.size(); i++) {
fun(arr[i]);
}
}
*/
int main() {
std::vector my_array{0,1,2,3};
auto no_early = [](int& x) {
std::cout << x << " ";
};
auto early = [](int &x) {
if (x == 2) return false;
std::cout << x << " ";
return true;
};
iterate(my_array, no_early);
std::cout << std::endl;
iterate(my_array, early);
std::cout << std::endl;
}
> I honestly do not think we should be checking in 2 implementations, unless
> they are nicely hidden away. I do not like the way we manage them right now
> with 2 explicit `if` checks, each and every time we need to
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 26 Aug 2024 07:16:39 GMT, Afshin Zafari wrote:
> > Is the plan to check-in the fix with both paths? Or are we going to remove
> > the linked-list based one after the review?
>
> The plan is to have both versions available at run-time. In this plan, we
> will add JVM options to let the user to select either of them. This can be
> used for: 1️⃣ stepping back if new version fails in some use-cases, and also
> for 2️⃣ running benchmarks and comparing the results of both.
If we are going to keep the 2 versions for a while, then I would really, really
like to see the two implementations as instances of
`VirtualMemoryTrackerWithLinkedList` and `VirtualMemoryTrackerWithTree`, and
have `VirtualMemoryTracker` be the single class we have the external code deal
with, i.e. I think we can do better that:
```
static void snapshot_thread_stacks() {
if (is_using_sorted_link_list())
VirtualMemoryTracker::snapshot_thread_stacks();
if (is_using_tree())
VirtualMemoryTrackerWithTree::Instance::snapshot_thread_stacks();
}
> > I still haven't run the benchmarks to verify the speed increase promise.
> > Ideally, I would like to do this with my mechanism, but will only do it, if
> > I can manage it without getting sucked into working on the mechanism itself.
> > I will definitively want to run the provided microbenchmarks though.
>
> In `test_vmt_with_tree.cpp`, two versions are compared. One of the tests
> (`PerformanceComparison`) is comparing the time they take for the same
> operation. This test is skipped for now, because its expectation of
> time-improvement fails. Until the time we still did not merge all other
> related changes (like the `copy_flag` or `use_tag_inplace`,
> [#20330](https://github.com/openjdk/jdk/pull/20330)) to this PR we cannot
> expect the time to be improved.
Should we say then, that this is blocked by those 2 issues? Is it OK then if I
wait till those get checked in before verifying the performance benefits if the
new implementation? The performance was the main motivation here, correct?
-
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2316167719
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379647798
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Thu, 26 Sep 2024 10:58:31 GMT, Johan Sjölen wrote:
> What is the consensus on having two implementations alive at the same time?
> I'd like to see us delete the old VirtualMemoryTracker and only have the tree
> implementation left as part of this PR. What is the status of our testing? Is
> the tree version equivalent now and passes all tests?
>
I have already told that the main reason to have 2 versions is to be able to
switch back from new to old version at customer site at runtime. This is what
Thomas has requested.
During the implementation, it is much beneficial to have access to the old
version to compare the results and debug problems when they occur.
Until the whole port to VMATree is not done, we need this 2 versions
side-by-side feature.
> That's good, let's wait until that PR is integrated before this is integrated.
Sure, I do.
> Is this part of the code under-tested as we didn't receive a test failure
> regarding this?
All JTREG runtime/NMT tests and all tier1 tests have been passed on this code.
> Note that we can't do the insertion in the iteration loop, as our iterator
> will be invalidated.
I meant that the iterator is not going through the nodes in VMATree. And the
`add_committeed_region` is called on a local `ReservedMemoryRegion*`.
Anyway, I agree to rewrite this for many more reasons than above, but not sure
how. The required change was deferred for a later time when we started new VMT.
Should think about it.
> src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 262:
>
>> 260: vm_exit_out_of_memory(word_size * BytesPerWord, OOM_MMAP_ERROR,
>> "Failed to reserve memory for metaspace");
>> 261: }
>> 262: assert(rs.size() == word_size * BytesPerWord, " must be");
>
> " must be" -> "must be"
Done
> src/hotspot/share/nmt/memReporter.cpp line 442:
>
>> 440: if (all_committed) {
>> 441: bool reserved_and_committed = false;
>> 442:
>> VirtualMemoryTracker::Instance::tree()->visit_committed_regions(*reserved_rgn,
>
> Change the signature of `visit_committed_regions` to taking `(position start,
> size size)` instead of the `ReservedMemoryRegion`.
Done.
> src/hotspot/share/nmt/memReporter.cpp line 458:
>
>> 456: }
>> 457:
>> 458: auto print_committed_rgn = [&](CommittedMemoryRegion* crgn) {
>
> `crgn` can be const reference instead of pointer.
Done.
> src/hotspot/share/nmt/memTracker.hpp line 291:
>
>> 289: // Query lock
>> 290: static Mutex* _query_lock;
>> 291:
>
> Remove
Done.
> src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 93:
>
>> 91:
>> 92: const inline NativeCallStack& get(StackIndex si) {
>> 93: if (is_invalid(si) || si >= _stacks.length()) {
>
> I don't think this should be here?
Removed.
> src/hotspot/share/nmt/regionsTree.cpp line 33:
>
>> 31: log_debug(nmt)("trc base: " INTPTR_FORMAT " , trc end: "
>> INTPTR_FORMAT,
>> 32: p2i(region_in_tree.base()),
>> p2i(region_in_tree.end()));
>> 33: }
>
> Remove `with_trace` and improve the logging message. This should log
> unconditionally, probably under a `(nmt,vmtracker)` tagset.
Done.
> src/hotspot/share/nmt/regionsTree.cpp line 50:
>
>> 48: ReservedMemoryRegion rgn = find_reserved_region(addr);
>> 49: return reserve_mapping((VMATree::position)addr, size,
>> make_region_data(NativeCallStack::empty_stack(), rgn.mem_tag()));
>> 50: }
>
> Can now be fixed.
Done.
> src/hotspot/share/nmt/regionsTree.cpp line 52:
>
>> 50: rgn = find_reserved_region(addr, true);
>> 51: ShouldNotReachHere();
>> 52: }
>
> TODO: Remove this when 8335091 is merged
Added a TODO comment to remind it.
> src/hotspot/share/nmt/regionsTree.cpp line 54:
>
>> 52: }
>> 53: return commit_mapping((VMATree::position)addr, size,
>> make_region_data(stack, rgn.flag()));
>> 54:
>
> Style: Extra space
Fixed.
> src/hotspot/share/nmt/regionsTree.cpp line 64:
>
>> 62: rgn = find_reserved_region(addr, true);
>> 63: ShouldNotReachHere();
>> 64: }
>
> TODO: Remove this when
> https://github.com/openjdk/jdk/commit/8335091a5dd9a0654d9a616addcfdcc1863747ca
> is merged
Added a TODO comment to remind this.
> src/hotspot/share/nmt/regionsTree.hpp line 31:
>
>> 29: #include "nmt/vmatree.hpp"
>> 30:
>> 31: class RegionsTree : public VMATree {
>
> Is it necessary for this to be a superclass? Seems like this class contains
> utilities for working with a `VMATree`. Could it instead be `AllStatic` and
> take a tree as its first argument? I'd like to avoid inheritance unless
> necessary (typically for the usage of virtual functions).
It is inherited because it is not a new type. I think that
`_tree->visit_reserved_region(par1, par2)` is more readable than
`VMATree::visit_reserved_region(_tree, par1, par2)`
I could not find any *virtual functions* in these classes, what do you mean by
that?
> src/hotspot/share/nmt/regionsTree.hpp line 33:
>
>> 31: class RegionsTree : public VMATree {
>> 32:
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 09:52:07 GMT, Afshin Zafari wrote:
> > What is the consensus on having two implementations alive at the same time?
> > I'd like to see us delete the old VirtualMemoryTracker and only have the
> > tree implementation left as part of this PR. What is the status of our
> > testing? Is the tree version equivalent now and passes all tests?
>
> I have already told that the main reason to have 2 versions is to be able to
> switch back from new to old version at customer site at runtime. This is what
> Thomas has requested.
The issue with having two implementations is that it requires adding a new
`java` option, and deprecating and removing those take 3 releases. That's 18
months of us being required to have the old version in the codebase, and being
required to maintain it. I don't think that's a negligible promise to make.
Rather than having 2 implementations, I'd like to see us aiming for integration
for JDK-25 after forking 24, so integration in December. That would give us 6
months of ensuring stability of the new implementation before it reaches any
customers. During those 6 months it will go through all of our testing multiple
times over, and be used by OpenJDK developers. What do you think of this idea?
> During the implementation, it is much beneficial to have access to the old
> version to compare the results and debug problems when they occur. Until the
> whole port to VMATree is not done, we need this 2 versions side-by-side
> feature.
Sure, I can understand that it's nice to have both versions present during
development. Right now it seems like we have a broken build, do you have any
plans on having a functioning and fully featured build soon?
>> src/hotspot/share/nmt/regionsTree.hpp line 31:
>>
>>> 29: #include "nmt/vmatree.hpp"
>>> 30:
>>> 31: class RegionsTree : public VMATree {
>>
>> Is it necessary for this to be a superclass? Seems like this class contains
>> utilities for working with a `VMATree`. Could it instead be `AllStatic` and
>> take a tree as its first argument? I'd like to avoid inheritance unless
>> necessary (typically for the usage of virtual functions).
>
> It is inherited because it is not a new type. I think that
> `_tree->visit_reserved_region(par1, par2)` is more readable than
> `VMATree::visit_reserved_region(_tree, par1, par2)`
> I could not find any *virtual functions* in these classes, what do you mean
> by that?
I'm saying that inheritance is mostly useful when we have virtual functions
which we can specialize, and that `VMATree` has none.
>> src/hotspot/share/nmt/regionsTree.hpp line 33:
>>
>>> 31: class RegionsTree : public VMATree {
>>> 32: private:
>>> 33: NativeCallStackStorage* _ncs_storage;
>>
>> No need for this to be a pointer.
>
> How to use the ctor with a `bool` arg, then?
The bool argument is just passed along.
```c++
RegionsTree(bool with_storage) : VMATree(), _ncs_storage(with_storage) {
}
>> src/hotspot/share/nmt/regionsTree.hpp line 108:
>>
>>> 106: CommittedMemoryRegion cmr((address)base, comm_size, st);
>>> 107: comm_size = 0;
>>> 108: prev.clear_node();
>>
>> This is unneeded.
>
> Which part? Why?
> `clear_node()` is the same as `prev = nullptr` if pointers were used.
> `is_valid()` is the same as `prev == nullptr` if pointers were used.
Because you immediately reassign `prev` to `curr` in L114 it's not necessary to
first call `clear_node`.
>> src/hotspot/share/nmt/virtualMemoryTrackerWithTree.cpp line 101:
>>
>>> 99: }
>>> 100:
>>> 101: void
>>> VirtualMemoryTrackerWithTree::apply_summary_diff(VMATree::SummaryDiff diff)
>>> {
>>
>> Applying a summary diff in the MemoryFileTracker is this:
>>
>> ```c++
>> for (int i = 0; i < mt_number_of_types; i++) {
>> VirtualMemory* summary =
>> file->_summary.by_type(NMTUtil::index_to_flag(i));
>> summary->reserve_memory(diff.flag[i].commit);
>> summary->commit_memory(diff.flag[i].commit);
>> }
>>
>>
>> This is much simpler and doesn't require looking at signs and so on.
>
> In `MemoryFileTracker`, the changes in commit/reserve are applied to a local
> `VirtualMemorySnapshot`. Here we have to apply them to the global
> `VirtualMemorySummary`.
Yeah, that doesn't seem like a problem.
```c++
for (int i = 0; i < mt_number_of_types; i++) {
r = diff.flag[i].reserve;
c = diff.flag[i].commit;
flag = NMTUtil::index_to_flag(i);
VirtualMemory* mem = VirtualMemorySummary::as_snapshot()->by_type(flag);
reserved = mem->reserved();
committed = mem->committed();
mem->reserve_memory(r);
mem->commit_memory(c);
if ((size_t)-r > reserved) {
print_err("release");
}
if ((size_t)-c > reserved || (size_t)c > committed) {
print_err("uncommit");
}
}
>> src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 34:
>>
>>> 32: #include "utilities/ostream.hpp"
>>> 33:
>>> 34: class VirtualMemoryTrackerWithTree : AllStatic {
>>
>> Can this c
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 26 Aug 2024 11:03:25 GMT, Johan Sjölen wrote: > It's tedious to have to write return true; if you never return false; in the > function. I looked into fixing this with some metaprogramming, and I found > two C++14 solutions and one C++17 one. Here's the Godbolt I'd like the lambdas without `return true` if possible, pretty please. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2316187388
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 16:23:29 GMT, Gerard Ziemski wrote: > Should we say then, that this is blocked by those 2 issues? Is it OK then if > I wait till those get checked in before verifying the performance benefits if > the new implementation? The performance was the main motivation here, correct? It is not blocked, but until those 2 other PRs we cannot expect the full speed of the new VMT. There are some functions that are not efficient without those 2 PRs. You can test VMT with your microbenchmarks but if it fails in competition give it another chance after those PRs. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379801032
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 14 Oct 2024 13:22:54 GMT, Afshin Zafari wrote:
> > Yes, but this code is incorrect. So we should have a test detecting this,
> > but we do not, and so this is under-tested.
>
> This code finds committed regions within a Stack region if they are not
> accounted by NMT so far, IIUC. By running this incorrect code, we may get
> some extra committed size in NMT reports. This is hard in NMT tests to catch.
> How can we expect a NMT test to measure X bytes of committed stack and not Y
> bytes? We need to know exactly the X and/or Y to test this code.
I think it's actually the opposite: None of the committed regions will survive
after this function.
This is one of those times when having the ability to create new instances of
NMT makes things very nice for testing.
You simply do something like this (pseudo-code):
```c++
void test_it() {
VMT vmt;
size_t sz = 4096 * 16;
void* mem = os::reserve_memory(sz, mtThreadStack);
os::commit_memory(mem + 4096*2, 4096);
vmt.reserve(mem, sz, mtThreadStack);
vmt.walk_thread_stacks();
EXPECT_EQ(4096, sum_committed_of(vmt, mtThreadStack));
}
-
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2412008724
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Thu, 7 Nov 2024 12:48:12 GMT, Afshin Zafari wrote: >> - `VMATree` is used instead of `SortedLinkList` in new class >> `VirtualMemoryTrackerWithTree`. >> - A wrapper/helper `RegionTree` is made around VMATree to make some calls >> easier. >> - Both old and new versions exist in the code and can be selected via >> `MemTracker::set_version()` >> - `find_reserved_region()` is used in 4 cases, it will be removed in >> further PRs. >> - All tier1 tests pass except one ~that expects a 50% increase in committed >> memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167. >> - Adding a runtime flag for selecting the old or new version can be added >> later. >> - Some performance tests are added for new version, VMATree and Treap, to >> show the idea and should be improved later. Based on the results of >> comparing speed of VMATree and VMT, VMATree shows ~40x faster response time. > > Dear @tstuefe, this PR has been reviewed couple of rounds. Would you please, > give your feedback? > Thanks. Hi @afshin-zafari, I'll take a look next week. Could you please remove draft state, so that the discussions are public on the ML? Could you also please, to make reviewing easier and for documentation, describe the desired behavior? You should also add this description (or reuse, could be the same text) to the header/start of the implementation. Detailed questions are: 1. where does the data structure live? E.g. "Its a global data structure". Important here is the distinction to the ZGC file mapping tracker. 2. How is the data structure guarded? (locking question) 3. how are memory reservations realized (what tree operations?) E.g. "creates a VMATree memory section with state=reserved and the given tag/stack combination". 3.1 Do we allow reservations to overlap with existing sections? If no, why? If yes, under which conditions, and what happens to the old sections? 4. how is memory commit realized? 4.2. What happens if I specify tag and stack? Any restrictions then? 4.1. What happens if I omit tag and stack on commit? What are the restrictions? 5. same for uncommit 6. How is release done? 7. How do we find memory sections? 8. Do we allow modifying the tag? (See @jdksjolen set_tag PR) Here is what I think how this should work: NMT should normally not put any restrictions on the use of mmap APIs. It should just faithfully track what we do. However, we may want a "paranoid" mode where we bail out/warn if something looks fishy. 3. reservations should create a new tree section with the given flag/stack 3.1 there should be no restrictions. If there are old sections in that range, they are replaced. However, with paranoid mode enabled, I would warn if we reserve over committed sections because that would replace the mapping in reality. Remapping just reserved but uncommitted sections should be fine, I think. I think we do this sometimes. 4. commit should work differently when giving tag+stack, or when tag and stack are omitted. 4.1 if tag+stack are given, commit just creates a single new section across the range with the given tag + stack. That should always work, regardless of whats there before (paranoid off). With paranoid=on, it should warn if we commit over already Committed sections, since that destroys those sections. 4.2) committing when tag+stack are omitted. As in "please commit what I reserved before, and just with the same tag and stack". In that case, commit just changes the tag for the range. That may or may not cause sections to coalesce (if changing the tag causes neighboring sections to have the same set of properties) or be created (if you commit middle of an existing section, and the set of properties now differs). Restrictions: there should be a clear unconditional restriction that this only works if there are no Released sections in that range, since a Released section (address holes) has no tag+stack to copy from. 5. Uncommit should never require tag+stack. It should change the state of the range to "Reserved" and leave tag+stack alone. Again, cannot work across address space holes. It can cause the splintering of existing sections, if we uncommit the middle of a committed section. 6. Releasing should just create a new section with State=Released, tag=mtNone and no stack. WDYT. I think it is a must to have all of this clearly specified before starting to implement, because the code is complex and one needs to know this logic beforehand. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2463896881
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 14 Oct 2024 18:56:47 GMT, Johan Sjölen wrote: > I think it's actually the opposite: None of the committed regions will > survive after this function. You maybe missed my point when said " ... some extra committed size in NMT reports". I emphasize on the " the NMT reports", since the committed sizes in this function are added to the global VirtualMemorySummary which is used in reports. And yes, no committed regions that added here won't survive after this function. > This is one of those times when having the ability to create new instances of > NMT makes things very nice for testing. I was talking about the existing tests and why they couldn't catch this bug.(why this is under-tested) > You simply do something like this (pseudo-code): Thanks for the pseudo-code, I will implement a test for this specific case. You will see it in one of the future commits here. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2413222662 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2413225089 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2413227952
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Wed, 14 Aug 2024 19:11:58 GMT, Gerard Ziemski wrote:
>> - `VMATree` is used instead of `SortedLinkList` in new class
>> `VirtualMemoryTrackerWithTree`.
>> - A wrapper/helper `RegionTree` is made around VMATree to make some calls
>> easier.
>> - Both old and new versions exist in the code and can be selected via
>> `MemTracker::set_version()`
>> - `find_reserved_region()` is used in 4 cases, it will be removed in
>> further PRs.
>> - All tier1 tests pass except one ~that expects a 50% increase in committed
>> memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167.
>> - Adding a runtime flag for selecting the old or new version can be added
>> later.
>> - Some performance tests are added for new version, VMATree and Treap, to
>> show the idea and should be improved later. Based on the results of
>> comparing speed of VMATree and VMT, VMATree shows ~40x faster response time.
>
> src/hotspot/share/nmt/memReporter.cpp line 467:
>
>> 465:
>> 466: if (reserved_and_committed)
>> 467: return;
>
> This looks better, but now that I got more comfortable with anonymous local
> functions using lambda, I think we can do better. We can write a single
> `print_rgn` lambda function that we can use in both "reserved" and
> "committed" cases:
>
>
> outputStream* out = output();
> const char* scale = current_scale();
> auto print_rgn = [&](const VirtualMemoryRegion* rgn, const NativeCallStack*
> stack,
>MEMFLAGS mem_type = MEMFLAGS::mtNone,
>const char* region_type = "committed",
>bool committed = true) {
> // Don't report if size is too small
> if (amount_in_current_scale(rgn->size()) == 0) return;
> out->cr();
> INDENT_BY(committed?8:0,
> print_virtual_memory_region(region_type, rgn->base(), rgn->size());
> if (stack->is_empty()) {
> out->cr();
> } else {
> if (!committed) {
> out->print(" for %s", NMTUtil::flag_to_name(mem_type));
> }
> out->print_cr(" from ");
> INDENT_BY(4, _stackprinter.print_stack(stack);)
> }
> )
> };
The whole method here then becomes:
void MemDetailReporter::report_virtual_memory_region(const
ReservedMemoryRegion* reserved_rgn) {
assert(reserved_rgn != nullptr, "null pointer");
// We don't bother about reporting peaks here.
// That is because peaks - in the context of virtual memory, peak of
committed areas - make little sense
// when we report *by region*, which are identified by their location in
memory. There is a philosophical
// question about identity here: e.g. a committed region that has been split
into three regions by
// uncommitting a middle section of it, should that still count as "having
peaked" before the split? If
// yes, which of the three new regions would be the spiritual successor?
Rather than introducing more
// complexity, we avoid printing peaks altogether. Note that peaks should
still be printed when reporting
// usage *by callsite*.
if (reserved_rgn->flag() == mtTest) {
log_debug(nmt)("report vmem, rgn base: " INTPTR_FORMAT " size: "
SIZE_FORMAT "cur-scale: " SIZE_FORMAT,
p2i(reserved_rgn->base()),
reserved_rgn->size(),amount_in_current_scale(reserved_rgn->size()));
}
outputStream* out = output();
const char* scale = current_scale();
auto print_rgn = [&](const VirtualMemoryRegion* rgn, const NativeCallStack*
stack,
MEMFLAGS mem_type = MEMFLAGS::mtNone,
const char* region_type = "committed",
bool committed = true) {
// Don't report if size is too small
if (amount_in_current_scale(rgn->size()) == 0) return;
out->cr();
INDENT_BY(committed?8:0,
print_virtual_memory_region(region_type, rgn->base(), rgn->size());
if (stack->is_empty()) {
out->cr();
} else {
if (!committed) {
out->print(" for %s", NMTUtil::flag_to_name(mem_type));
}
out->print_cr(" from ");
INDENT_BY(4, _stackprinter.print_stack(stack);)
}
)
};
bool all_committed = reserved_rgn->size() == reserved_rgn->committed_size();
const char* region_type = (all_committed ? "reserved and committed" :
"reserved");
print_rgn(reserved_rgn, reserved_rgn->call_stack(), reserved_rgn->flag(),
region_type, false);
if (all_committed) {
if (MemTracker::is_using_sorted_link_list()) {
CommittedRegionIterator itr = reserved_rgn->iterate_committed_regions();
const CommittedMemoryRegion* committed_rgn = itr.next();
if (committed_rgn->size() == reserved_rgn->size() &&
committed_rgn->call_stack()->equals(*reserved_rgn->call_stack())) {
// One region spanning the entire reserved region, with the same stack
trace.
// Don't print this regions because the "reserved and committed" line
above
// already indicates that the region is
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Wed, 14 Aug 2024 19:13:49 GMT, Gerard Ziemski wrote:
>> src/hotspot/share/nmt/memReporter.cpp line 467:
>>
>>> 465:
>>> 466: if (reserved_and_committed)
>>> 467: return;
>>
>> This looks better, but now that I got more comfortable with anonymous local
>> functions using lambda, I think we can do better. We can write a single
>> `print_rgn` lambda function that we can use in both "reserved" and
>> "committed" cases:
>>
>>
>> outputStream* out = output();
>> const char* scale = current_scale();
>> auto print_rgn = [&](const VirtualMemoryRegion* rgn, const
>> NativeCallStack* stack,
>>MEMFLAGS mem_type = MEMFLAGS::mtNone,
>>const char* region_type = "committed",
>>bool committed = true) {
>> // Don't report if size is too small
>> if (amount_in_current_scale(rgn->size()) == 0) return;
>> out->cr();
>> INDENT_BY(committed?8:0,
>> print_virtual_memory_region(region_type, rgn->base(), rgn->size());
>> if (stack->is_empty()) {
>> out->cr();
>> } else {
>> if (!committed) {
>> out->print(" for %s", NMTUtil::flag_to_name(mem_type));
>> }
>> out->print_cr(" from ");
>> INDENT_BY(4, _stackprinter.print_stack(stack);)
>> }
>> )
>> };
>
> The whole method here then becomes:
>
>
> void MemDetailReporter::report_virtual_memory_region(const
> ReservedMemoryRegion* reserved_rgn) {
> assert(reserved_rgn != nullptr, "null pointer");
>
> // We don't bother about reporting peaks here.
> // That is because peaks - in the context of virtual memory, peak of
> committed areas - make little sense
> // when we report *by region*, which are identified by their location in
> memory. There is a philosophical
> // question about identity here: e.g. a committed region that has been
> split into three regions by
> // uncommitting a middle section of it, should that still count as "having
> peaked" before the split? If
> // yes, which of the three new regions would be the spiritual successor?
> Rather than introducing more
> // complexity, we avoid printing peaks altogether. Note that peaks should
> still be printed when reporting
> // usage *by callsite*.
>
> if (reserved_rgn->flag() == mtTest) {
> log_debug(nmt)("report vmem, rgn base: " INTPTR_FORMAT " size: "
> SIZE_FORMAT "cur-scale: " SIZE_FORMAT,
> p2i(reserved_rgn->base()),
> reserved_rgn->size(),amount_in_current_scale(reserved_rgn->size()));
> }
>
> outputStream* out = output();
> const char* scale = current_scale();
> auto print_rgn = [&](const VirtualMemoryRegion* rgn, const NativeCallStack*
> stack,
>MEMFLAGS mem_type = MEMFLAGS::mtNone,
>const char* region_type = "committed",
>bool committed = true) {
> // Don't report if size is too small
> if (amount_in_current_scale(rgn->size()) == 0) return;
> out->cr();
> INDENT_BY(committed?8:0,
> print_virtual_memory_region(region_type, rgn->base(), rgn->size());
> if (stack->is_empty()) {
> out->cr();
> } else {
> if (!committed) {
> out->print(" for %s", NMTUtil::flag_to_name(mem_type));
> }
> out->print_cr(" from ");
> INDENT_BY(4, _stackprinter.print_stack(stack);)
> }
> )
> };
>
> bool all_committed = reserved_rgn->size() == reserved_rgn->committed_size();
> const char* region_type = (all_committed ? "reserved and committed" :
> "reserved");
> print_rgn(reserved_rgn, reserved_rgn->call_stack(), reserved_rgn->flag(),
> region_type, false);
>
> if (all_committed) {
> if (MemTracker::is_using_sorted_link_list()) {
> CommittedRegionIterator itr = reserved_rgn->iterate_committed_regions();
> const CommittedMemoryRegion* committed_rgn = itr.next();
> if (committed_rgn->size() == reserved_rgn->...
Notice that by changing:
`INDENT_BY(4, stack->print_on(out);)`
to
`INDENT_BY(4, _stackprinter.print_stack(stack);)`
we went from:
[0x0006 - 0x00062000] committed 536870912 from
[0x00010ce40dfe]ReservedSpace::reserve(unsigned long, unsigned
long, unsigned long, char*,
bool)+0x2da[0x00010ce41214]ReservedHeapSpace::try_reserve_heap(unsigned
long, unsigned long, unsigned long,
char*)+0x60[0x00010ce413ac]ReservedHeapSpace::try_reserve_range(char*,
char*, unsigned long, char*, char*, unsigned long, unsigned long, unsigned
long)+0xae[0x00010ce41571]ReservedHeapSpace::initialize_compressed_heap(unsigned
long, unsigned long, unsigned long)+0x1b5
to
[0x0006 - 0x00062000] committed 536870912 from
[0x00010be40d4e]ReservedSpace::reserve(unsigned long, unsigned
long, unsigned long, char*, bool)+0x2da
[0x00010be41164]ReservedHeapSpace::try_reserve_heap(unsigned
long, unsigned
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Sat, 12 Oct 2024 14:24:15 GMT, Johan Sjölen wrote:
>> No, returned back.
>> This assert is triggered:
>>
>> # Internal Error
>> (/home/afshin/scratch/833XX_nmt_VMT_with_tree/src/hotspot/share/utilities/growableArray.hpp:142),
>> pid=1972883, tid=1972931
>> # assert(0 <= i && i < _len) failed: illegal index 0 for length 0
>
> Why is this assert triggered? That sounds like a bug.
The assertion that happens during building jdk-image:
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error
(/home/afshin/scratch/833XX_nmt_VMT_with_tree/src/hotspot/share/utilities/growableArray.hpp:142),
pid=1234272, tid=1234278
# assert(0 <= i && i < _len) failed: illegal index 0 for length 0
#
# JRE version: (24.0) (fastdebug build )
# Java VM: OpenJDK 64-Bit Server VM (fastdebug
24-internal-2024-09-30-0937351.afshin..., mixed mode, emulated-client, tiered,
compressed oops, compressed class ptrs, serial gc, linux-amd64)
# Core dump will be written. Default location: Core dumps may be processed with
"/usr/share/apport/apport -p%p -s%s -c%c -d%d -P%P -u%u -g%g -- %E" (or dumping
to /home/afshin/scratch/833XX_nmt_VMT_with_tree/make/core.1234272)
#
#
--- S U M M A R Y
Command Line: -Denv.class.path=
-Dapplication.home=/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/jdk
-Xms8m -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
-Djdk.module.main=jdk.jlink jdk.jlink/jdk.tools.jmod.Main create
--module-version 24-internal --target-platform linux-amd64 --module-path
/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/images/jmods
--libs
/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/support/modules_libs/jdk.sctp
--class-path
/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/jdk/modules/jdk.sctp
--legal-notices
/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/support/modules_legal/common
--exclude **{_the.*,_*.marker*,*.diz,*.debuginfo,*.dSYM/**,*.dSYM} --compress
zip-1 --date 2024-09-30T09:37:57Z
/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/support/images/jmods/jdk.sctp.jmod
Host: afshin-Precision-7920-Tower, Intel(R) Xeon(R) Silver 4114 CPU @ 2.20GHz,
40 cores, 62G, Ubuntu 20.04.6 LTS
Time: Mon Oct 14 10:52:34 2024 CEST elapsed time: 0.002402 seconds (0d 0h 0m 0s)
--- T H R E A D ---
Current thread is native thread
Stack: [0x7ffa5243a000,0x7ffa5253b000], sp=0x7ffa52539390, free
space=1020k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x18df026] VMError::report_and_die(int, char const*, char
const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*,
int, unsigned long)+0x486 (growableArray.hpp:142)
V [libjvm.so+0x18df870] VMError::report_and_die(Thread*, void*, char const*,
int, char const*, char const*, ...)+0x0 (vmError.cpp:1611)
V [libjvm.so+0xac3a81] report_vm_error(char const*, int, char const*, char
const*, ...)+0xf1 (debug.cpp:193)
V [libjvm.so+0x15f7851] void Treap::visit_in_order(RegionsTree::find_reserved_region(unsigned
char*)::{lambda(ReservedMemoryRegion&)#1})::{lambda(Treap::TreapNode*)#1}>(RegionsTree::visit_reserved_regions(RegionsTree::find_reserved_region(unsigned
char*)::{lambda(ReservedMemoryRegion&)#1})::{lambda(Treap::TreapNode*)#1}) const+0x4c1 (growableArray.hpp:142)
V [libjvm.so+0x15f7b59] RegionsTree::commit_region(unsigned char*, unsigned
long, NativeCallStack const&)+0x149 (vmatree.hpp:227)
V [libjvm.so+0x18cc8cc] VirtualMemoryTracker::add_committed_region(unsigned
char*, unsigned long, NativeCallStack const&)+0x2c
(virtualMemoryTracker.cpp:150)
V [libjvm.so+0x14dc5b7] os::commit_memory(char*, unsigned long, bool)+0x97
(memTracker.hpp:175)
V [libjvm.so+0xac611f] initialize_assert_poison()+0x7f (debug.cpp:715)
V [libjvm.so+0x181aecd] Threads::create_vm(JavaVMInitArgs*, bool*)+0x53d
(threads.cpp:490)
V [libjvm.so+0xfb4494] JNI_CreateJavaVM+0x54 (jni.cpp:3596)
C [libjli.so+0x432f] JavaMain+0x8f (java.c:1490)
C [libjli.so+0x79b9] ThreadJavaMain+0x9 (java_md.c:633)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1799014090
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 09:57:35 GMT, Afshin Zafari wrote: >> Removed. > > No, returned back. > This assert is triggered: > > # Internal Error > (/home/afshin/scratch/833XX_nmt_VMT_with_tree/src/hotspot/share/utilities/growableArray.hpp:142), > pid=1972883, tid=1972931 > # assert(0 <= i && i < _len) failed: illegal index 0 for length 0 Why is this assert triggered? That sounds like a bug. - PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797710083
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 09:46:27 GMT, Afshin Zafari wrote:
>> src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 93:
>>
>>> 91:
>>> 92: const inline NativeCallStack& get(StackIndex si) {
>>> 93: if (is_invalid(si) || si >= _stacks.length()) {
>>
>> I don't think this should be here?
>
> Removed.
No, returned back.
This assert is triggered:
# Internal Error
(/home/afshin/scratch/833XX_nmt_VMT_with_tree/src/hotspot/share/utilities/growableArray.hpp:142),
pid=1972883, tid=1972931
# assert(0 <= i && i < _len) failed: illegal index 0 for length 0
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778358195
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Thu, 1 Aug 2024 15:44:32 GMT, Afshin Zafari wrote:
> - `VMATree` is used instead of `SortedLinkList` in new class
> `VirtualMemoryTrackerWithTree`.
> - A wrapper/helper `RegionTree` is made around VMATree to make some calls
> easier.
> - Both old and new versions exist in the code and can be selected via
> `MemTracker::set_version()`
> - `find_reserved_region()` is used in 4 cases, it will be removed in further
> PRs.
> - All tier1 tests pass except one ~that expects a 50% increase in committed
> memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167.
> - Adding a runtime flag for selecting the old or new version can be added
> later.
> - Some performance tests are added for new version, VMATree and Treap, to
> show the idea and should be improved later. Based on the results of comparing
> speed of VMATree and VMT, VMATree shows ~40x faster response time.
Hi,
Great work! I'm slowly working through the code, let's take our time with this.
My PR got too stressful for everyone involved :-/.
I've reviewed some of the code. Could you go a bit more in-depth on what the
results of this work was? Did it improve performance, for example?
>All tier1 tests pass except one that expects a 50% increase in committed
>memory *but it does not happen*.
~~I don't understand what this means, could you expand on this?~~ Edit: Re-read
it again now and I get it, which test is this?
Thanks!
Changes requested by jsjolen (Reviewer).
Changes requested by jsjolen (Reviewer).
This code generally over-uses pointers. Please have a go through this code and
change to references when applicable.
Another round of review comments :).
What is the consensus on having two implementations alive at the same time? I'd
like to see us delete the old VirtualMemoryTracker and only have the tree
implementation left as part of this PR. What is the status of our testing? Is
the tree version equivalent now and passes all tests?
I'd also like to see the tty printing code to either be deleted or if it's
useful for mainline be adjusted so that it is using UL instead.
Some more reviews here along with some questions. I see that you've merged in
my PR 8340103. That's good, let's wait until that PR is integrated before this
is integrated.
> Is there a concrete advantage here for using lambda expression when iterating
> committed regions? I'm asking because personally I find
>
> ```c++
> while ((committed_rgn = itr.next()) != nullptr) {
> print_committed_rgn(committed_rgn);
> }
> ```
>
> simpler and more compact, hence easier to understand, than
>
> ```c++
> CommittedMemoryRegion cmr;
>
> VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn,
> &cmr, [&](CommittedMemoryRegion* crgn) {
> print_committed_rgn(crgn);
> return true;
> });
> ```
Hi Gerard, I'm the one who prefers passing in a lambda and introduced that
style, sorry :-).
First off, I think the lambda code should be simplified, it should be:
```c++
VMWT::tree().visit_committed_regions_of(*reserved_rgn, [](const
CommittedMemoryRegion& crgn) {
print_committed_region(crgn));
return true;
});
I don't think that's too bad, right? The `return true` is a bit unfortunate.
It's for being able to stop early, which I agree that regular iterators do
better by just performing a `break;`.
I'll go ahead and say one nice thing too: If the body of the lambda is a bit
more complicated, then we can look at the capture list (the `[]` above) to see
what the lambda can alter in the outside scope. With a while-loop, you really
have to read the whole thing.
**The reason** that I write these kinds of iterators is that they're much
simpler to implement and maintain and, *to me*, STL-style iterators aren't
easier to read, it's about the same level of complexity.
I'll admit that your style of iterators (which are *not* STL) is about the same
complexity, though I find it unfortunate that we have to write an entire class
for each iterator.
With the simplifications I made, is this style of iterator acceptable?
It seems like there's a bug with the virtual memory walker. Essentially, the
thread stack walker extends the reserved memory region that's passed in, but we
don't save the results of that addition. This is missed because the
`SnapshotThreadStackWalker` casts away the `const` of the `const
ReservedMemoryRegion*`.
```c++
// All code is in vmtCommon.cpp
void VirtualMemoryTracker::Instance::snapshot_thread_stacks() {
SnapshotThreadStackWalker walker;
walk_virtual_memory(&walker);
}
// ...
while (itr.next_committed(committed_start, committed_size)) {
assert(committed_start != nullptr, "Should not be null");
assert(committed_size > 0, "Should not be 0");
// unaligned stack_size case: correct the region to fit the actual
stack_size
if (stack_bottom + stack_size < committed_start + committed_size) {
committed_size = stack_b
RFR: 8337217: Port VirtualMemoryTracker to use VMATree
- `VMATree` is used instead of `SortedLinkList` in new class `VirtualMemoryTrackerWithTree`. - A wrapper/helper `RegionTree` is made around VMATree to make some calls easier. - Both old and new versions exist in the code and can be selected via `MemTracker::set_version()` - `find_reserved_region()` is used in 4 cases, it will be removed in further PRs. - All tier1 tests pass except one ~that expects a 50% increase in committed memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167. - Adding a runtime flag for selecting the old or new version can be added later. - Some performance tests are added for new version, VMATree and Treap, to show the idea and should be improved later. Based on the results of comparing speed of VMATree and VMT, VMATree shows ~40x faster response time. - Commit messages: - added some descriptions of how VMT works. - Merge remote-tracking branch 'origin/master' into _8337217_nmt_VMT_with_tree - fixes after merge with use_tag_inplace - fixed merged glitches. - Merge remote-tracking branch 'origin/master' into _8337217_nmt_VMT_with_tree - Merge remote-tracking branch 'origin/master' into _8337217_nmt_VMT_with_tree - snaphot thread stack updated and tested. - removed unnecessary line from visit committed region function. - fixed hpp gaurd of vmtCommon. - Johan's feedback. - ... and 33 more: https://git.openjdk.org/jdk/compare/ac82a8f8...1e4fbf21 Changes: https://git.openjdk.org/jdk/pull/20425/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20425&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337217 Stats: 3294 lines in 55 files changed: 1520 ins; 1525 del; 249 mod Patch: https://git.openjdk.org/jdk/pull/20425.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20425/head:pull/20425 PR: https://git.openjdk.org/jdk/pull/20425
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Fri, 8 Nov 2024 10:26:36 GMT, Alan Bateman wrote: >> jtreg patches the `java.base` module and those langtools tests *link* from >> the run-time image (in such a config). Having a patched JDK module is not >> supported (and I don't think we ever should). Running in a separate JVM >> doesn't have this problem. See >> https://github.com/openjdk/jdk/pull/14787/commits/d8e1e834508589725adb8d70acb862a1270678ca. >> Does that make sense? > > That's okay, I wasn't initially sure why they were changed. I'm looking at > JRTArchiveFile.toEntry and wondering there should be a follow-up issue (not > this PR) to fail early if running on a patched run-time even though it would > be an odd configuration to attempt to do that. There already is. See: https://github.com/openjdk/jdk/pull/14787/files#diff-b6b47eacb6060eb0a583a253f322f5d274063e082a12a72e8628a6e1ba6cdd3eR466-R471 It's also tested with [PatchedJDKModuleJlinkTest.java](https://github.com/openjdk/jdk/pull/14787/files#diff-11b8a26307346fd7ca016a349243cabd3b982964aaf4335298e28e956b3968eb). Do we need more? - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834114823
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v47]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Remove unused imports from VMProps - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/f55bb853..0bf4843d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=46 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=45-46 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8211033: Clean up the processing -classpath argument not to set LM_CLASS
On Fri, 8 Nov 2024 09:28:07 GMT, Alan Bateman wrote:
> On the surface this looks okay but just to double check: this has no impact
> on `java -cp -jar app.jar`. The class path in that case is
> `app.jar`, the class path specified to -cp is ignored.
That's right - the launcher explicitly overrides the classpath in that case
here
https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L313:
/* Override class path if -jar flag was specified */
if (mode == LM_JAR) {
SetClassPath(what); /* Override class path */
}
-
PR Comment: https://git.openjdk.org/jdk/pull/21971#issuecomment-2464377538
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Thu, 1 Aug 2024 15:44:32 GMT, Afshin Zafari wrote: > - `VMATree` is used instead of `SortedLinkList` in new class > `VirtualMemoryTrackerWithTree`. > - A wrapper/helper `RegionTree` is made around VMATree to make some calls > easier. > - Both old and new versions exist in the code and can be selected via > `MemTracker::set_version()` > - `find_reserved_region()` is used in 4 cases, it will be removed in further > PRs. > - All tier1 tests pass except one ~that expects a 50% increase in committed > memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167. > - Adding a runtime flag for selecting the old or new version can be added > later. > - Some performance tests are added for new version, VMATree and Treap, to > show the idea and should be improved later. Based on the results of comparing > speed of VMATree and VMT, VMATree shows ~40x faster response time. Thanks for your comments. They are replied or applied. More fixes and questions. Review comments applied. The comments are applied. Performance test runs insert/remove operations for SortedLinkList (SLL in the code) and Treap and expects that Treap be faster. Both tests pass with a factor of 70+ faster for 1 elements. Dear reviewers in recent commit the following can be considered: ### Changes made to the code - Instance/Static implemented. - Using of Reference instead of pointers, as much as possible. - Removed extra un-needed args from functions - `NodeHelper` is implemented using composition approach. - Non-used methods are removed from `NodeHelper` ### Still open conversations from previous reviews - Two alternatives for implementing `apply_summary_diff` - Is readability improved after latest changes? - Are `lambda` usages acceptable now? Any improvement suggestion? - Which pointers still remained to be replaced by references? GHA failures are for some tests that have already a related JBS issue, not for this PR. > ```c++ > region->add_committed_region(committed_start, committed_size, ncs); // <-- > LOST! > ``` The `region` is not a VMATree::node, it is a `ReservedMemoryRegion*`. Dear @tstuefe, this PR has been reviewed couple of rounds. Would you please, give your feedback? Thanks. - PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2217791061 PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2219547262 PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2333278135 PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2365877947 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2273208292 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2283953888 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2380017488 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2410734124 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2462154991
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 09:49:37 GMT, Johan Sjölen wrote: > The fork for JDK-24 is on December 5th. That means that we have at most 8 > weeks in the testing system to find and fix any bugs that we might have > missed after integration. To me, that feels a bit short. Either we wait after > the fork to integrate, or we integrate with two implementations and a `java` > option for choosing implementation. I'm open to other opinions. Yes, time is tight. The chance of catching up the December 5th is not high. Anyway, we have already decided to add a new JVM option to enable/disable this new VMATree ( default = disabled). - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2378951599
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 10:22:57 GMT, Afshin Zafari wrote: > > The fork for JDK-24 is on December 5th. That means that we have at most 8 > > weeks in the testing system to find and fix any bugs that we might have > > missed after integration. To me, that feels a bit short. Either we wait > > after the fork to integrate, or we integrate with two implementations and a > > `java` option for choosing implementation. I'm open to other opinions. > > Yes, time is tight. The chance of catching up the December 5th is not high. > Anyway, we have already decided to add a new JVM option to enable/disable > this new VMATree ( default = disabled). I don't believe that we have decided that already. Both me and, I believe, Gerard have been negative towards the proposal to keep both implementations. I'd like to use this as an opportunity to discuss the pros and cons of different approaches, which is why I asked for your opinion on my alternative. I believe that it is very important that we all agree on the approach taken, as we will all have to deal with the consequences of that approach. > ( default = disabled). Why should the default be disabled? That requires customers to explicitly pick the new tree to be used, which they are very unlikely to do. As I understand it, and correct me if I am wrong, the main goal of having two implementations is the ability to revert back in case we have bugs in the new one. If the default is to have the new implementation be disabled, then how are we ever going to find any real use bugs? Cheers, Johan - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379082883
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 10:10:37 GMT, Johan Sjölen wrote:
> Sure, I can understand that it's nice to have both versions present during
> development. Right now it seems like we have a broken build, do you have any
> plans on having a functioning and fully featured build soon?
The new commit is pushed. I will monitor the build results.
> Rather than having 2 implementations, I'd like to see us aiming for
> integration for JDK-25 after forking 24, so integration in December. That
> would give us 6 months of ensuring stability of the new implementation before
> it reaches any customers. During those 6 months it will go through all of our
> testing multiple times over, and be used by OpenJDK developers. What do you
> think of this idea?
No objection. We can do this way.
>> It is inherited because it is not a new type. I think that
>> `_tree->visit_reserved_region(par1, par2)` is more readable than
>> `VMATree::visit_reserved_region(_tree, par1, par2)`
>> I could not find any *virtual functions* in these classes, what do you mean
>> by that?
>
> I'm saying that inheritance is mostly useful when we have virtual functions
> which we can specialize, and that `VMATree` has none.
Done.
I used inheritance for extending a class.
>> How to use the ctor with a `bool` arg, then?
>
> The bool argument is just passed along.
>
> ```c++
> RegionsTree(bool with_storage) : VMATree(), _ncs_storage(with_storage) {
> }
Done.
For my curiosity, what is the advantage?
>> Can we do this change as a separate RFE? It impacts the code a lot.
>
> Then just invert it: Have the outer class be static and the inner class be an
> instance. We can change the `MemoryFileTracker` to be that, as it's not as
> large of a change.
It is still a big change. Why not another RFE?
>> Generally, I think it is necessary to have performance tests to verify if
>> any future changes do not degrade the performance.
>> The approach of stress testing can be agreed between us, but the tests have
>> to exist.
>> In this approach, the input size is scaled N times and we expect the
>> execution time grows as O(log(N)) which is much less than N.
>> This test fails and we need to have a justification for it. If approach is
>> invalid or not correct implemented, we fix it. But the expectations remains
>> the same.
>
> Why would the execution time grow logarithmically when we do linearly more
> work? When we run this with `N2` we will perform `10_000 * log(10_000, 2)`
> units of work, and for `N1` we will perform `1_000 * log(1_000, 2)` units of
> work, approximately. A closer approximation is `O(log(1)) + O(log(2)) + ... +
> O(log(n))` for `n = N2` or `n = N1`.
>
> Let's calculate that:
>
>
import math
def time_it(n):
> ... t = 0
> ... for i in range(1, n):
> ... t = t + math.log(i)
> ... return [t, 3*t] # Approximate best and worst case
> ...
time_it(1000)
> [5905.220423209189, 17715.661269627566]
time_it(10_000)
> [82099.71749644217, 246299.15248932652]
def compare(a, b):
> ... ab, aw = a
> ... bb, bw = b
> ... return [ bb / ab, bw / aw ]
> ...
compare(time_it(1000), time_it(10_000))
> [13.902904821938064, 13.902904821938066]
# Ouch, that's outside of our linear bound!
>
>
> What I said previously also applies, especially the performance of `malloc`
> will impact this I suspect.
It is considered that `malloc` or other external events are the same for two
cases. If we know that there might be some noise for one or another, we should
check and disable them. This is the approach I have talked. How can we avoid
noise from `malloc` side?
-
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379615290
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379618102
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713461169
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704394160
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704401031
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704424029
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 11:43:19 GMT, Johan Sjölen wrote: > Why should the default be disabled? That requires customers to explicitly > pick the new tree to be used, which they are very unlikely to do. As I > understand it, and correct me if I am wrong, the main goal of having two > implementations is the ability to revert back in case we have bugs in the new > one. If the default is to have the new implementation be disabled, then how > are we ever going to find any real use bugs? You are right. This should be defualt = enabled. Anyway, we are not going to have two versions. > I don't believe that we have decided that already. Both me and, I believe, > Gerard have been negative towards the proposal to keep both implementations. > I'd like to use this as an opportunity to discuss the pros and cons of > different approaches, which is why I asked for your opinion on my > alternative. I believe that it is very important that we all agree on the > approach taken, as we will all have to deal with the consequences of that > approach. Sorry for misunderstanding. We go for 1 version only. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379619838 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379622866
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Fri, 8 Nov 2024 09:58:48 GMT, Severin Gehwolf wrote: >> test/jtreg-ext/requires/VMProps.java line 28: >> >>> 26: import java.lang.module.ModuleFinder; >>> 27: import java.lang.module.ModuleReader; >>> 28: import java.lang.module.ModuleReference; >> >> I assume they aren't needed now. > > True. fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834118546
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 16:05:33 GMT, Afshin Zafari wrote: > > Rather than having 2 implementations, I'd like to see us aiming for > > integration for JDK-25 after forking 24, so integration in December. That > > would give us 6 months of ensuring stability of the new implementation > > before it reaches any customers. During those 6 months it will go through > > all of our testing multiple times over, and be used by OpenJDK developers. > > What do you think of this idea? > > No objection. We can do this way. Thank you Afshin for agreeing and thank you Johan for clearly stating the arguments. I was trying to think of a precedent where we shipped JDK with 2 small implementation details that were exposed via runtime flags, but nothing comes to mind... - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379627354
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 27 Sep 2024 16:10:59 GMT, Gerard Ziemski wrote: > > > Rather than having 2 implementations, I'd like to see us aiming for > > > integration for JDK-25 after forking 24, so integration in December. That > > > would give us 6 months of ensuring stability of the new implementation > > > before it reaches any customers. During those 6 months it will go through > > > all of our testing multiple times over, and be used by OpenJDK > > > developers. What do you think of this idea? > > > > > > No objection. We can do this way. > > Thank you Afshin for agreeing and thank you Johan for clearly stating the > arguments. > > I was trying to think of a precedent where we shipped JDK with 2 small > implementation details that were exposed via runtime flags, but nothing comes > to mind... Yes, thank you Afshin for reconsidering. I do sympathise with the goal of ensuring that no user receives a faulty build. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379664905
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 14 Oct 2024 12:37:05 GMT, Johan Sjölen wrote: > Yes, but this code is incorrect. So we should have a test detecting this, but > we do not, and so this is under-tested. This code finds committed regions within a Stack region if they are not accounted by NMT so far, IIUC. By running this incorrect code, we may get some extra committed size in NMT reports. This is hard in NMT tests to catch. How can we expect a NMT test to measure X bytes of committed stack and not Y bytes? We need to know exactly the X and/or Y to test this code. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2411263187
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 14 Oct 2024 10:24:54 GMT, Afshin Zafari wrote: > > ```c++ > > region->add_committed_region(committed_start, committed_size, ncs); // <-- > > LOST! > > ``` > > The `region` is not a VMATree::node, it is a `ReservedMemoryRegion*`. I don't understand what you're trying to say here. Do you see the bug that I'm talking about? > > Is this part of the code under-tested as we didn't receive a test failure > > regarding this? > > All JTREG runtime/NMT tests and all tier1 tests have been passed on this code. Yes, but this code is incorrect. So we should have a test detecting this, but we do not, and so this is under-tested. - PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-245944
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 14 Oct 2024 08:50:52 GMT, Afshin Zafari wrote:
>> Why is this assert triggered? That sounds like a bug.
>
> The assertion that happens during building jdk-image:
>
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # Internal Error
> (/home/afshin/scratch/833XX_nmt_VMT_with_tree/src/hotspot/share/utilities/growableArray.hpp:142),
> pid=1234272, tid=1234278
> # assert(0 <= i && i < _len) failed: illegal index 0 for length 0
> #
> # JRE version: (24.0) (fastdebug build )
> # Java VM: OpenJDK 64-Bit Server VM (fastdebug
> 24-internal-2024-09-30-0937351.afshin..., mixed mode, emulated-client,
> tiered, compressed oops, compressed class ptrs, serial gc, linux-amd64)
> # Core dump will be written. Default location: Core dumps may be processed
> with "/usr/share/apport/apport -p%p -s%s -c%c -d%d -P%P -u%u -g%g -- %E" (or
> dumping to /home/afshin/scratch/833XX_nmt_VMT_with_tree/make/core.1234272)
> #
> #
>
> --- S U M M A R Y
>
> Command Line: -Denv.class.path=
> -Dapplication.home=/home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/jdk
> -Xms8m -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
> -Djdk.module.main=jdk.jlink jdk.jlink/jdk.tools.jmod.Main create
> --module-version 24-internal --target-platform linux-amd64 --module-path
> /home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/images/jmods
> --libs
> /home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/support/modules_libs/jdk.sctp
> --class-path
> /home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/jdk/modules/jdk.sctp
> --legal-notices
> /home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/support/modules_legal/common
> --exclude **{_the.*,_*.marker*,*.diz,*.debuginfo,*.dSYM/**,*.dSYM}
> --compress zip-1 --date 2024-09-30T09:37:57Z
> /home/afshin/scratch/833XX_nmt_VMT_with_tree/build/linux-x64/support/images/jmods/jdk.sctp.jmod
>
> Host: afshin-Precision-7920-Tower, Intel(R) Xeon(R) Silver 4114 CPU @
> 2.20GHz, 40 cores, 62G, Ubuntu 20.04.6 LTS
> Time: Mon Oct 14 10:52:34 2024 CEST elapsed time: 0.002402 seconds (0d 0h 0m
> 0s)
>
> --- T H R E A D ---
>
> Current thread is native thread
>
> Stack: [0x7ffa5243a000,0x7ffa5253b000], sp=0x7ffa52539390, free
> space=1020k
> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native
> code)
> V [libjvm.so+0x18df026] VMError::report_and_die(int, char const*, char
> const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*,
> int, unsigned long)+0x486 (growableArray.hpp:142)
> V [libjvm.so+0x18df870] VMError::report_and_die(Thread*, void*, char
> const*, int, char const*, char const*,...
OK, so there's a bug in the new VMT. You should be getting the command run so
that you can reproduce it in gdb. If not, you can add `LOG=cmdlines` as an
environment variable before building.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1802521408
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 5 Aug 2024 16:50:00 GMT, Afshin Zafari wrote:
>> The bool argument is just passed along.
>>
>> ```c++
>> RegionsTree(bool with_storage) : VMATree(), _ncs_storage(with_storage) {
>> }
>
> Done.
> For my curiosity, what is the advantage?
1. No malloc
2. No indirection, so no cache misses
3. A clear lifetime and clear ownership, both are bound to the `RegionsTree`
object
>> Then just invert it: Have the outer class be static and the inner class be
>> an instance. We can change the `MemoryFileTracker` to be that, as it's not
>> as large of a change.
>
> It is still a big change. Why not another RFE?
Because we want to switch to a new way of exposing the public interface, as
opposed to an under-the-hood optimization. This PR is still in draft and we're
far away from next RDP1, we're not in a hurry and can afford to get this right.
I do not want to repeat the same mistakes of the old codebase.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711099968
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711190300
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 9 Aug 2024 09:05:37 GMT, Johan Sjölen wrote: >> Done. >> For my curiosity, what is the advantage? > > 1. No malloc > 2. No indirection, so no cache misses > 3. A clear lifetime and clear ownership, both are bound to the `RegionsTree` > object OK. Thanks for your description. - PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713371752
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 9 Aug 2024 15:05:59 GMT, Johan Sjölen wrote:
>> src/hotspot/share/nmt/regionsTree.hpp line 46:
>>
>>> 44: using Node = VMATree::TreapNode;
>>> 45:
>>> 46: class NodeHelper : public Node {
>>
>> This shouldn't inherit from `Node` and then have each instance be cast into
>> `NodeHelper`. Make into `class Utils : public AllStatic`.
>
> Alternatively create it by composition:
>
> ```c++
> class NodeHelper {
> Node& node;
> NodeHelper(Node* node) : node(*node) {}
> // All of the methods
> };
>
> { // Some Node* node
> NodeHelper nh(node);
> // Use nh
> }
Done. The alternative way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713462208
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 5 Aug 2024 08:42:43 GMT, Johan Sjölen wrote:
>> Yeah, that doesn't seem like a problem.
>>
>> ```c++
>> for (int i = 0; i < mt_number_of_types; i++) {
>> r = diff.flag[i].reserve;
>> c = diff.flag[i].commit;
>> flag = NMTUtil::index_to_flag(i);
>> VirtualMemory* mem = VirtualMemorySummary::as_snapshot()->by_type(flag);
>> reserved = mem->reserved();
>> committed = mem->committed();
>> mem->reserve_memory(r);
>> mem->commit_memory(c);
>> if ((size_t)-r > reserved) {
>> print_err("release");
>> }
>> if ((size_t)-c > reserved || (size_t)c > committed) {
>> print_err("uncommit");
>> }
>> }
>
> This applies the reserve/commit regardless of outcome, so slightly different.
The main purpose of the `if (...)` cases is to find if the request to apply the
delta is valid or not. There are related assertions in VirtualMemory but not so
informative. Also, using `log_debug` lets the build proceed and just show the
messages.
These messages help a lot when something goes wrong in terms of
commit/uncommit/release failure.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704398731
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 5 Aug 2024 08:41:49 GMT, Johan Sjölen wrote:
>> In `MemoryFileTracker`, the changes in commit/reserve are applied to a local
>> `VirtualMemorySnapshot`. Here we have to apply them to the global
>> `VirtualMemorySummary`.
>
> Yeah, that doesn't seem like a problem.
>
> ```c++
> for (int i = 0; i < mt_number_of_types; i++) {
> r = diff.flag[i].reserve;
> c = diff.flag[i].commit;
> flag = NMTUtil::index_to_flag(i);
> VirtualMemory* mem = VirtualMemorySummary::as_snapshot()->by_type(flag);
> reserved = mem->reserved();
> committed = mem->committed();
> mem->reserve_memory(r);
> mem->commit_memory(c);
> if ((size_t)-r > reserved) {
> print_err("release");
> }
> if ((size_t)-c > reserved || (size_t)c > committed) {
> print_err("uncommit");
> }
> }
This applies the reserve/commit regardless of outcome, so slightly different.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1703753128
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Fri, 9 Aug 2024 10:03:52 GMT, Johan Sjölen wrote: >> The main purpose of the `if (...)` cases is to find if the request to apply >> the delta is valid or not. There are related assertions in VirtualMemory but >> not so informative. Also, using `log_debug` lets the build proceed and just >> show the messages. >> These messages help a lot when something goes wrong in terms of >> commit/uncommit/release failure. > > How does my example code not account for this? You can still get rid of the > sign checking. Case of 'commit' error is missing from your suggestion. At commit time, ` c > reserved` is invalid too. `(size_t)-r` for positive `r` is a large number and is greater than `reserved`. We have to find out the intent of the call first (by checking the sign of `r` or `c`) and then check if it is valid or not. - PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713513436
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 5 Aug 2024 17:20:24 GMT, Afshin Zafari wrote: >> Why would the execution time grow logarithmically when we do linearly more >> work? When we run this with `N2` we will perform `10_000 * log(10_000, 2)` >> units of work, and for `N1` we will perform `1_000 * log(1_000, 2)` units of >> work, approximately. A closer approximation is `O(log(1)) + O(log(2)) + ... >> + O(log(n))` for `n = N2` or `n = N1`. >> >> Let's calculate that: >> >> > import math > def time_it(n): >> ... t = 0 >> ... for i in range(1, n): >> ... t = t + math.log(i) >> ... return [t, 3*t] # Approximate best and worst case >> ... > time_it(1000) >> [5905.220423209189, 17715.661269627566] > time_it(10_000) >> [82099.71749644217, 246299.15248932652] > def compare(a, b): >> ... ab, aw = a >> ... bb, bw = b >> ... return [ bb / ab, bw / aw ] >> ... > compare(time_it(1000), time_it(10_000)) >> [13.902904821938064, 13.902904821938066] > # Ouch, that's outside of our linear bound! >> >> >> What I said previously also applies, especially the performance of `malloc` >> will impact this I suspect. > > It is considered that `malloc` or other external events are the same for two > cases. If we know that there might be some noise for one or another, we > should check and disable them. This is the approach I have talked. How can we > avoid noise from `malloc` side? When it is said that an algorithm has the log(n) time-complexity, it means that if the input grows n times, the times grows log(n) times. The tree data-structure has log(n) time-complexity. VMATree may have not exactly log(n) response times due to self-balancing costs. But it is still expected to be less than O(n). - PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704427407
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 5 Aug 2024 16:54:40 GMT, Afshin Zafari wrote: >> This applies the reserve/commit regardless of outcome, so slightly different. > > The main purpose of the `if (...)` cases is to find if the request to apply > the delta is valid or not. There are related assertions in VirtualMemory but > not so informative. Also, using `log_debug` lets the build proceed and just > show the messages. > These messages help a lot when something goes wrong in terms of > commit/uncommit/release failure. How does my example code not account for this? You can still get rid of the sign checking. - PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711187386
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Mon, 5 Aug 2024 17:23:46 GMT, Afshin Zafari wrote:
>> It is considered that `malloc` or other external events are the same for two
>> cases. If we know that there might be some noise for one or another, we
>> should check and disable them. This is the approach I have talked. How can
>> we avoid noise from `malloc` side?
>
> When it is said that an algorithm has the log(n) time-complexity, it means
> that if the input grows n times, the times grows log(n) times. The tree
> data-structure has log(n) time-complexity. VMATree may have not exactly
> log(n) response times due to self-balancing costs. But it is still expected
> to be less than O(n).
Hi Afshin, could we take a step back and do some asymptotic time complexity
analysis of this problem?
The test is measuring the following code:
```c++
for (int i = 0; i < n; i++) {
int a = (os::random() % n) * 100;
treap.upsert(a, 0);
}
So this algorithm is the tme complexity which we are trying to understand.
First, let's simplify the code slightly:
```c++
auto f = [&](auto n) { int a = (os::random() % n) * 100; treap.upsert(a, i); };
for (int i = 0; i < n; i++) {
f(i);
}
Clearly, `f(n)` is executed `n` times, agreed? Then the time complexity of the
whole program must be `O(n*f(n))`, correct? It's the time complexity of `f(n)`
performed `n` times.
Let's replace `f` with something else to see if we can understand the time
complexity of the whole code snippet again.
```c++
int arr[n];
auto f = [&](auto n) { arr[n] = 0; };
for (int i = 0; i < n; i++) {
f(i);
}
Now, we both agree that assigning to an array has time complexity `O(1)`,
correct? Then, if we fill that in in our expression `O(n * f(n))` we receive
`O(n * 1) = O(n)`, correct? In other words, the time complexity of the
algorithm the test is measuring is *linear*, and we ought to expect that with
an array the time taken for the array should be 10x longer with 10k elements as
compared to 1k elements.
OK, now let's *assume* that `f(n)` has time complexity `O(log n)`, then
shouldn't the algorithm we're measuring have time complexity `O(n * log n)`,
that is actually *slower* than `O(n)`.
In conclusion: if `treap.upsert()` has time complexity `O(log n)` then the
whole algorithm should have time complexity `O(n * log n)` and the measurements
we're seeing are as expected *and the test shouldn't fail*.
Have I missed anything or made any mistakes? Please let me know.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1705025716
Re: RFR: 8337217: Port VirtualMemoryTracker to use VMATree
On Tue, 6 Aug 2024 07:12:13 GMT, Johan Sjölen wrote:
>> When it is said that an algorithm has the log(n) time-complexity, it means
>> that if the input grows n times, the times grows log(n) times. The tree
>> data-structure has log(n) time-complexity. VMATree may have not exactly
>> log(n) response times due to self-balancing costs. But it is still expected
>> to be less than O(n).
>
> Hi Afshin, could we take a step back and do some asymptotic time complexity
> analysis of this problem?
>
> The test is measuring the following code:
>
> ```c++
> for (int i = 0; i < n; i++) {
> int a = (os::random() % n) * 100;
> treap.upsert(a, 0);
> }
>
>
> So this algorithm is the tme complexity which we are trying to understand.
> First, let's simplify the code slightly:
>
> ```c++
> auto f = [&](auto n) { int a = (os::random() % n) * 100; treap.upsert(a, i);
> };
> for (int i = 0; i < n; i++) {
> f(i);
> }
>
>
> Clearly, `f(n)` is executed `n` times, agreed? Then the time complexity of
> the whole program must be `O(n*f(n))`, correct? It's the time complexity of
> `f(n)` performed `n` times.
>
> Let's replace `f` with something else to see if we can understand the time
> complexity of the whole code snippet again.
>
> ```c++
> int arr[n];
> auto f = [&](auto n) { arr[n] = 0; };
> for (int i = 0; i < n; i++) {
> f(i);
> }
>
>
> Now, we both agree that assigning to an array has time complexity `O(1)`,
> correct? Then, if we fill that in in our expression `O(n * f(n))` we receive
> `O(n * 1) = O(n)`, correct? In other words, the time complexity of the
> algorithm the test is measuring is *linear*, and we ought to expect that with
> an array the time taken for the array should be 10x longer with 10k elements
> as compared to 1k elements.
>
> OK, now let's *assume* that `f(n)` has time complexity `O(log n)`, then
> shouldn't the algorithm we're measuring have time complexity `O(n * log n)`,
> that is actually *slower* than `O(n)`.
>
> In conclusion: if `treap.upsert()` has time complexity `O(log n)` then the
> whole algorithm should have time complexity `O(n * log n)` and the
> measurements we're seeing are as expected *and the test shouldn't fail*.
>
> Have I missed anything or made any mistakes? Please let me know.
The big O is a _notation_ and is not a math function. So `O(a * b)` is not
always same as `O(a) * O(b)`.
Stick to this _definition_: "when an algorithm has time-complexity of
O(`g(n)`), it means if the input grows `n` times the time of executing the
algorithm grows `g(n)` times." Where `g()` is `log()` in our case.
IOW, the big O answers the following question:
$t_1$ = time for running `f()` for $n_1$ items
$t_2$ = time for running `f()` for $n_2$ items
if we know $\Large{\frac{n_2}{n_1} = n}$ what is expected value of $t_2$?
A detailed description can be found
[here](https://en.wikipedia.org/wiki/Big_O_notation).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1705161898
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Fri, 8 Nov 2024 10:34:43 GMT, Severin Gehwolf wrote: >> That's okay, I wasn't initially sure why they were changed. I'm looking at >> JRTArchiveFile.toEntry and wondering there should be a follow-up issue (not >> this PR) to fail early if running on a patched run-time even though it would >> be an odd configuration to attempt to do that. > > There already is. See: > https://github.com/openjdk/jdk/pull/14787/files#diff-b6b47eacb6060eb0a583a253f322f5d274063e082a12a72e8628a6e1ba6cdd3eR466-R471 > > It's also tested with > [PatchedJDKModuleJlinkTest.java](https://github.com/openjdk/jdk/pull/14787/files#diff-11b8a26307346fd7ca016a349243cabd3b982964aaf4335298e28e956b3968eb). > Do we need more? Yes, I know there is a test. It's "no such file" case in JRTArchiveFile.toEntry that I'm looking at. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834151588
RFR: 8211033: Clean up the processing -classpath argument not to set LM_CLASS
Can I please get a review of this change which addresses https://bugs.openjdk.org/browse/JDK-8211033? As noted in that issue, this is a clean up of the code which determines the "mode" through with the `java` application is being launched. In its current form the presence of `--classpath` (or its equivalent arguments) was unnecessary updating the mode to `LM_CLASS`. The commit in this PR removes that part to allow for the mode to be detected based merely on the presence (or absence) of `-m`, `-jar`, `--source` options. If neither is specified, the file extension is checked to determine the launch mode. Given the nature of this clean up, no new tests have been introduced. Existing tests in tier1, tier2, tier3 continue to pass with this change. - Commit messages: - 8211033: Clean up the processing -classpath argument not to set LM_CLASS Changes: https://git.openjdk.org/jdk/pull/21971/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21971&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8211033 Stats: 8 lines in 1 file changed: 3 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21971.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21971/head:pull/21971 PR: https://git.openjdk.org/jdk/pull/21971
Re: RFR: 8211033: Clean up the processing -classpath argument not to set LM_CLASS
On Fri, 8 Nov 2024 08:51:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.org/browse/JDK-8211033? > > As noted in that issue, this is a clean up of the code which determines the > "mode" through with the `java` application is being launched. In its current > form the presence of `--classpath` (or its equivalent arguments) was > unnecessary updating the mode to `LM_CLASS`. The commit in this PR removes > that part to allow for the mode to be detected based merely on the presence > (or absence) of `-m`, `-jar`, `--source` options. If neither is specified, > the file extension is checked to determine the launch mode. > > Given the nature of this clean up, no new tests have been introduced. > Existing tests in tier1, tier2, tier3 continue to pass with this change. On the surface this looks okay but just to double check: this has no impact on `java -cp -jar app.jar`. The class path in that case is `app.jar`, the class path specified to -cp is ignored. - PR Comment: https://git.openjdk.org/jdk/pull/21971#issuecomment-2464224551
