Re: RFR: 8286956: Loom: Define test groups for development/porting use

[Zhengyu Gu]

On Wed, 18 May 2022 11:10:58 GMT, Aleksey Shipilev  wrote:

> It would be beneficial to bring over the Loom-specific test groups from the 
> loom repo to aid development/porting work.
> 
> https://github.com/openjdk/loom/blob/fibers/test/jdk/TEST.groups#L97-L108
> https://github.com/openjdk/loom/blob/6f42923b3342e41d95b262733205283068802b40/test/hotspot/jtreg/TEST.groups#L111-L125
> 
> I had to drop `jdk/incubator/concurrent` from JDK definition, because there 
> are no tests in that folder and tests break.
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug `hotspot_loom jdk_loom`
>  - [x] Linux AArch64 fastdebug `hotspot_loom jdk_loom`

This will help fixing the breakage of Shenandoah + Loom.

-

Marked as reviewed by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8765

Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold

[Zhengyu Gu]

On Fri, 13 May 2022 02:43:55 GMT, Jie Fu  wrote:

> Hi all,
> 
> Some tests fail with Shenandoah GC after JDK-8282191.
> The reason is that the assert in `ShenandoahControlThread::request_gc` misses 
> the case of `GCCause::_codecache_GC_threshold`.
> It would be better to fix it.
> 
> Thanks.
> Best regards,
> Jie

LGTM

-

Marked as reviewed by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8691

Integrated: 8282887: Potential memory leak in sun.util.locale.provider.HostLocaleProviderAdapterImpl.getNumberPattern() on Windows

[Zhengyu Gu]

On Thu, 10 Mar 2022 18:40:13 GMT, Zhengyu Gu  wrote:

> Please review this small patch that fixes early `CHECK_NULL_RETURN` results 
> not releasing native `jchar` array returned by `GetStringChars()`.

This pull request has now been integrated.

Changeset: 2cddf3f5
Author:Zhengyu Gu 
URL:   
https://git.openjdk.java.net/jdk/commit/2cddf3f5391518ea40796e6c4759047d51b7b312
Stats: 8 lines in 1 file changed: 2 ins; 3 del; 3 mod

8282887: Potential memory leak in 
sun.util.locale.provider.HostLocaleProviderAdapterImpl.getNumberPattern() on 
Windows

Reviewed-by: naoto, alanb

-

PR: https://git.openjdk.java.net/jdk/pull/

Re: RFR: 8282887: Potential memory leak in sun.util.locale.provider.HostLocaleProviderAdapterImpl.getNumberPattern() on Windows

[Zhengyu Gu]

On Thu, 10 Mar 2022 19:46:28 GMT, Naoto Sato  wrote:

>> Please review this small patch that fixes early `CHECK_NULL_RETURN` results 
>> not releasing native `jchar` array returned by `GetStringChars()`.
>
> LGTM.

Thanks, @naotoj @AlanBateman

-

PR: https://git.openjdk.java.net/jdk/pull/

Re: RFR: 8282887: Potential memory leak in sun.util.locale.provider.HostLocaleProviderAdapterImpl.getNumberPattern() on Windows

[Zhengyu Gu]

On Thu, 10 Mar 2022 18:40:13 GMT, Zhengyu Gu  wrote:

> Please review this small patch that fixes early `CHECK_NULL_RETURN` results 
> not releasing native `jchar` array returned by `GetStringChars()`.

Friendly ping. May I have a second review, please!

-

PR: https://git.openjdk.java.net/jdk/pull/

RFR: 8282887: Potential memory leak in sun.util.locale.provider.HostLocalProviderAdapterImpl.getNumberPattern() on Windows

[Zhengyu Gu]

Please review this small patch that fixes early `CHECK_NULL_RETURN` results not 
releasing native `jchar` array returned by `GetStringChars()`.

-

Commit messages:
 - 8282887: Potential memory leak in 
sun.util.locale.provider.HostLocaleProviderAdapterImpl.getNumberPattern() on 
Windows

Changes: https://git.openjdk.java.net/jdk/pull//files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk==00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282887
  Stats: 9 lines in 1 file changed: 2 ins; 3 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull//head:pull/

PR: https://git.openjdk.java.net/jdk/pull/

Integrated: 8282897: Fix call parameter to GetStringChars() in HostLocaleProviderAdapter_md.c

[Zhengyu Gu]

On Thu, 10 Mar 2022 13:33:05 GMT, Zhengyu Gu  wrote:

> Please review this trivial patch to correct last parameter of 
> `GetStringChars()` call, which should be a  `jboolean*`, instead of `jboolean`

This pull request has now been integrated.

Changeset: 879b6445
Author:Zhengyu Gu 
URL:   
https://git.openjdk.java.net/jdk/commit/879b6445e33ad3a07461d01ea8f28a09979a4313
Stats: 19 lines in 1 file changed: 0 ins; 0 del; 19 mod

8282897: Fix call parameter to GetStringChars() in 
HostLocaleProviderAdapter_md.c

Reviewed-by: shade, naoto

-

PR: https://git.openjdk.java.net/jdk/pull/7775

Re: RFR: 8282897: Fix call parameter to GetStringChars() in HostLocaleProviderAdapter_md.c

[Zhengyu Gu]

On Thu, 10 Mar 2022 17:25:55 GMT, Naoto Sato  wrote:

> LGTM. Thanks for the fix!

Thanks for the review.

-

PR: https://git.openjdk.java.net/jdk/pull/7775

Re: RFR: 8282897: Fix call parameter to GetStringChars() in HostLocaleProviderAdapter_md.c

[Zhengyu Gu]

On Thu, 10 Mar 2022 15:28:15 GMT, Aleksey Shipilev  wrote:

> Looks fine to me.

Thanks, @shipilev

-

PR: https://git.openjdk.java.net/jdk/pull/7775

RFR: 8282897: Fix call parameter to GetStringChars() in HostLocaleProviderAdapter_md.c

[Zhengyu Gu]

Please review this trivial patch to correct last parameter of 
`GetStringChars()` call, which should be a  `jboolean*`, instead of `jboolean`

-

Commit messages:
 - 8282897: Fix call parameter to GetStringChars() in 
HostLocaleProviderAdapter_md.c

Changes: https://git.openjdk.java.net/jdk/pull/7775/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7775=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282897
  Stats: 19 lines in 1 file changed: 0 ins; 0 del; 19 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7775.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7775/head:pull/7775

PR: https://git.openjdk.java.net/jdk/pull/7775

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

[Zhengyu Gu]

On Tue, 8 Mar 2022 21:05:13 GMT, Alexey Ivanov  wrote:

>> I think `NewStringUTF()` can throw OOM and also return `NULL`, just which 
>> one you pick.
>
>> I think `NewStringUTF()` can throw OOM and also return `NULL`, just which 
>> one you pick.
> 
> Yes.
> 
> But we're discussing whether a check for exception is necessary after 
> `(*env)->SetObjectArrayElement`.

`SetObjectArrayElement()` may throw `ArrayIndexOutOfBoundsException` and 
`ArrayStoreException`, but I don't see it is possible here.

-

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

[Zhengyu Gu]

On Tue, 8 Mar 2022 20:57:27 GMT, Alexey Ivanov  wrote:

>>> ??? You want to check and cleanup if NewStringUTF fails. You only want to 
>>> call SetObjectElementArray if NewStringUTF succeeds.
>> 
>> If the SetObjectArrayElement will throw an exception we will call the 
>> NewStringUTF while the exception is raised which is a kind of "jni" issue. 
>> If such an exception is possible we should check and cleanup.
>
> A quick search for `SetObjectArrayElement` in `java.desktop` module shows the 
> call to `SetObjectArrayElement` is rarely followed by exception check.
> 
> What kind of exception can `SetObjectArrayElement` raise if the index is 
> within the range and the stored element is of correct type?

I think `NewStringUTF()` can throw OOM and also return `NULL`, just which one 
you pick.

-

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

[Zhengyu Gu]

On Tue, 8 Mar 2022 15:58:57 GMT, Alexey Ivanov  wrote:

> > I did a quick grep, there are a few suspicious spots, e.g. 
> > [https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711](url),
> >  I have yet take a close look.
> 
> It doesn't leak the icon handle here, it's assigned to the member of the 
> object (probably `NULL`). Yet it's inconsistent: if an exception is expected 
> from `CreateIconFromRaster` on the line above, why isn't it checked after 
> `CreateIconFromRaster` is called on the following line?

I am not familiar with this code. What I see is that, it assigns `m_hIcon` and 
`m_hIconSm` to local variables, then reset them to `NULL`, once exception 
returns, are references to icon and small icon handles lost? and it will 
destroy icons as L2727 - 2732 do. But I may miss something.

-

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

[Zhengyu Gu]

> Please review this small patch that fixes a potential memory leak that 
> exception return fails to release allocated `cacheDirs`
> 
> Test:
> 
> - [x] jdk_desktop on Linux x86_64

Zhengyu Gu has updated the pull request incrementally with one additional 
commit since the last revision:

  mrserb and aivanov-jdk's comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7691/files
  - new: https://git.openjdk.java.net/jdk/pull/7691/files/0ff07ddd..c4a3ec87

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7691=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7691=00-01

  Stats: 23 lines in 2 files changed: 2 ins; 18 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7691.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7691/head:pull/7691

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[Zhengyu Gu]

On Fri, 4 Mar 2022 13:25:12 GMT, Zhengyu Gu  wrote:

> Please review this small patch that fixes a potential memory leak that 
> exception return fails to release allocated `cacheDirs`
> 
> Test:
> 
> - [x] jdk_desktop on Linux x86_64

> 



> > The macros are used to hide the different syntax of c and c++ to avoid 
> > polluting code here, it seems to be a common pattern in client code.
> 
> That's to allow for general use by C or C++ source files. But you are only 
> needing this in a C source file so you would only need the C variant of the 
> calling syntax.



> > The macros are used to hide the different syntax of c and c++ to avoid 
> > polluting code here, it seems to be a common pattern in client code.
> 
> That's to allow for general use by C or C++ source files. But you are only 
> needing this in a C source file so you would only need the C variant of the 
> calling syntax.

I don't like the macros. I can argument all `JNU_CHECK_EXCEPTION(_RETURN)` 
macros are particular bad, because these patterns can easily result premature 
returns without proper cleanup. This bug is an evidence. But they exist for a 
long time, and I just follow the convention :-( I would prefer to eliminate 
them all, as inline code is not big at all.

I did a quick grep, there are a few suspicious spots, e.g. 
[https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711](url),
 I have yet take a close look.

-

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[Zhengyu Gu]

On Fri, 4 Mar 2022 21:30:53 GMT, David Holmes  wrote:

> I would have just inlined JNU_CHECK_EXCEPTION and added the cleanup code 
> directly. The additional macro wasn't really necessary and would not really 
> be usable for any kind of general cleanup actions beyond a one-liner.
> 
> But it is okay.
> 
> Thanks.

Thanks for the review, @dholmes-ora .

The macros are used to hide the different syntax of c and c++ to avoid 
polluting code here, it seems to be a common pattern in client code.

-

PR: https://git.openjdk.java.net/jdk/pull/7691

RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[Zhengyu Gu]

Please review this small patch that fixes a potential memory leak that 
exception return fails to release allocated `cacheDirs`

Test:

- [x] jdk_desktop on Linux x86_64

-

Commit messages:
 - 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

Changes: https://git.openjdk.java.net/jdk/pull/7691/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7691=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282628
  Stats: 21 lines in 2 files changed: 18 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7691.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7691/head:pull/7691

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

[Zhengyu Gu]

On Wed, 4 Aug 2021 03:36:11 GMT, Thomas Stuefe  wrote:

> Nightlies at SAP showed no problems for several runs now. The failed GHA test 
> (StringDeduplication) seems to have nothing to do with my patch.
> 
> @zhengyu123 are you fine with the latest version of this patch?

Still good.

-

PR: https://git.openjdk.java.net/jdk/pull/4874

Re: RFR: JDK-8256844: Make NMT late-initializable

[Zhengyu Gu]

On Thu, 22 Jul 2021 14:58:47 GMT, Thomas Stuefe  wrote:

> Short: this patch makes NMT available in custom-launcher scenarios and during 
> gtests. It simplifies NMT initialization. It adds a lot of NMT-specific 
> testing, cleans them up and makes them sideeffect-free.
> 
> -
> 
> NMT continues to be an extremely useful tool for SAP to tackle memory 
> problems in the JVM.
> 
> However, NMT is of limited use due to the following restrictions:
> 
> - NMT cannot be used if the hotspot is embedded into a custom launcher unless 
> the launcher actively cooperates. Just creating and invoking the JVM is not 
> enough, it needs to do some steps prior to loading the hotspot. This 
> limitation is not well known (nor, do I believe, documented). Many products 
> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this 
> problem limits NMT usefulness greatly since our VMs are often embedded into 
> custom launchers and modifying every launcher is impossible.
> - Worse, if that custom launcher links the libjvm *statically* there is just 
> no way to activate NMT at all. This is the reason NMT cannot be used in the 
> `gtestlauncher`.
> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` 
> and `-XX:Flags=`.
> - The fact that NMT cannot be used in gtests is really a pity since it would 
> allow us to both test NMT itself more rigorously and check for memory leaks 
> while testing other stuff.
> 
> The reason for all this is that NMT initialization happens very early, on the 
> first call to `os::malloc()`. And those calls happen already during dynamic 
> C++ initialization - a long time before the VM gets around parsing arguments. 
> So, regular VM argument parsing is too late to parse NMT arguments.
> 
> The current solution is to pass NMT arguments via a specially prepared 
> environment variable: `NMT_LEVEL_=`. That environment 
> variable has to be set by the embedding launcher, before it loads the libjvm. 
> Since its name contains the PID, we cannot even set that variable in the 
> shell before starting the launcher.
> 
> All that means that every launcher needs to especially parse and process the 
> NMT arguments given at the command line (or via whatever method) and prepare 
> the environment variable. `java` itself does this. This only works before the 
> libjvm.so is loaded, before its dynamic C++ initialization. For that reason, 
> it does not work if the launcher links statically against the hotspot, since 
> in that case C++ initialization of the launcher and hotspot are folded into 
> one phase with no possibility of executing code beforehand.
> 
> And since it bypasses argument handling in the VM, it bypasses a number of 
> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`.
> 
> --
> 
> This patch fixes these shortcomings by making NMT late-initializable: it can 
> now be initialized after normal VM argument parsing, like all other parts of 
> the VM. This greatly simplifies NMT initialization and makes it work 
> automagically for every third party launcher, as well as within our gtests.
> 
> The glaring problem with late-initializing NMT is the NMT malloc headers. If 
> we rule out just always having them (unacceptable in terms of memory 
> overhead), there is no safe way to determine, in os::free(), if an allocation 
> came from before or after NMT initialization ran, and therefore what to do 
> with its malloc headers. For a more extensive explanation, please see the 
> comment block `nmtPreInit.hpp` and the discussion with @kimbarrett and 
> @zhengyu123 in the JBS comment section.
> 
> The heart of this patch is a new way to track early, pre-NMT-init 
> allocations. These are tracked via a lookup table. This was a suggestion by 
> Kim and it worked out well.
> 
> Changes in detail:
> 
> - pre-NMT-init handling:
>   - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init 
> handling. They contain a small global lookup table managing C-heap blocks 
> allocated in the pre-NMT-init phase.
>   - `os::malloc()/os::realloc()/os::free()` defer to this code before 
> doing anything else.
>   - Please see the extensive comment block at the start of 
> `nmtPreinit.hpp` explaining the details.
> 
> - Changes to NMT:
>   - Before, NMT initialization was spread over two phases, `initialize()` 
> and `late_initialize()`. Those were merged into one and simplified - there is 
> only one initialization now which happens after argument parsing.
>   - Minor changes were needed for the `NMT_TrackingLevel` enum - to 
> simplify code, I changed NMT_unknown to be numerically 0. A new comment block 
> in `nmtCommon.hpp` now clearly specifies what's what, including allowed level 
> state transitions.
>   - New utility functions to translate tracking level from/to strings 
> added to `NMTUtil`
>   - NMT has never been able to handle virtual memory allocations before 
> initialization, which is fine since os::reserve_memory() is not 

Re: RFR: JDK-8256844: Make NMT late-initializable

[Zhengyu Gu]

On Fri, 30 Jul 2021 16:36:41 GMT, Thomas Stuefe  wrote:

>> src/hotspot/share/services/nmtPreInit.hpp line 309:
>> 
>>> 307: ::memcpy(p_new, a->payload(), MIN2(a->size, new_size));
>>> 308: (*rc) = p_new;
>>> 309: _num_reallocs_pre_to_post++;
>> 
>> post-NMT-init counter updates are racy.
>
> True, this is racy. It's just for diagnostics though - I rather remove them 
> than make them atomic since we would pay for this with every malloc. Or, 
> maybe atomic + debug only?

Either one is fine.

-

PR: https://git.openjdk.java.net/jdk/pull/4874

Re: RFR: JDK-8256844: Make NMT late-initializable

[Zhengyu Gu]

On Fri, 30 Jul 2021 16:33:17 GMT, Zhengyu Gu  wrote:

>> The code is implicitly thread-safe because the hashmap is only modified in 
>> the pre-NMT-init phase. After NMT initialization, the table is read-only. 
>> During pre-NMT-init we are effectively single-threaded - at most two threads 
>> access the map, the thread loading the libjvm, and the thread calling 
>> CreateJavaVM, but not at the same time.
>> 
>> See also the asserts in the AllStatic class `NMTPreInit`.
>> 
>> (I should have described it more clearly, will add a comment.)
>
> So, you are saying that there is no memory that is malloc'd pre-NMT-init 
> phase and freed post-NMT-init phase?

Okay, I see. It just leaks those memory, so the table can be read-only.

-

PR: https://git.openjdk.java.net/jdk/pull/4874

Re: RFR: JDK-8256844: Make NMT late-initializable

[Zhengyu Gu]

On Fri, 30 Jul 2021 16:28:54 GMT, Thomas Stuefe  wrote:

>> src/hotspot/share/services/nmtPreInit.hpp line 202:
>> 
>>> 200: assert((*aa) != NULL, "Entry not found: " PTR_FORMAT, p2i(p));
>>> 201: NMTPreInitAllocation* a = (*aa);
>>> 202: (*aa) = (*aa)->next; // remove from its list
>> 
>> Could this be a race? There is no synchronization, read/write result could 
>> be arbitrary.
>
> The code is implicitly thread-safe because the hashmap is only modified in 
> the pre-NMT-init phase. After NMT initialization, the table is read-only. 
> During pre-NMT-init we are effectively single-threaded - at most two threads 
> access the map, the thread loading the libjvm, and the thread calling 
> CreateJavaVM, but not at the same time.
> 
> See also the asserts in the AllStatic class `NMTPreInit`.
> 
> (I should have described it more clearly, will add a comment.)

So, you are saying that there is no memory that is malloc'd pre-NMT-init phase 
and freed post-NMT-init phase?

-

PR: https://git.openjdk.java.net/jdk/pull/4874

Re: RFR: JDK-8256844: Make NMT late-initializable

[Zhengyu Gu]

On Thu, 22 Jul 2021 14:58:47 GMT, Thomas Stuefe  wrote:

> Short: this patch makes NMT available in custom-launcher scenarios and during 
> gtests. It simplifies NMT initialization. It adds a lot of NMT-specific 
> testing, cleans them up and makes them sideeffect-free.
> 
> -
> 
> NMT continues to be an extremely useful tool for SAP to tackle memory 
> problems in the JVM.
> 
> However, NMT is of limited use due to the following restrictions:
> 
> - NMT cannot be used if the hotspot is embedded into a custom launcher unless 
> the launcher actively cooperates. Just creating and invoking the JVM is not 
> enough, it needs to do some steps prior to loading the hotspot. This 
> limitation is not well known (nor, do I believe, documented). Many products 
> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this 
> problem limits NMT usefulness greatly since our VMs are often embedded into 
> custom launchers and modifying every launcher is impossible.
> - Worse, if that custom launcher links the libjvm *statically* there is just 
> no way to activate NMT at all. This is the reason NMT cannot be used in the 
> `gtestlauncher`.
> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` 
> and `-XX:Flags=`.
> - The fact that NMT cannot be used in gtests is really a pity since it would 
> allow us to both test NMT itself more rigorously and check for memory leaks 
> while testing other stuff.
> 
> The reason for all this is that NMT initialization happens very early, on the 
> first call to `os::malloc()`. And those calls happen already during dynamic 
> C++ initialization - a long time before the VM gets around parsing arguments. 
> So, regular VM argument parsing is too late to parse NMT arguments.
> 
> The current solution is to pass NMT arguments via a specially prepared 
> environment variable: `NMT_LEVEL_=`. That environment 
> variable has to be set by the embedding launcher, before it loads the libjvm. 
> Since its name contains the PID, we cannot even set that variable in the 
> shell before starting the launcher.
> 
> All that means that every launcher needs to especially parse and process the 
> NMT arguments given at the command line (or via whatever method) and prepare 
> the environment variable. `java` itself does this. This only works before the 
> libjvm.so is loaded, before its dynamic C++ initialization. For that reason, 
> it does not work if the launcher links statically against the hotspot, since 
> in that case C++ initialization of the launcher and hotspot are folded into 
> one phase with no possibility of executing code beforehand.
> 
> And since it bypasses argument handling in the VM, it bypasses a number of 
> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`.
> 
> --
> 
> This patch fixes these shortcomings by making NMT late-initializable: it can 
> now be initialized after normal VM argument parsing, like all other parts of 
> the VM. This greatly simplifies NMT initialization and makes it work 
> automagically for every third party launcher, as well as within our gtests.
> 
> The glaring problem with late-initializing NMT is the NMT malloc headers. If 
> we rule out just always having them (unacceptable in terms of memory 
> overhead), there is no safe way to determine, in os::free(), if an allocation 
> came from before or after NMT initialization ran, and therefore what to do 
> with its malloc headers. For a more extensive explanation, please see the 
> comment block `nmtPreInit.hpp` and the discussion with @kimbarrett and 
> @zhengyu123 in the JBS comment section.
> 
> The heart of this patch is a new way to track early, pre-NMT-init 
> allocations. These are tracked via a lookup table. This was a suggestion by 
> Kim and it worked out well.
> 
> Changes in detail:
> 
> - pre-NMT-init handling:
>   - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init 
> handling. They contain a small global lookup table managing C-heap blocks 
> allocated in the pre-NMT-init phase.
>   - `os::malloc()/os::realloc()/os::free()` defer to this code before 
> doing anything else.
>   - Please see the extensive comment block at the start of 
> `nmtPreinit.hpp` explaining the details.
> 
> - Changes to NMT:
>   - Before, NMT initialization was spread over two phases, `initialize()` 
> and `late_initialize()`. Those were merged into one and simplified - there is 
> only one initialization now which happens after argument parsing.
>   - Minor changes were needed for the `NMT_TrackingLevel` enum - to 
> simplify code, I changed NMT_unknown to be numerically 0. A new comment block 
> in `nmtCommon.hpp` now clearly specifies what's what, including allowed level 
> state transitions.
>   - New utility functions to translate tracking level from/to strings 
> added to `NMTUtil`
>   - NMT has never been able to handle virtual memory allocations before 
> initialization, which is fine since os::reserve_memory() is not 

Re: RFR: 8254598: StringDedupTable should use OopStorage [v4]

[Zhengyu Gu]

On Fri, 14 May 2021 04:31:59 GMT, Kim Barrett  wrote:

>> Please review this change to the String Deduplication facility.  It is being
>> changed to use OopStorage to hold weak references to relevant objects,
>> rather than bespoke data structures requiring dedicated processing phases by
>> supporting GCs.
>> 
>> (The Shenandoah update was contributed by Zhengyu Gu.)
>> 
>> This change significantly simplifies the interface between a given GC and
>> the String Deduplication facility, which should make it much easier for
>> other GCs to opt in.  However, this change does not alter the set of GCs
>> providing support; currently only G1 and Shenandoah support string
>> deduplication.  Adding support by other GCs will be in followup RFEs.
>> 
>> Reviewing via the diffs might not be all that useful for some parts, as
>> several files have been essentially completely replaced, and a number of
>> files have been added or eliminated.  The full webrev might be a better
>> place to look.
>> 
>> The major changes are in gc/shared/stringdedup/* and in the supporting
>> collectors, but there are also some smaller changes in other places, most
>> notably classfile/{stringTable,javaClasses}.
>> 
>> This change is additionally labeled for review by core-libs, although there
>> are no source changes there.  This change injects a byte field of bits into
>> java.lang.String, using one of the previously unused padding bytes in that
>> class.  (There were two unused bytes, now only one.)
>> 
>> Testing:
>> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>> 
>> Locally (linux-x64) ran all of the existing tests that use string
>> deduplication with both G1 and Shenandoah.  Note that
>> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
>> currently fails, for reasons I haven't figured out but suspect are test
>> related.
>> 
>> - gc/stringdedup/   -- these used to be in gc/g1
>> - runtime/cds/SharedStringsDedup.java
>> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
>> - runtime/cds/appcds/sharedStrings/*
>> 
>> shenandoah-only:
>> - gc/shenandoah/jvmti/TestHeapDump.java
>> - gc/shenandoah/TestStringDedup.java
>> - gc/shenandoah/TestStringDedupStress.java
>> 
>> Performance tested baseline, baseline + stringdedup, new with stringdedup,
>> finding no significant differences.
>
> Kim Barrett has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into new_dedup2
>  - misc cleanups
>  - refactor Requests::add
>  - fix typo
>  - more comment improvements
>  - improve naming and comments around injected String flags
>  - fix some typos in comments
>  - New string deduplication

> The "merge from master" commit 
> ([ccb9951](https://github.com/openjdk/jdk/commit/ccb99515d020785d7fe1cf9a1c247aeed775dc19))
>  doesn't build with Shenandoah. I've asked Zhengyu to take a look.

Just missing a parameter:

```diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp 
b/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
index ddaa66ccc14..93a067fa22d 100644
--- a/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
+++ b/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
@@ -57,7 +57,7 @@ 
ShenandoahInitMarkRootsClosure::ShenandoahInitMarkRootsClosure(ShenandoahObjToSc
 
 template 
 void ShenandoahInitMarkRootsClosure::do_oop_work(T* p) {
-  ShenandoahMark::mark_through_ref(p, _queue, _mark_context, 
false);
+  ShenandoahMark::mark_through_ref(p, _queue, _mark_context, 
NULL, false);
 }```

src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:

> 312:   size_t _bucket_index;
> 313:   size_t _shrink_index;
> 314:   bool _grow_only;

Indentation

-

PR: https://git.openjdk.java.net/jdk/pull/3662

Re: RFR: 8254598: StringDedupTable should use OopStorage

[Zhengyu Gu]

On Fri, 23 Apr 2021 19:48:47 GMT, Kim Barrett  wrote:

> Please review this change to the String Deduplication facility.  It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
> 
> (The Shenandoah update was contributed by Zhengyu Gu.)
> 
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in.  However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication.  Adding support by other GCs will be in followup RFEs.
> 
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated.  The full webrev might be a better
> place to look.
> 
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
> 
> This change is additionally labeled for review by core-libs, although there
> are no source changes there.  This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class.  (There were two unused bytes, now only one.)
> 
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
> 
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah.  Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
> 
> - gc/stringdedup/   -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
> 
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
> 
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.

Just FYI:
Concurrent GC, such as Sheanndoah and ZGC (if it decides to implement string 
deduplication in the future), can not enqueue candidates during concurrent 
thread root scanning, because of potential lock rank inversion between 
OopStorage lock and stack watermark lock.

src/hotspot/share/classfile/stringTable.cpp line 355:

> 353:   // Notify deduplication support that the string is being interned.  A 
> string
> 354:   // must never be deduplicated after it has been interned.  Doing so 
> interferes
> 355:   // with compiler optimizations don on e.g. interned string literals.

typo: don -> done

-

PR: https://git.openjdk.java.net/jdk/pull/3662

Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v3]

[Zhengyu Gu]

On Wed, 20 Jan 2021 01:47:54 GMT, Alex Menkov  wrote:

>> The fix adds NMT handling for non-java launchers
>
> Alex Menkov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated comment

Sorry, I overlooked some of details. Final change looks fine to me.

-

PR: https://git.openjdk.java.net/jdk/pull/2106

Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly

[Zhengyu Gu]

On Fri, 15 Jan 2021 23:50:16 GMT, Alex Menkov  wrote:

> The fix adds NMT handling for non-java launchers

Looks good

-

Marked as reviewed by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2106

Re: RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Zhengyu Gu]

On 06/05/2018 12:10 PM, Thomas Stüfe wrote:> On Tue, Jun 5, 2018 at 3:46 
PM, Adam Farley8  wrote:

Hi All,

Native memory allocation for DBBs is tracked in java.nio.Bits, but that
only includes what the user thinks they are allocating.



Which is exactly what I would expect as a user...



I agree with Thomas, there is no point for a user to aware of tracking 
overhead, and the overhead only incurs when native memory tracking is 
on. As a matter of fact, it can really confuse user that values can be 
varied, depending on whether native memory tracking is on.


Thanks,

-Zhengyu



When the VM adds extra memory to the allocation amount this extra bit is
not represented in the Bits total. A cursory glance
shows, minimum, that we round the requested memory quantity up to the heap
word size in the Unsafe.allocateMemory code


which I do not understand either - why do we do this? After all,
normal allocations from inside hotspot do not get aligned up in size,
and the java doc to Unsafe allocateMemory does not state anything
about the size being aligned.

In addition to questioning the align up of the user requested size, I
would be in favor of adding a new NMT tag for these, maybe "mtUnsafe"?
That would be an easy fix.


, and
something to do with nmt_header_size in os:malloc() (os.cpp) too.


That is mighty unspecific and also wrong. The align-up mentioned above
goes into the size reported by Bits; the nmt header size does not.



On its own, and in small quantities, align_up(sz, HeapWordSize) isn't that
big of an issue. But when you allocate a lot of DBBs,
and coupled with the nmt_header_size business, it makes the Bits values
wrong. The more DBB allocations, the more inaccurate those
numbers will be.


To be annoyingly precise, it will never be more wrong than 1:7 on
64bit machines :) - if all memory requested via Unsafe.allocateMemory
would be of size 1 byte.



To get the "+X", it seems to me that the best option would be to introduce
an native method in Bits that fetches "X" directly
from Hotspot, using the same code that Hotspot uses (so we'd have to
abstract-out the Hotspot logic that adds X to the memory
quantity). This way, anyone modifying the Hotspot logic won't risk
rendering the Bits logic wrong again.


I don't follow that.



That's only one way to fix the accuracy problem here though. Suggestions
welcome.


You are throwing two effects together:

- As mentioned above, I consider the align-up of the user requested
size to be at least questionable. It shows up as user size in NMT
which should not be. I also fail to see a compelling reason for it,
but maybe someone else can enlighten me.

- But anything else - NMT headers, overwriter guards, etc added by the
VM I consider in the same class as any other overhead incurred e.g. by
the CRT or the OS when calling malloc (e.g. malloc allocator bucket
size). Basically, rss will go up by more than size requested by
malloc. Something maybe worth noting, but IMHO not as part of the
numbers returned by java.nio.Bits.

Just my 2 cents.

Best Regards, Thomas



Best Regards

Adam Farley
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Zhengyu Gu]

Think of it as an NMT upgrade.

Here's an example of what the output should look like:

https://developer.ibm.com/answers/questions/288697/why-
does-nativememinfo-in-javacore-show-incorrect.html?sort=oldest

- Adam



I think NMT walks the stack, so we should get allocation points grouped by
call stacks. Provided we have symbols loaded for the native library using
Unsafe.allocateMemory(), this should give us too a fine granularity. But I
have not yet tested this in practice. Maybe Zhengyu knows more.


Quick test shows this call site:

[0x7f8558b26243] Unsafe_AllocateMemory0+0x93
[0x7f8537b085cb]
 (malloc=2KB type=Internal #1)

I will take a look why there is a frame not decoded.

Thanks,

-Zhengyu





..Thomas





David

On 14/02/2018 9:32 PM, Adam Farley8 wrote:

Hi All,

Currently, diagnostic core files generated from OpenJDK seem to lump

all

of the
native memory usages together, making it near-impossible for someone

to

figure
out *what* is using all that memory in the event of a memory leak.

The OpenJ9 VM has a feature which allows it to track the allocation of
native
memory for Direct Byte Buffers (DBBs), and to supply that information
into
the
cores when they are generated. This makes it a *lot* easier to find

out

what is using
all that native memory, making memory leak resolution less like some

dark

art, and
more like logical debugging.

To use this feature, there is a native method referenced in

Unsafe.java.

To open
up this feature so that any VM can make use of it, the java code below
sets the
stage for it. This change starts letting people call DBB-specific

methods

when
allocating native memory, and getting into the habit of using it.

Thoughts?

Best Regards

Adam Farley

P.S. Code:

diff --git
a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
---

a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template

+++

b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template

@@ -85,7 +85,7 @@
   // Paranoia
   return;
   }
-UNSAFE.freeMemory(address);
+UNSAFE.freeDBBMemory(address);
   address = 0;
   Bits.unreserveMemory(size, capacity);
   }
@@ -118,7 +118,7 @@
   long base = 0;
   try {
-base = UNSAFE.allocateMemory(size);
+base = UNSAFE.allocateDBBMemory(size);
   } catch (OutOfMemoryError x) {
   Bits.unreserveMemory(size, cap);
   throw x;
diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -632,6 +632,26 @@
   }
   /**
+ * Allocates a new block of native memory for DirectByteBuffers,

of

the
+ * given size in bytes.  The contents of the memory are
uninitialized;
+ * they will generally be garbage.  The resulting native pointer
will
+ * never be zero, and will be aligned for all value types.

Dispose

of
+ * this memory by calling {@link #freeDBBMemory} or resize it

with

+ * {@link #reallocateDBBMemory}.
+ *
+ * @throws RuntimeException if the size is negative or too large
+ *  for the native size_t type
+ *
+ * @throws OutOfMemoryError if the allocation is refused by the
system
+ *
+ * @see #getByte(long)
+ * @see #putByte(long, byte)
+ */
+public long allocateDBBMemory(long bytes) {
+return allocateMemory(bytes);
+}
+
+/**
* Resizes a new block of native memory, to the given size in
bytes.
The
* contents of the new block past the size of the old block are
* uninitialized; they will generally be garbage.  The resulting
native
@@ -687,6 +707,27 @@
   }
   /**
+ * Resizes a new block of native memory for DirectByteBuffers, to



the
+ * given size in bytes.  The contents of the new block past the

size

of
+ * the old block are uninitialized; they will generally be

garbage.

The
+ * resulting native pointer will be zero if and only if the
requested
size
+ * is zero.  The resulting native pointer will be aligned for all
value
+ * types.  Dispose of this memory by calling {@link

#freeDBBMemory},

or
+ * resize it with {@link #reallocateDBBMemory}.  The address

passed

to
+ * this method may be null, in which case an allocation will be
performed.
+ *
+ * @throws RuntimeException if the size is negative or too large
+ *  for the native size_t type
+ *
+ * @throws OutOfMemoryError if the allocation is refused by the
system
+ *
+ * @see #allocateDBBMemory
+ */
+public long reallocateDBBMemory(long address, long bytes) {
+

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Zhengyu Gu]

On 02/14/2018 08:16 AM, Thomas Stüfe wrote:

On Wed, Feb 14, 2018 at 1:53 PM, David Holmes 
wrote:


On 14/02/2018 10:43 PM, David Holmes wrote:


Adding in core-libs-dev as there's nothing related to hotspot directly
here.



Correction, this is of course leading to a proposed change in hotspot to
implement the new Unsafe methods and perform the native memory tracking. Of
course we already have NMT so the obvious question is how this will fit in
with NMT?



I thought Unsafe.allocateMemory is served by hotspot os::malloc(), is it
not? So, allocations should show up in NMT with "Unsafe_AllocateMemory0".


Could use another category? to make it earlier to identify.

Thanks,

-Zhengyu



..Thomas




David


David


On 14/02/2018 9:32 PM, Adam Farley8 wrote:


Hi All,

Currently, diagnostic core files generated from OpenJDK seem to lump all
of the
native memory usages together, making it near-impossible for someone to
figure
out *what* is using all that memory in the event of a memory leak.

The OpenJ9 VM has a feature which allows it to track the allocation of
native
memory for Direct Byte Buffers (DBBs), and to supply that information
into
the
cores when they are generated. This makes it a *lot* easier to find out
what is using
all that native memory, making memory leak resolution less like some dark
art, and
more like logical debugging.

To use this feature, there is a native method referenced in Unsafe.java.
To open
up this feature so that any VM can make use of it, the java code below
sets the
stage for it. This change starts letting people call DBB-specific methods
when
allocating native memory, and getting into the habit of using it.

Thoughts?

Best Regards

Adam Farley

P.S. Code:

diff --git
a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
--- a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
+++ b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
@@ -85,7 +85,7 @@
   // Paranoia
   return;
   }
-UNSAFE.freeMemory(address);
+UNSAFE.freeDBBMemory(address);
   address = 0;
   Bits.unreserveMemory(size, capacity);
   }
@@ -118,7 +118,7 @@
   long base = 0;
   try {
-base = UNSAFE.allocateMemory(size);
+base = UNSAFE.allocateDBBMemory(size);
   } catch (OutOfMemoryError x) {
   Bits.unreserveMemory(size, cap);
   throw x;
diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -632,6 +632,26 @@
   }
   /**
+ * Allocates a new block of native memory for DirectByteBuffers, of
the
+ * given size in bytes.  The contents of the memory are
uninitialized;
+ * they will generally be garbage.  The resulting native pointer
will
+ * never be zero, and will be aligned for all value types.  Dispose
of
+ * this memory by calling {@link #freeDBBMemory} or resize it with
+ * {@link #reallocateDBBMemory}.
+ *
+ * @throws RuntimeException if the size is negative or too large
+ *  for the native size_t type
+ *
+ * @throws OutOfMemoryError if the allocation is refused by the
system
+ *
+ * @see #getByte(long)
+ * @see #putByte(long, byte)
+ */
+public long allocateDBBMemory(long bytes) {
+return allocateMemory(bytes);
+}
+
+/**
* Resizes a new block of native memory, to the given size in
bytes.
The
* contents of the new block past the size of the old block are
* uninitialized; they will generally be garbage.  The resulting
native
@@ -687,6 +707,27 @@
   }
   /**
+ * Resizes a new block of native memory for DirectByteBuffers, to
the
+ * given size in bytes.  The contents of the new block past the size
of
+ * the old block are uninitialized; they will generally be garbage.
The
+ * resulting native pointer will be zero if and only if the
requested
size
+ * is zero.  The resulting native pointer will be aligned for all
value
+ * types.  Dispose of this memory by calling {@link #freeDBBMemory},
or
+ * resize it with {@link #reallocateDBBMemory}.  The address passed
to
+ * this method may be null, in which case an allocation will be
performed.
+ *
+ * @throws RuntimeException if the size is negative or too large
+ *  for the native size_t type
+ *
+ * @throws OutOfMemoryError if the allocation is refused by the
system
+ *
+ * @see #allocateDBBMemory
+ */
+public long reallocateDBBMemory(long address, long bytes) {
+return reallocateMemory(address, bytes);
+}
+
+ 

Re: Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement

[Zhengyu Gu]

Hi Neil,

There is still an issue. Apparently, you can NOT free the buffer for the 
environment variable.


678 char * pbuf = (char*)JLI_MemAlloc(pbuflen);
 679 JLI_Snprintf(pbuf, pbuflen, %s%d=%s, NMT_Env_Name, 
JLI_GetPid(), value);
 680 retval = JLI_PutEnv(pbuf);
 681 if (JLI_IsTraceLauncher()) {
 682 char* envName;
 683 char* envBuf;
 684
 685 // ensures that malloc successful
 686 envName = (char*)JLI_MemAlloc(pbuflen);
 687 JLI_Snprintf(envName, pbuflen, %s%d, NMT_Env_Name, 
JLI_GetPid());
 688
 689 printf(TRACER_MARKER: NativeMemoryTracking: env var is 
%s\n,envName);
 690 printf(TRACER_MARKER: NativeMemoryTracking: putenv arg 
%s\n,pbuf);
 691 envBuf = getenv(envName);
 692 printf(TRACER_MARKER: NativeMemoryTracking: got value 
%s\n,envBuf);
 693 free(envName);
 694 }
 695
 696 free(pbuf);=== can not free this buffer
 697 }


You can experiment it move #696 to #681, you will see the test to fail.

Linux putenv document says:

*putenv*is very widely available, but it might or might not copy its 
argument, risking memory leaks.



Thanks,

-Zhengyu


On 6/25/2014 4:40 PM, Neil Toda wrote:


Okay, Very glad you checked.  It really does need to go as early as 
your prototype suggested.

I'll get right on it.

-neil

On 6/25/2014 12:05 PM, Zhengyu Gu wrote:

Hi Neil,

I tried out this patch with my hotspot, it does not work.

The reason is that, the environment variable is setup too late, it 
has to be set before it launches JavaVM (before calling LoadJavaVM()) 
method.


Thanks,

-Zhengyu

On 6/25/2014 1:58 PM, Neil Toda wrote:

Sorry.  One more webrev .. 06

http://cr.openjdk.java.net/~ntoda/8042469/webrev-06/

Kumar's nit was correct and in fact index was old and should have 
been removed
allowing .contains member to be used instead of .indexOf.  So 
cleaned up a bit more

as can be seen below.  Other of Kumar's nits done.

Thanks Kumar.

webrev-06
  102 // get the PID from the env var we set for the JVM
  103 String envVarPid = null;
  104 for (String line : tr.testOutput) {
  105 if (line.contains(envVarPidString)) {
  106 int sindex = envVarPidString.length();
  107 envVarPid = line.substring(sindex);
  108 break;
  109 }
  110 }
  111 // did we find envVarPid?
  112 if (envVarPid == null) {
  113 System.out.println(tr);
  114 throw new RuntimeException(Error: failed to find env Var Pid 
in tracking info);
  115 }


webrev-05
  102 // get the PID from the env var we set for the JVM
  103 haveLauncherPid = false;
  104 String envVarPid = null;
  105 for (String line : tr.testOutput) {
  106 int index;
  107 if ((index = line.indexOf(envVarPidString)) = 0) {
  108 int sindex = envVarPidString.length();
  109 envVarPid = line.substring(sindex);
  110 haveLauncherPid = true;
  111 break;
  112 }
  113 }
  114 // did we find envVarPid?
  115 if (!haveLauncherPid) {
  116 System.out.println(tr);
  117 throw new RuntimeException(Error: failed to find env Var Pid 
in tracking info);
  118 }


On 6/24/2014 2:28 PM, Neil Toda wrote:


New webrev-05 with Kumar's comments except for the C'ish style. 
Explained below.


http://cr.openjdk.java.net/~ntoda/8042469/webrev-05/

105 : DONE
146: DONE
168: DONE

106: Your suggestion was the way I had originally coded it for 
Linux.  However
 on Windows/Cygwin, the code will not compile  There is 
some interaction
 with the doExec() preceeding it and the fact that it is a 
varlist.  This coding

 was the simplest workaround.

Thanks for the nits Kumar.


On 6/24/2014 5:36 AM, Kumar Srinivasan wrote:

Neil,

Some nits:

TestSpecialArgs.java:

extra space
105 for ( String line : tr.testOutput) {



This is very C'ish, I suggest.
 -106 int index;
 -107 if ((index = line.indexOf(envVarPidString)) 
= 0) {


 +106 int index = line.indexOf(envVarPidString);
 +107 if (index = 0) {


Needs space
-146 launcherPid = line.substring(sindex,tindex);
+146 launcherPid = line.substring(sindex, tindex);

Needs space

-168 if (!tr.contains(NativeMemoryTracking: got value 
+NMT_Option_Value)) {
+168 if (!tr.contains(NativeMemoryTracking: got value 
 + NMT_Option_Value)) {



Thanks
Kumar


On 6/23/2014 10:26 AM

Re: Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement

[Zhengyu Gu]

The possible values are: off, summary and detail

Thanks,

-Zhengyu

On 6/26/2014 3:58 PM, Neil Toda wrote:


You are right.  I found this upon looking:

https://www.securecoding.cert.org/confluence/display/seccode/POS34-C.+Do+not+call+putenv()+with+a+pointer+to+an+automatic+variable+as+the+argument

We moved to malloc since the launcher doesn't know about the value 
being passed
with the native memory flag.  We can put some limits on value, but 
that means

the launcher making a few more decisions.  We'll think on this a bit.

What are the possible values for
-XX:NativeMemoryTracking
For the future, are there any other possibilities?

Thanks

-neil

On 6/26/2014 10:57 AM, Zhengyu Gu wrote:

Hi Neil,

There is still an issue. Apparently, you can NOT free the buffer for 
the environment variable.


678 char * pbuf = (char*)JLI_MemAlloc(pbuflen);
  679 JLI_Snprintf(pbuf, pbuflen, %s%d=%s, NMT_Env_Name, 
JLI_GetPid(), value);
  680 retval = JLI_PutEnv(pbuf);
  681 if (JLI_IsTraceLauncher()) {
  682 char* envName;
  683 char* envBuf;
  684
  685 // ensures that malloc successful
  686 envName = (char*)JLI_MemAlloc(pbuflen);
  687 JLI_Snprintf(envName, pbuflen, %s%d, NMT_Env_Name, 
JLI_GetPid());
  688
  689 printf(TRACER_MARKER: NativeMemoryTracking: env var is 
%s\n,envName);
  690 printf(TRACER_MARKER: NativeMemoryTracking: putenv arg 
%s\n,pbuf);
  691 envBuf = getenv(envName);
  692 printf(TRACER_MARKER: NativeMemoryTracking: got value 
%s\n,envBuf);
  693 free(envName);
  694 }
  695
  696 free(pbuf);=== can not free this buffer
  697 }

You can experiment it move #696 to #681, you will see the test to fail.

Linux putenv document says:

*putenv*is very widely available, but it might or might not copy its 
argument, risking memory leaks.



Thanks,

-Zhengyu


On 6/25/2014 4:40 PM, Neil Toda wrote:


Okay, Very glad you checked.  It really does need to go as early as 
your prototype suggested.

I'll get right on it.

-neil

On 6/25/2014 12:05 PM, Zhengyu Gu wrote:

Hi Neil,

I tried out this patch with my hotspot, it does not work.

The reason is that, the environment variable is setup too late, it 
has to be set before it launches JavaVM (before calling 
LoadJavaVM()) method.


Thanks,

-Zhengyu

On 6/25/2014 1:58 PM, Neil Toda wrote:

Sorry.  One more webrev .. 06

http://cr.openjdk.java.net/~ntoda/8042469/webrev-06/

Kumar's nit was correct and in fact index was old and should have 
been removed
allowing .contains member to be used instead of .indexOf. So 
cleaned up a bit more

as can be seen below.  Other of Kumar's nits done.

Thanks Kumar.

webrev-06
  102 // get the PID from the env var we set for the JVM
  103 String envVarPid = null;
  104 for (String line : tr.testOutput) {
  105 if (line.contains(envVarPidString)) {
  106 int sindex = envVarPidString.length();
  107 envVarPid = line.substring(sindex);
  108 break;
  109 }
  110 }
  111 // did we find envVarPid?
  112 if (envVarPid == null) {
  113 System.out.println(tr);
  114 throw new RuntimeException(Error: failed to find env Var Pid 
in tracking info);
  115 }


webrev-05
  102 // get the PID from the env var we set for the JVM
  103 haveLauncherPid = false;
  104 String envVarPid = null;
  105 for (String line : tr.testOutput) {
  106 int index;
  107 if ((index = line.indexOf(envVarPidString)) = 0) {
  108 int sindex = envVarPidString.length();
  109 envVarPid = line.substring(sindex);
  110 haveLauncherPid = true;
  111 break;
  112 }
  113 }
  114 // did we find envVarPid?
  115 if (!haveLauncherPid) {
  116 System.out.println(tr);
  117 throw new RuntimeException(Error: failed to find env Var Pid 
in tracking info);
  118 }


On 6/24/2014 2:28 PM, Neil Toda wrote:


New webrev-05 with Kumar's comments except for the C'ish style. 
Explained below.


http://cr.openjdk.java.net/~ntoda/8042469/webrev-05/

105 : DONE
146: DONE
168: DONE

106: Your suggestion was the way I had originally coded it for 
Linux.  However
 on Windows/Cygwin, the code will not compile There is 
some interaction
 with the doExec() preceeding it and the fact that it is 
a varlist.  This coding

 was the simplest workaround.

Thanks for the nits Kumar.


On 6/24/2014 5:36 AM

Re: Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement

[Zhengyu Gu]

Hi Neil,

I tried out this patch with my hotspot, it does not work.

The reason is that, the environment variable is setup too late, it has 
to be set before it launches JavaVM (before calling LoadJavaVM()) method.


Thanks,

-Zhengyu

On 6/25/2014 1:58 PM, Neil Toda wrote:

Sorry.  One more webrev .. 06

http://cr.openjdk.java.net/~ntoda/8042469/webrev-06/

Kumar's nit was correct and in fact index was old and should have been 
removed
allowing .contains member to be used instead of .indexOf.  So cleaned 
up a bit more

as can be seen below.  Other of Kumar's nits done.

Thanks Kumar.

webrev-06
  102 // get the PID from the env var we set for the JVM
  103 String envVarPid = null;
  104 for (String line : tr.testOutput) {
  105 if (line.contains(envVarPidString)) {
  106 int sindex = envVarPidString.length();
  107 envVarPid = line.substring(sindex);
  108 break;
  109 }
  110 }
  111 // did we find envVarPid?
  112 if (envVarPid == null) {
  113 System.out.println(tr);
  114 throw new RuntimeException(Error: failed to find env Var Pid 
in tracking info);
  115 }


webrev-05
  102 // get the PID from the env var we set for the JVM
  103 haveLauncherPid = false;
  104 String envVarPid = null;
  105 for (String line : tr.testOutput) {
  106 int index;
  107 if ((index = line.indexOf(envVarPidString)) = 0) {
  108 int sindex = envVarPidString.length();
  109 envVarPid = line.substring(sindex);
  110 haveLauncherPid = true;
  111 break;
  112 }
  113 }
  114 // did we find envVarPid?
  115 if (!haveLauncherPid) {
  116 System.out.println(tr);
  117 throw new RuntimeException(Error: failed to find env Var Pid 
in tracking info);
  118 }


On 6/24/2014 2:28 PM, Neil Toda wrote:


New webrev-05 with Kumar's comments except for the C'ish style. 
Explained below.


http://cr.openjdk.java.net/~ntoda/8042469/webrev-05/

105 : DONE
146: DONE
168: DONE

106: Your suggestion was the way I had originally coded it for 
Linux.  However
 on Windows/Cygwin, the code will not compile  There is some 
interaction
 with the doExec() preceeding it and the fact that it is a 
varlist.  This coding

 was the simplest workaround.

Thanks for the nits Kumar.


On 6/24/2014 5:36 AM, Kumar Srinivasan wrote:

Neil,

Some nits:

TestSpecialArgs.java:

extra space
105 for ( String line : tr.testOutput) {



This is very C'ish, I suggest.
 -106 int index;
 -107 if ((index = line.indexOf(envVarPidString)) = 
0) {


 +106 int index = line.indexOf(envVarPidString);
 +107 if (index = 0) {


Needs space
-146 launcherPid = line.substring(sindex,tindex);
+146 launcherPid = line.substring(sindex, tindex);

Needs space

-168 if (!tr.contains(NativeMemoryTracking: got value 
+NMT_Option_Value)) {
+168 if (!tr.contains(NativeMemoryTracking: got value  
+ NMT_Option_Value)) {



Thanks
Kumar


On 6/23/2014 10:26 AM, Neil Toda wrote:


I've spun a new webrev based on the comments for webrev-03:

http://cr.openjdk.java.net/~ntoda/8042469/webrev-04/

Changes are:

  1) java.c
a) add free(envName) line 1063
b) change from malloc() to JLI_MemAlloc() @ lines 1048 and 
1056


Thanks

-neil

On 6/20/2014 4:45 PM, Kumar Srinivasan wrote:

Neil,

Generally looks good, yes JLI_* functions must be used, I missed 
that one.

Are you going to post another iteration ?

Kumar

On 6/20/2014 4:27 PM, Neil Toda wrote:


Thanks Joe.  It would have checked for NULL for me.
I'll use the JLI wrapper.

-neil

On 6/20/2014 4:04 PM, Joe Darcy wrote:
Memory allocation in the launcher should use one of the 
JLI_MemAlloc wrappers, if possible.


-Joe


On 06/20/2014 09:50 AM, Neil Toda wrote:


They should complain.  Thanks Zhengyu.  I'll make sure these 
are non-null.


-neil

On 6/20/2014 5:01 AM, Zhengyu Gu wrote:

Neil,

Thanks for quick implementation.

java.c:
  Did not check return values of malloc(), I wonder if source 
code analyzers will complain.


-Zhengyu

On 6/19/2014 8:29 PM, Neil Toda wrote:


Launcher support for modified Native Memory Tracking 
mechanism in JVM in JDK9.


Webrev  : http://cr.openjdk.java.net/~ntoda/8042469/webrev-03/
bug : https://bugs.openjdk.java.net/browse/JDK-8042469
CCC : http://ccc.us.oracle.com/8042469

Thanks.

-neil

Re: Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement

[Zhengyu Gu]

Neil,

Thanks for quick implementation.

java.c:
  Did not check return values of malloc(), I wonder if source code 
analyzers will complain.


-Zhengyu

On 6/19/2014 8:29 PM, Neil Toda wrote:


Launcher support for modified Native Memory Tracking mechanism in JVM 
in JDK9.


Webrev  : http://cr.openjdk.java.net/~ntoda/8042469/webrev-03/
bug : https://bugs.openjdk.java.net/browse/JDK-8042469
CCC : http://ccc.us.oracle.com/8042469

Thanks.

-neil

hg: jdk8/tl/jdk: 8006691: Remove jvm_version_info-is_kernel_jvm field

[zhengyu . gu]

Changeset: cd111064d4e9
Author:zgu
Date:  2013-02-12 14:47 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cd111064d4e9

8006691: Remove jvm_version_info-is_kernel_jvm field
Summary: Remove is_kernel_jvm field in jvm_version_info structure, as kernel VM 
has been deprecated
Reviewed-by: mchung

! src/share/javavm/export/jvm.h

Code review request: 8006691: Remove jvm_version_info.is_kernel_jvm field

[Zhengyu Gu]

Kernel VM has been deprecated since JDK7 and related JVM code has also 
been removed recently.  This change is to remove is_kernel_jvm flag from 
jvm_version_info structure in jvm.h on JDK side.



Bug:
http://bugs.sun.com/view_bug.do?bug_id=8006691

CCC:
http://ccc.us.oracle.com/8006691

JDK7:
http://cr.openjdk.java.net/~zgu/8006691/jdk7/webrev.00/

JDK8:
http://cr.openjdk.java.net/~zgu/8006691/jdk8/webrev.00/

Thanks,

-Zhengyu

hg: jdk7/tl/jdk: 2 new changesets

[zhengyu . gu]

Changeset: 2381e810330b
Author:zgu
Date:  2011-01-20 10:45 -0500
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/2381e810330b

6983248: net/net001 and net/net003 fail on WinXP with JDK7-B108
Summary: Using closesocket to close socket handler to avoid invalid C runtime 
parameter exception.
Reviewed-by: alanb, phh, dcubed, dsamersoff, coleenp, acorn

! src/windows/demo/jvmti/hprof/hprof_md.c

Changeset: d03e47de3a89
Author:zgu
Date:  2011-01-21 11:38 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d03e47de3a89

Merge