Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()

[Claes Redestad]

On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam  wrote:

> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never 
> supported the value `"rw"` since the source code was imported to the openjdk 
> repo more than 15 years ago. In fact HotSpot throws 
> `IllegalArgumentException` when such a mode is specified.
> 
> It's unlikely such a mode will be required for future enhancements. Support 
> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` 
> is the only supported mode.
> 
> I also cleaned up related code in the JDK and HotSpot.
> 
> Testing:
> - Passed tiers 1 and 2
> - Tiers 3, 4, 5 are in progress

Nice cleanup!

(Some stylistic suggestions inline, feel free to ignore)

src/hotspot/os/windows/perfMemory_windows.cpp line 1781:

> 1779: // address space.
> 1780: //
> 1781: void PerfMemory::attach(const char* user, int vmid,

One line?

src/hotspot/share/prims/perf.cpp line 84:

> 82: 
> 83:   // attach to the PerfData memory region for the specified VM
> 84:   PerfMemory::attach(user_utf, vmid,

One line?

src/hotspot/share/runtime/perfMemory.hpp line 146:

> 144: // methods for attaching to and detaching from the PerfData
> 145: // memory segment of another JVM process on the same system.
> 146: static void attach(const char* user, int vmid,

One line?

src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java line 74:

> 72: Integer v = lvmid;
> 73: RemoteVm stub = null;
> 74: StringBuilder sb = new StringBuilder();

Suggestion:

String vmidStr = "local://" + lvmid + "@localhost";

-

Marked as reviewed by redestad (Reviewer).

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

Re: RFR: 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider

[Claes Redestad]

On Tue, 22 Mar 2022 21:19:20 GMT, Kevin Walls  wrote:

> There are no uses of jdk/internal/agent/spi/AgentProvider, since the SNMP 
> agent was removed ( 8071367 ): this class should be removed.  It is not a 
> public interface.
> 
> Remove 
> src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java
> Remove import from 
> src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java
> Remove uses from src/jdk.management.agent/share/classes/module-info.java
> 
> Testing with the test/jdk/sun/management tests looks good.

Marked as reviewed by redestad (Reviewer).

-

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

Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]

[Claes Redestad]

On Thu, 4 Nov 2021 02:15:45 GMT, Ioi Lam  wrote:

>> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs 
>> owned by a specific user. No one uses these APIs, and they don't work anyway.
>> 
>> The current code is very confusing to look at. Since we're likely to change 
>> code in this area for further container support, it's better to clean up the 
>> code now.
>> 
>> - Remove all APIs that take a user name
>> - Also removed PerfDataFile.getFile() methods that are unused
>> - Cleaned up the code that looks up the hsperfdata_xxx files
>>   - Fix comments to explain what's happening
>>   - Avoid using Matcher.reset which is not thread-safe
>>   - Renamed confusing variables such as `userFilter` to make the code more 
>> readable
>>   - LocalVmManager.activeVms() probably doesn't need to be synchronized, but 
>> I kept it anyway to avoid unnecessary risks.
>> 
>> Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers 
>> and have extensive use of the management tools).
>
> Ioi Lam 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 three additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into 
> 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code
>  - @kevinjwalls and @plummercj review - (1) restore 
> PerfDataFile.userDirNamePattern, etc. (2) Fixed comments
>  - 8275185: Remove dead code and clean up jvmstat LocalVmManager

Supporting 1.4.1 hsperfdata seems pointless at this point, so I support a 
removal of that code. It also seems reasonable to do as a follow-up, as @iklam 
suggests, rather than shifting the scope this PR.

-

Marked as reviewed by redestad (Reviewer).

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

Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules

[Claes Redestad]

On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov  wrote:

> This is the continuation of JDK-8233884 and JDK-8271456. This change affects 
> fewer cases so I fix all "java." modules at once.
> 
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
> 
> This could be done more efficiently(up to x20 time faster) with use of 
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
> 
> The later variant also makes the code cleaner, as it is known not to throw 
> UnsupportedEncodingException in contrary to the former variant.
> 
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.

Yes, while I don't know exactly which changes resolved JDK-6764325, it's clear 
from the microbenchmarks added for #2102 that it's no longer an issue - at 
least not in the mainline.

-

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

Re: RFR: 8264087: Use the blessed modifier order in jdk.jconsole

[Claes Redestad]

On Tue, 23 Mar 2021 21:43:47 GMT, Alex Blewitt 
 wrote:

> 8264087: Use the blessed modifier order in jdk.jconsole

Marked as reviewed by redestad (Reviewer).

-

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

Re: RFR: 8263855: Use the blessed modifier order in java.management/naming [v2]

[Claes Redestad]

On Fri, 19 Mar 2021 17:13:58 GMT, Alex Blewitt 
 wrote:

>> As with #2993 changing the order of `final static` to `static final` for the 
>> `java.management`, `java.management.rmi` and `java.naming` modules.
>
> Alex Blewitt has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added more replacements of 'final public' with 'public final'

Marked as reviewed by redestad (Reviewer).

-

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

Re: RFR: 8263855: Use the blessed modifier order in java.management/naming

[Claes Redestad]

On Thu, 18 Mar 2021 18:26:20 GMT, Alex Blewitt 
 wrote:

> As with #2993 changing the order of `final static` to `static final` for the 
> `java.management`, `java.management.rmi` and `java.naming` modules.

Overall good - found a few cases where the private modifier is in the wrong 
place that ought to be fixed as well.

src/java.naming/share/classes/com/sun/jndi/ldap/EventQueue.java line 48:

> 46:  */
> 47: final class EventQueue implements Runnable {
> 48: static final private boolean debug = false;

Move private to the front?

src/java.naming/share/classes/com/sun/jndi/ldap/EventSupport.java line 116:

> 114:  */
> 115: final class EventSupport {
> 116: static final private boolean debug = false;

Move private to the front?

src/java.naming/share/classes/com/sun/jndi/ldap/LdapSchemaCtx.java line 389:

> 387: }
> 388: 
> 389: static final private class SchemaInfo {

Move private to the front?

-

Changes requested by redestad (Reviewer).

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

Re: RFR: 8263855: Use the blessed modifier order in java.management/naming

[Claes Redestad]

On Fri, 19 Mar 2021 13:08:24 GMT, Alex Blewitt 
 wrote:

>> Created the subtask https://bugs.openjdk.java.net/browse/JDK-8263855 for 
>> this along with an umbrella RFE.
>
> Thanks @cl4es -- do I need to update the git commit message as well, or is 
> updating the title of the PR sufficient? I recall you suggesting not to do 
> amend/rebases previously.

No, the git commit messages here doesn't matter, the bots will use the PR name 
when merging into master. It's good form to use reasonably consistent commit 
messages as you add commits to a PR, but altering commit history is not 
necessary and might even be disruptive once a PR has been opened.

There's the /summary command you could use to add additional comments to the 
final commit, see 
https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/summary

-

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

Re: RFR: 8263855: Use the blessed modifier order in java.management/naming

[Claes Redestad]

On Fri, 19 Mar 2021 10:20:39 GMT, Alex Blewitt 
 wrote:

>> Would someone mind creating a bug for me? In addition, if it would help, we 
>> could create a parent bug for applying cleanups  and associate #2993 and 
>> #2991.
>
> @cl4es would you mind creating a parent task of something like "Source code 
> cleanups" and then another sub task for this change please?

Created the subtask https://bugs.openjdk.java.net/browse/JDK-8263855 for this 
along with an umbrella RFE.

-

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

Integrated: 8260448: Simplify ManagementFactory$PlatformMBeanFinder

[Claes Redestad]

On Tue, 26 Jan 2021 19:34:35 GMT, Claes Redestad  wrote:

> Simplify and desugar the static initialization of PlatformMBeanFinder. 
> 
> While arguably making the code easier to comprehend, this saves 5-6ms on 
> startup in various applications such as the spring-petclinic.

This pull request has now been integrated.

Changeset: e696baab
Author:Claes Redestad 
URL:   https://git.openjdk.java.net/jdk/commit/e696baab
Stats: 64 lines in 1 file changed: 21 ins; 26 del; 17 mod

8260448: Simplify ManagementFactory$PlatformMBeanFinder

Reviewed-by: mchung, dfuchs

-

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

Re: RFR: 8260448: Simplify ManagementFactory$PlatformMBeanFinder

[Claes Redestad]

On Tue, 26 Jan 2021 21:26:32 GMT, Mandy Chung  wrote:

>> Simplify and desugar the static initialization of PlatformMBeanFinder. 
>> 
>> While arguably making the code easier to comprehend, this saves 5-6ms on 
>> startup in various applications such as the spring-petclinic.
>
> Looks fine.

@mlchung @dfuch, thank you for reviewing

-

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

RFR: 8260448: Simplify ManagementFactory$PlatformMBeanFinder

[Claes Redestad]

Simplify and desugar the static initialization of PlatformMBeanFinder. 

While arguably making the code easier to comprehend, this saves 5-6ms on 
startup in various applications such as the spring-petclinic.

-

Commit messages:
 - Simplify PlatformMBeanFinder

Changes: https://git.openjdk.java.net/jdk/pull/2244/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2244=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260448
  Stats: 64 lines in 1 file changed: 21 ins; 26 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2244.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2244/head:pull/2244

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

Integrated: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()

[Claes Redestad]

On Tue, 17 Nov 2020 12:53:48 GMT, Claes Redestad  wrote:

> This moves the mirroring of vmSymbols in ciSymbols to a separate file, 
> ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the 
> ciSymbol namespace). In a few places code is moved from .hpp to .cpp to 
> facilitate this.
> 
> With PCH disabled, this reduces total includes from 276949 to 276332

This pull request has now been integrated.

Changeset: f5740561
Author:Claes Redestad 
URL:   https://git.openjdk.java.net/jdk/commit/f5740561
Stats: 183 lines in 32 files changed: 102 ins; 30 del; 51 mod

8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()

Reviewed-by: kvn, iklam

-

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

Re: RFR: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name() [v3]

[Claes Redestad]

> This moves the mirroring of vmSymbols in ciSymbols to a separate file, 
> ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the 
> ciSymbol namespace). In a few places code is moved from .hpp to .cpp to 
> facilitate this.
> 
> With PCH disabled, this reduces total includes from 276949 to 276332

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  ciMethod needs classfile/vmIntrinsics.hpp after 8252505

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1256/files
  - new: https://git.openjdk.java.net/jdk/pull/1256/files/b8c485b9..2144b16f

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

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1256/head:pull/1256

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

Re: RFR: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name() [v2]

[Claes Redestad]

> This moves the mirroring of vmSymbols in ciSymbols to a separate file, 
> ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the 
> ciSymbol namespace). In a few places code is moved from .hpp to .cpp to 
> facilitate this.
> 
> With PCH disabled, this reduces total includes from 276949 to 276332

Claes Redestad has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 13 commits:

 - Merge branch 'master' into ciSymbols
 - @iklam reviews, additional cleanup
 - remote merge
 - Prepare for 1237 changes
 - Adjust includes after vmIntrinsic changes
 - Merge master
 - Outline is_call_site_target to remove include from ciField.hpp
 - Outline is_autobox_cache to remove include from ciField.hpp
 - Merge branch 'master' into ciSymbols
 - Fix bad definition
 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/d0c52651...b8c485b9

-

Changes: https://git.openjdk.java.net/jdk/pull/1256/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1256=01
  Stats: 182 lines in 32 files changed: 101 ins; 30 del; 51 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1256/head:pull/1256

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

RFR: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()

[Claes Redestad]

This moves the mirroring of vmSymbols in ciSymbols to a separate file, 
ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the ciSymbol 
namespace). In a few places code is moved from .hpp to .cpp to facilitate this.

With PCH disabled, this reduces total includes from 276949 to 276332

-

Commit messages:
 - remote merge
 - Prepare for 1237 changes
 - Adjust includes after vmIntrinsic changes
 - Merge master
 - Outline is_call_site_target to remove include from ciField.hpp
 - Outline is_autobox_cache to remove include from ciField.hpp
 - Merge branch 'master' into ciSymbols
 - Fix bad definition
 - Define debug-only sid_ok in .cpp to avoid need to include vmSymbols.hpp in 
ciSymbol.hpp
 - Revert accidental include order swap
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/86b65756...b05957b8

Changes: https://git.openjdk.java.net/jdk/pull/1256/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1256=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256424
  Stats: 174 lines in 32 files changed: 102 ins; 30 del; 42 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1256/head:pull/1256

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

Integrated: 8256741: Reduce footprint of compiler interface data structures

[Claes Redestad]

On Fri, 20 Nov 2020 12:19:48 GMT, Claes Redestad  wrote:

> A few data structure in the ci allocate unconditionally created 
> GrowableArrays out-of-line, have fields that are newer updated/read, or are 
> unnecessarily cached. By cleaning this up we can slightly reduce memory used 
> for JIT compilations while slightly speeding them up.

This pull request has now been integrated.

Changeset: c0689d25
Author:Claes Redestad 
URL:   https://git.openjdk.java.net/jdk/commit/c0689d25
Stats: 226 lines in 9 files changed: 28 ins; 88 del; 110 mod

8256741: Reduce footprint of compiler interface data structures

Reviewed-by: cjplummer, kvn

-

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

Re: RFR: 8256741: Reduce footprint of compiler interface data structures

[Claes Redestad]

On Fri, 20 Nov 2020 22:51:23 GMT, Chris Plummer  wrote:

>> A few data structure in the ci allocate unconditionally created 
>> GrowableArrays out-of-line, have fields that are newer updated/read, or are 
>> unnecessarily cached. By cleaning this up we can slightly reduce memory used 
>> for JIT compilations while slightly speeding them up.
>
> Marked as reviewed by cjplummer (Reviewer).

@plummercj @vnkozlov - thank you for reviewing

-

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

Re: RFR: 8256741: Reduce footprint of compiler interface data structures

[Claes Redestad]

On Fri, 20 Nov 2020 22:21:04 GMT, Chris Plummer  wrote:

>> A few data structure in the ci allocate unconditionally created 
>> GrowableArrays out-of-line, have fields that are newer updated/read, or are 
>> unnecessarily cached. By cleaning this up we can slightly reduce memory used 
>> for JIT compilations while slightly speeding them up.
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ci/ciObjectFactory.java 
> line 82:
> 
>> 80:   public GrowableArray symbols() {
>> 81: Address addr = getAddress().addOffsetTo(symbolsField.getOffset());
>> 82: return GrowableArray.create(addr, ciSymbolConstructor);
> 
> It's unclear to me why these two changes were needed. Don't they produce the 
> same result before and after?

When changing the fields from  `GrowableArray<..>*` to `GrowableArray<..>` I 
looked around and found another use of an embedded `GrowableArray` in 
`InlineTree.java` and took my cues from that. If you're certain these 
approaches are semantically identical I can drop these changes, but as I'm not 
exactly sure how to verify and test this I went the safe(?) route of leaning on 
prior art here.

-

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

RFR: 8256741: Reduce footprint of compiler interface data structures

[Claes Redestad]

A few data structure in the ci allocate unconditionally created GrowableArrays 
out-of-line, have fields that are newer updated/read, or are unnecessarily 
cached. By cleaning this up we can slightly reduce memory used for JIT 
compilations while slightly speeding them up.

-

Commit messages:
 - Copyrights, syntax polish
 - Adjust vmStructs/SA to changes in ciObjectFactory
 - Inline unconditionally created GAs in ciObjectFactory
 - Remove sad assert
 - Remove ciMethod fields cached in ciTypeFlow
 - Reduce footprint of compiler interface data structures

Changes: https://git.openjdk.java.net/jdk/pull/1346/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1346=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256741
  Stats: 226 lines in 9 files changed: 28 ins; 88 del; 110 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1346/head:pull/1346

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

Re: RFR: 8253081: G1 fails on stale objects in archived module graph in Open Archive regions [v5]

[Claes Redestad]

On Tue, 17 Nov 2020 18:20:17 GMT, Thomas Schatzl  wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that changes the way how archive 
>> regions are managed in general and specifically by the G1 collector, fixing 
>> the crashes caused by adding the module graph into the archive in 
>> [JDK-8244778](https://bugs.openjdk.java.net/browse/JDK-8244778)?
>> 
>> Previously before the JDK-8244778 change, archived objects could always be 
>> assumed as live, and so the G1 collector did so, not caring about the 
>> archive region's contents at all. With JDK-8244778 however, archived objects 
>> could die, and keep stale references to objects outside of the archive 
>> regions, which obviously causes crashes when walking these objects.
>> 
>> With this change, open archive region contents are basically handled as any 
>> other objects; to support that, all open archive regions are now reachable 
>> via a single object array root. This hopefully also facilitates 
>> implementation in other collectors.
>> 
>> This allows us to remove quite a bit of special handling in G1 too; the only 
>> difference is that open archive regions will generally not be collected 
>> unless they are completely empty: we do want to profit from the sharing 
>> across VMs as much as possible.
>> 
>> Testing: tier1-5, one or two 6-8 runs
>> 
>> The appcds changes were done by @iklam. These changes are described in this 
>> document: 
>> https://wiki.openjdk.java.net/display/HotSpot/CDS+Archived+Heap+Improvements
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   kbarrett cl4es review

Marked as reviewed by redestad (Reviewer).

-

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

Re: RFR: 8253081: G1 fails on stale objects in archived module graph in Open Archive regions [v4]

[Claes Redestad]

On Tue, 17 Nov 2020 10:55:19 GMT, Thomas Schatzl  wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that changes the way how archive 
>> regions are managed in general and specifically by the G1 collector, fixing 
>> the crashes caused by adding the module graph into the archive in 
>> [JDK-8244778](https://bugs.openjdk.java.net/browse/JDK-8244778)?
>> 
>> Previously before the JDK-8244778 change, archived objects could always be 
>> assumed as live, and so the G1 collector did so, not caring about the 
>> archive region's contents at all. With JDK-8244778 however, archived objects 
>> could die, and keep stale references to objects outside of the archive 
>> regions, which obviously causes crashes when walking these objects.
>> 
>> With this change, open archive region contents are basically handled as any 
>> other objects; to support that, all open archive regions are now reachable 
>> via a single object array root. This hopefully also facilitates 
>> implementation in other collectors.
>> 
>> This allows us to remove quite a bit of special handling in G1 too; the only 
>> difference is that open archive regions will generally not be collected 
>> unless they are completely empty: we do want to profit from the sharing 
>> across VMs as much as possible.
>> 
>> Testing: tier1-5, one or two 6-8 runs
>> 
>> The appcds changes were done by @iklam. These changes are described in this 
>> document: 
>> https://wiki.openjdk.java.net/display/HotSpot/CDS+Archived+Heap+Improvements
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - sjohanss review
>  - Remove code that "activates" dormant objects as now all active objects are 
> reachable via the root object array

Looks good!

I took a sweep through the code and have some nits that you may choose to 
ignore.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 4538:

> 4536:   } else {
> 4537: // We ignore free regions, we'll empty the free list afterwards.
> 4538: assert(hr->is_free(),

Can make this one line

src/hotspot/share/memory/heapShared.cpp line 334:

> 332: }
> 333: 
> 334: // Returns an objArray that contains all the roots of the archived 
> objects

It does..?

src/hotspot/share/memory/heapShared.cpp line 413:

> 411:   int length = _pending_roots != NULL ? _pending_roots->length() : 0;
> 412:   int size = objArrayOopDesc::object_size(length);
> 413:   Klass *k = Universe::objectArrayKlassObj(); // already relocated to 
> point to archived klass

`Klass* k`

-

Marked as reviewed by redestad (Reviewer).

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

Re: RFR: 8256052: Remove unused allocation type from fieldInfo

[Claes Redestad]

On Mon, 9 Nov 2020 15:43:09 GMT, Frederic Parain  wrote:

> Please review this small cleanup code, removing the now unused allocation 
> type from the fieldInfo structure.
> 
> Tested with Mach5, tiers 1 to 3 and locally by running 
> test/hotspot/jtreg/serviceability/sa tests.
> 
> Thank you,
> 
> Fred

Nice cleanup!

src/hotspot/share/classfile/classFileParser.cpp line 1708:

> 1706: 
> 1707: // Remember how many oops we encountered and compute allocation type
> 1708: const FieldAllocationType atype = fac->update(is_static, type);

The returned `FieldAllocationType` is never used at either call-site, so maybe 
the `update` method can be simplified, too? (It seems all `update` does is 
increment a per-type counter, so the name is a bit surprising)

src/hotspot/share/runtime/vmStructs.cpp line 2261:

> 2259:   declare_preprocessor_constant("FIELDINFO_TAG_SIZE", 
> FIELDINFO_TAG_SIZE) \
> 2260:   declare_preprocessor_constant("FIELDINFO_TAG_OFFSET", 
> FIELDINFO_TAG_OFFSET) \
> 2261:   declare_preprocessor_constant("FIELDINFO_TAG_CONTENDED", 
> FIELDINFO_TAG_CONTENDED) \

Not sure it's necessary to add this with no usage?

-

Marked as reviewed by redestad (Reviewer).

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c [v6]

[Claes Redestad]

On Fri, 6 Nov 2020 14:30:14 GMT, Coleen Phillimore  wrote:

>> Apply patch suggested by @cl4es in the bug report.  Passes 
>> linux-x86-open,linux-x64-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug
>>  builds with this patch, and tier1.
>> 
>> thanks,
>> Coleen
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Now this builds.  I hope it's perfect.

Perfect!

-

Marked as reviewed by redestad (Reviewer).

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c [v5]

[Claes Redestad]

On Fri, 6 Nov 2020 13:28:12 GMT, Coleen Phillimore  wrote:

>> Apply patch suggested by @cl4es in the bug report.  Passes 
>> linux-x86-open,linux-x64-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug
>>  builds with this patch, and tier1.
>> 
>> thanks,
>> Coleen
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More.

Marked as reviewed by redestad (Reviewer).

-

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c [v3]

[Claes Redestad]

On Fri, 6 Nov 2020 11:58:04 GMT, Coleen Phillimore  wrote:

>> The concern is when it is less than 100ms.
>
> unsigned millisecs[] = { 2, 20, 200, 1000 };
>   get_time_stamp(millisecs[i], buf, sizeof(buf));
> 
> gets:
> 
> timestamp 06.11.2020 06:56:08.002 EST
> timestamp 06.11.2020 06:56:08.020 EST
> timestamp 06.11.2020 06:56:08.200 EST
> timestamp 06.11.2020 06:56:08.1000 EST
> 
> with
> 
> char tmp[10 + 1];
> snprintf(tmp, sizeof(tmp), "%.3d", millisecs);
> snprintf(tbuf, ltbuf, "%s.%s %s", timestamp_date_time, tmp, 
> timestamp_timezone);
> 
> Is this what you want?

I think you need .3 in both places, otherwise I expect the warning will still 
be there? (We don't need to worry about values of millisecs larger than 999):

char tmp[11 + 1];
snprintf(tmp, sizeof(tmp), "%.3d", millisecs);
snprintf(tbuf, ltbuf, "%s.%.3s %s", timestamp_date_time, tmp, 
timestamp_timezone);

-

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c [v3]

[Claes Redestad]

On Thu, 5 Nov 2020 22:11:44 GMT, David Holmes  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Use Thomas's fix instead.
>
> src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c line 82:
> 
>> 80: "%Z", localtime());
>> 81: // Truncate milliseconds in buffer large enough to hold the
>> 82: // value which is always < 1000
> 
> I initially missed the %d to %s change. Can we augment the comment to say:
> 
> // value which is always < 1000 (and so a maximum of 3 digits for "%.3s")
> 
> Also I'm not clear whether this will result in extra spaces when the ms value 
> is < 100 ?

Right, `snprintf(tmp, sizeof(tmp), "%d", millisecs);` needs to be 
`snprintf(tmp, sizeof(tmp), "%.3d", millisecs);` to pad it out correctly.

-

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c [v3]

[Claes Redestad]

On Thu, 5 Nov 2020 20:30:05 GMT, Coleen Phillimore  wrote:

>> Apply patch suggested by @cl4es in the bug report.  Passes 
>> linux-x86-open,linux-x64-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug
>>  builds with this patch, and tier1.
>> 
>> thanks,
>> Coleen
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use Thomas's fix instead.

Marked as reviewed by redestad (Reviewer).

-

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c

[Claes Redestad]

On Thu, 5 Nov 2020 18:35:52 GMT, Thomas Stuefe  wrote:

> ... so the problem would be that the compiler does not believe us that 
> millisecs will be always <1000. And there is no way to truncate output for 
> numerical format specifiers.

Interesting, and a reasonable explanation. Odd this doesn't get caught by the 
64-bit build

> 
> One solution could be to first print the millisecs to a buffer large enough 
> to hold MAX_INT. And then print that buffer as a string, which can be 
> truncated with precision.
> 
> ```
> char tmp[10 + 1];
> snprintf(tmp, sizeof(tmp), "%d", millisecs);
> snprintf(tbuf, ltbuf, "%s.%.3s %s", timestamp_date_time, tmp, 
> timestamp_timezone);
> ```
> 
> That may be enough to shut the compiler up.

It'd be interesting to see if statically limiting the value printed to be < 
1000 would also convince the compiler, e.g.
`snprintf(tbuf, ltbuf, "%s.%.3d %s", timestamp_date_time, millisecs % 1000, 
timestamp_timezone);`

(I'm not sure a perfect solution is worth our while here, but seeing we've 
gotten this far down the rabbit hole..)

-

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c

[Claes Redestad]

On Thu, 5 Nov 2020 17:26:50 GMT, Chris Plummer  wrote:

> I don't think this is the case. If you assume the arguments are not null 
> terminated, then there is no limit to how long the string could be, where-as 
> the error messages are very specific with the (incorrectly) calculated range 
> of potential overflow.

I must be missing what you're suggesting here?

-

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c

[Claes Redestad]

On Thu, 5 Nov 2020 00:37:15 GMT, Coleen Phillimore  wrote:

> Apply patch suggested by @cl4es in the bug report.  Passes 
> linux-x86-open,linux-x64-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug
>  builds with this patch, and tier1.
> 
> thanks,
> Coleen

While you might want to wait for someone to suggest a better alternative, this 
LGTM. As you say the x86 failures on GH look related to the setup of the build 
image and thus unrelated to this patch.

-

Marked as reviewed by redestad (Reviewer).

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

Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c

[Claes Redestad]

On Thu, 5 Nov 2020 09:12:54 GMT, Aleksey Shipilev  wrote:

>>> Where did the magic numbers in
>>> 
>>> "%.19s.%.3d %.50s"
>>> 
>>> come from?
>> 
>> The first argument is a char[DT_SIZE], which is MAXLEN_DT+1, which is 20. 
>> The +1 is probably for a null. The 3rd argument is char[TZ_SIZE], which is 
>> TIMESTAMP_SIZE - MAXLEN_DT - MAXLEN_MS, which is 81 -19 - 5, which is 56. 
>> The target buffer is char[MAXLEN_TIMESTAMP+1], which is 81 (and so is 
>> ltbuf). So without any of Coleen's changes we are writing at most 19 + 3 + 1 
>> + 56 + 1 bytes, which is 80 bytes, into an 81 byte buffer. The compiler 
>> should not be complaining here. 
>> 
>> And just to clarify, the compiler DOES see the size of the destination 
>> buffer. It's looking at the source of the argument passed in. However, it 
>> seems to have computed some lengths incorrectly and came to the conclusion 
>> that the buffer might not be big enough to handle the known sizes of all the 
>> snprintf arguments.
>
> Please make sure `linux-x86` jobs pass in GH actions. I think you can 
> navigate to https://github.com/coleenp/jdk/runs/1355854648 -- and press 
> "Re-run jobs" at top right.

FWIW I don't think I intended the workaround of specifying limits to be patched 
in as-is, although it seems benign to go ahead and do this. While it's possible 
that these warnings on 32-bit builds is just a matter of the compiler being 
wrong, it could also be that it can't determine all arguments are 
null-terminated. Specifying max string length in format specifiers seem a 
benign and cheap safeguard.

-

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

Re: RFR: 8255455: Pre-generate ThreadHeapSampler::_log_table [v7]

[Claes Redestad]

On Fri, 30 Oct 2020 17:39:05 GMT, Ioi Lam  wrote:

>> Claes Redestad 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 11 additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into threadHeapSampler_constexpr
>>  - Check log_table on startup in debug builds
>>  - Add explicit include of logging
>>  - Add const, fix copyright
>>  - Desugar constexpr into code generator and output
>>  - Merge branch 'master' into threadHeapSampler_constexpr
>>  - Declare and define log_table as a static constexpr inside 
>> threadHeapSampler.cpp
>>  - Merge branch 'master' into threadHeapSampler_constexpr
>>  - Merge branch 'master' into threadHeapSampler_constexpr
>>  - Remove _log_table_initialized assert
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/7a199d94...0eebe57c
>
> LGTM

@iklam: thanks!

-

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

Integrated: 8255455: Pre-generate ThreadHeapSampler::_log_table

[Claes Redestad]

On Tue, 27 Oct 2020 14:00:34 GMT, Claes Redestad  wrote:

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

This pull request has now been integrated.

Changeset: 4158567f
Author:Claes Redestad 
URL:   https://git.openjdk.java.net/jdk/commit/4158567f
Stats: 317 lines in 3 files changed: 296 ins; 10 del; 11 mod

8255455: Pre-generate ThreadHeapSampler::_log_table

Reviewed-by: iklam, sspitsyn

-

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

Re: RFR: 8255455: Pre-generate ThreadHeapSampler::_log_table [v6]

[Claes Redestad]

On Thu, 29 Oct 2020 20:04:11 GMT, Ioi Lam  wrote:

>> Claes Redestad has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Add explicit include of logging
>>  - Add const, fix copyright
>
> Marked as reviewed by iklam (Reviewer).

@iklam: I added a runtime verification pass that the values in the log_table 
are reasonably accurate. This adds around 100k instructions on startup on a 
debug build, which is in the noise there (currently hello world takes 470M on 
fastdebug builds). The checking is disabled in a code gen run to not put up 
tripwires when resizing the table.

-

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

Re: RFR: 8255455: Pre-generate ThreadHeapSampler::_log_table [v7]

[Claes Redestad]

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

Claes Redestad 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 11 additional commits 
since the last revision:

 - Merge branch 'master' into threadHeapSampler_constexpr
 - Check log_table on startup in debug builds
 - Add explicit include of logging
 - Add const, fix copyright
 - Desugar constexpr into code generator and output
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Declare and define log_table as a static constexpr inside 
threadHeapSampler.cpp
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Remove _log_table_initialized assert
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/b5bd0028...0eebe57c

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/880/files
  - new: https://git.openjdk.java.net/jdk/pull/880/files/a802814f..0eebe57c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=880=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=880=05-06

  Stats: 11692 lines in 331 files changed: 6229 ins; 3920 del; 1543 mod
  Patch: https://git.openjdk.java.net/jdk/pull/880.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/880/head:pull/880

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v6]

[Claes Redestad]

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

Claes Redestad has updated the pull request incrementally with two additional 
commits since the last revision:

 - Add explicit include of logging
 - Add const, fix copyright

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/880/files
  - new: https://git.openjdk.java.net/jdk/pull/880/files/61bac3dc..a802814f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=880=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=880=04-05

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

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr

[Claes Redestad]

On Thu, 29 Oct 2020 08:56:13 GMT, Lin Zang  wrote:

>> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
>> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
>> initialization into a constexpr, we can precalculate the helper table at 
>> compile time, which trades a runtime overhead for a small, 8kb, static 
>> footprint increase.
>> 
>> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
>> experiment[1] (not included in this PR) and show that the `fast_log2` is 
>> ~2.5x faster than `log2` on my system. And that without the lookup table 
>> we'd be much worse. So I think it makes sense to preserve this optimization, 
>> but get rid of the startup overhead:
>> 
>> [5.428s][debug][heapsampling] log2, 0.0751173 secs
>> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
>> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
>> 
>> I've verified that this refactoring does not affect performance in this 
>> naive setup.
>> 
>> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1
>
> Dear @cl4es, 
> I am not a reviewer, just have 1 comment that maybe you need to update the 
> Year info in the headers of touched files. 
> 
> Thanks.
> Lin

Unfortunately there's currently no portable way to use `std::log` (or any of 
the other `std` math functions) in a constexpr, so I had to resort to a code 
generator approach instead. It's either that or withdrawing this PR.

Using UL and a debug-only block to implement an adhoc code generator 
(`-Xlog:heapsampling+generate::none`) might be a bit unorthodox, but I think it 
turned out OK.

-

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v5]

[Claes Redestad]

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Desugar constexpr into code generator and output

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/880/files
  - new: https://git.openjdk.java.net/jdk/pull/880/files/f796147d..61bac3dc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=880=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=880=03-04

  Stats: 292 lines in 2 files changed: 281 ins; 0 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/880.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/880/head:pull/880

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v2]

[Claes Redestad]

On Tue, 27 Oct 2020 23:08:21 GMT, Ioi Lam  wrote:

>> Claes Redestad 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 three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into threadHeapSampler_constexpr
>>  - Remove _log_table_initialized assert
>>  - Refactor ThreadHeapSampler::_log_table as constexpr
>
> Changes requested by iklam (Reviewer).

The change to declare the log_table as constexpr exposed an issue with the std 
`log` function not being constexpr, which now cause a build error on Windows 
and Mac. It works on Linux, but likely not robustly so since I seem to have 
been stumbling into a non-compliant quirk: 
https://stackoverflow.com/questions/50477974/constexpr-exp-log-pow

-

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v4]

[Claes Redestad]

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

Claes Redestad 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 six additional 
commits since the last revision:

 - Merge branch 'master' into threadHeapSampler_constexpr
 - Declare and define log_table as a static constexpr inside 
threadHeapSampler.cpp
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Remove _log_table_initialized assert
 - Refactor ThreadHeapSampler::_log_table as constexpr

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/880/files
  - new: https://git.openjdk.java.net/jdk/pull/880/files/62148744..f796147d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=880=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=880=02-03

  Stats: 364 lines in 32 files changed: 294 ins; 21 del; 49 mod
  Patch: https://git.openjdk.java.net/jdk/pull/880.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/880/head:pull/880

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v2]

[Claes Redestad]

On Tue, 27 Oct 2020 23:08:13 GMT, Ioi Lam  wrote:

>> Claes Redestad 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 three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into threadHeapSampler_constexpr
>>  - Remove _log_table_initialized assert
>>  - Refactor ThreadHeapSampler::_log_table as constexpr
>
> src/hotspot/share/runtime/threadHeapSampler.cpp line 48:
> 
>> 46: };
>> 47: 
>> 48: const FastLogTable 
>> ThreadHeapSampler::_log_table;
> 
> To guarantee that `_log_table` is evaluated at C++ compile time, it's best to 
> change the code to
> 
> constexpr FastLogTable _log_table;
> 
> There are 2 reasons:
> 
> 1. C++ guarantees compile-time evaluation only if the expression is used in a 
> "constexpr context". You can read more from 
> [here](https://isocpp.org/blog/2013/01/when-does-a-constexpr-function-get-evaluated-at-compile-time-stackoverflow).
> 2. In the future, if the `FastLogTable` constructor is modified in a way that 
> cannot be evaluated at compile time, (e.g., someone removes `constexpr` from 
> the constructor's declaration by mistake, the C++ compiler will catch that 
> and tell you that `_log_table` cannot be `constexpr`.
> 
> Unfortunately,  you cannot use `constexpr` in forward declarations, so you 
> should either move the definition of `FastLogTable` to the hpp file, or 
> remove the declaration of `_log_table` from the hpp file.

Ok, makes sense.

I went with removing the `_log_table` declaration from the hpp file. I think 
things came out a bit cleaner as a result, too.

-

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v3]

[Claes Redestad]

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

Claes Redestad 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 five additional 
commits since the last revision:

 - Declare and define log_table as a static constexpr inside 
threadHeapSampler.cpp
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Merge branch 'master' into threadHeapSampler_constexpr
 - Remove _log_table_initialized assert
 - Refactor ThreadHeapSampler::_log_table as constexpr

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/880/files
  - new: https://git.openjdk.java.net/jdk/pull/880/files/f28b9cc4..62148744

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

  Stats: 922 lines in 50 files changed: 556 ins; 221 del; 145 mod
  Patch: https://git.openjdk.java.net/jdk/pull/880.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/880/head:pull/880

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

Re: RFR: 8255455: Refactor ThreadHeapSampler::_log_table as constexpr [v2]

[Claes Redestad]

> The static `ThreadHeapSampler::_log_table` is currently initialized on JVM 
> bootstrap to an overhead of ~67k instructions (linux-x64). By turning the 
> initialization into a constexpr, we can precalculate the helper table at 
> compile time, which trades a runtime overhead for a small, 8kb, static 
> footprint increase.
> 
> I compared `fast_log2` with the `log2` builtin with a naive benchmarking 
> experiment[1] (not included in this PR) and show that the `fast_log2` is 
> ~2.5x faster than `log2` on my system. And that without the lookup table we'd 
> be much worse. So I think it makes sense to preserve this optimization, but 
> get rid of the startup overhead:
> 
> [5.428s][debug][heapsampling] log2, 0.0751173 secs
> [5.457s][debug][heapsampling] fast_log2, 0.0298244 secs
> [5.622s][debug][heapsampling] fast_log2_uncached, 0.1645569 secs
> 
> I've verified that this refactoring does not affect performance in this naive 
> setup.
> 
> [1] https://github.com/openjdk/jdk/compare/master...cl4es:log2_micro?expand=1

Claes Redestad 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 three additional 
commits since the last revision:

 - Merge branch 'master' into threadHeapSampler_constexpr
 - Remove _log_table_initialized assert
 - Refactor ThreadHeapSampler::_log_table as constexpr

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/880/files
  - new: https://git.openjdk.java.net/jdk/pull/880/files/2a66158a..f28b9cc4

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

  Stats: 1880 lines in 75 files changed: 1373 ins; 312 del; 195 mod
  Patch: https://git.openjdk.java.net/jdk/pull/880.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/880/head:pull/880

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

Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)

[Claes Redestad]

Hi Jie,

fix looks good to me!

/Claes

On 2020-08-25 04:12, jiefu(傅杰) wrote:

Thanks Serguei for your review.

Claes, are you okay with this change: 
http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/


Thanks.
Best regards,
Jie



*From:* serguei.spit...@oracle.com 
*Sent:* Tuesday, August 25, 2020 8:38 AM
*To:* jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
*Subject:* Re: 8251155: HostIdentifier fails to canonicalize hostnames 
starting with digits(Internet mail)

Hi Jie,

I'm okay with the fix.

Thanks,
Serguei


On 8/24/20 09:21, jiefu(傅杰) wrote:


Hi Serguei and Claes,

I forget to mention that you can also verify this fix using the 
following tests:


--

test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java

test/jdk/sun/tools/jstatd/TestJstatdPort.java

test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java

test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java

--

Without the patch, All of them will fail if the hostname starting from 
digits.


We've found that it seems very common that the hostname will start 
with digits in dockers.


So it would be better to fix it.

What do you think?

Thanks.

Best regards,

Jie

*From: *"jiefu(傅杰)" 
*Date: *Wednesday, August 19, 2020 at 4:05 PM
*To: *"serguei.spit...@oracle.com" , 
"serviceability-dev@openjdk.java.net" 
, Claes Redestad 

*Subject: *Re: 8251155: HostIdentifier fails to canonicalize hostnames 
starting with digits(Internet mail)


Hi Serguei,

Thanks for your review and help.

Please see comments inline.



*From:*serguei.spit...@oracle.com 
*Sent:* Wednesday, August 19, 2020 4:03 AM
*To:* jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
*Subject:* Re: 8251155: HostIdentifier fails to canonicalize hostnames 
starting with digits(Internet mail)


  83  * 
  84  *   {@code } - transformed into "//localhost"
  85  *   localhost - transformed into "//localhost"
  86  *   hostname - transformed into "//hostname"
  87  *   hostname:port - transformed into "//hostname:port"
  88  *   proto:hostname - transformed into "proto://hostname"
  89  *   proto:hostname:port - transformed into
  90  *  "proto://hostname:port"
  91  *   proto://hostname:port
  92  * 

>> Is it worth to add an example to the list above?

Yes. It's really helpful for the review process. Thanks.



>> I wander if this fix needs a CSR.

I don't think so.

This is just a bug fix which doesn't add/remove/change any feature of 
the tools.


The original design has claimed to support hostname and hostname:port 
cases.


But it fails to do so when the hostname starts with digits.

It seems to be very common that the hostname will be started with 
digits in dockers.


So I think it's worth to fix this bug.


>> How did you check this fix does not introduce any regressions?

In fact, Claes had helped me to answer this question here: 
https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032691.html.


Also, I've tested this patch on Linux/x64 with 
tier1 ~ tier3 (no regression).


Thanks a lot.

Best regards,

Jie

Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)

[Claes Redestad]

Hi,

not sure I do, but a quick read of the relevant RFC suggests that since
a URI scheme (protocol) must start with a letter[1] it seems safe to
assume the string must be of the form hostname or hostname:port if the
first character in the string is a digit.

/Claes

[1] https://tools.ietf.org/html/rfc3986#section-3.1

On 2020-08-18 22:03, serguei.spit...@oracle.com wrote:

Hi Jie,

I've added Claes to the list as he may have an expertise in this area.

   83  * 
   84  *   {@code } - transformed into "//localhost"
   85  *   localhost - transformed into "//localhost"
   86  *   hostname - transformed into "//hostname"
   87  *   hostname:port - transformed into "//hostname:port"
   88  *   proto:hostname - transformed into "proto://hostname"
   89  *   proto:hostname:port - transformed into
   90  *  "proto://hostname:port"
   91  *   proto://hostname:port
   92  * 

Is it worth to add an example to the list above?

I wander if this fix needs a CSR.
How did you check this fix does not introduce any regressions?

Thanks,
Serguei


On 8/17/20 08:13, jiefu(傅杰) wrote:


Ping…

Any comments?

Thanks.

Best regards,

Jie

*From: *serviceability-dev  
on behalf of "jiefu(傅杰)" 

*Date: *Friday, August 7, 2020 at 7:44 AM
*To: *"serviceability-dev@openjdk.java.net" 

*Subject: *Re: RFR: 8251155: HostIdentifier fails to canonicalize 
hostnames starting with digits(Internet mail)


FYI:

  This bug will lead to failures of the following tests on machines 
with hostname starting from digits.


    - test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java

    - test/jdk/sun/tools/jstatd/TestJstatdPort.java

    - test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java

    - test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java

So it's worth fixing it.

Testing:

  - tier1-3 on Linux/x64

Thanks.

Best regards,

Jie

*From: *"jiefu(傅杰)" 
*Date: *Wednesday, August 5, 2020 at 3:19 PM
*To: *"serviceability-dev@openjdk.java.net" 

*Subject: *RFR: 8251155: HostIdentifier fails to canonicalize 
hostnames starting with digits


Hi all,

JBS: https://bugs.openjdk.java.net/browse/JDK-8251155

Webrev: http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/

HostIdentifier fails to canonicalize hostname:port if the hostname 
starts with digits.


The current implementation will get "scheme = hostname".

But the scheme should not be started with digits, which leads to this bug.

Thanks a lot.

Best regards,

Jie

Re: RFR(S) 8246019 PerfClassTraceTime slows down VM start-up

[Claes Redestad]

On 2020-06-17 05:19, Ioi Lam wrote:



On 6/16/20 6:20 PM, David Holmes wrote:

Hi Ioi,

On 17/06/2020 6:14 am, Ioi Lam wrote:

https://bugs.openjdk.java.net/browse/JDK-8246019
http://cr.openjdk.java.net/~iklam/jdk16/8246019-avoid-PerfClassTraceTime.v01/ 



PerfClassTraceTime is (a rarely used feature) for measuring the time 
spent during class linking and initialization.


"A special command jcmd  PerfCounter.print 
prints all performance counters in the process."


How do you know this is a "rarely used feature"?

Hi David,

Sure, the counter will be dumped, but by "rarely used" -- I mean no one 
will find this particular counter useful, and no one will be actively 
looking at it.


I changed two parts of the code -- class init and class linking.

For class initialization, the counter may be useful for people who want 
to know how much time is spent in their  functions, and my patch 
doesn't change that. It only avoids using the counter when a class has 
no , i.e., we know that the counter counts nothing (except for a 
logging statement).


=

For class linking, no user code is executed, so it only measures VM 
code. If it's useful for anyone, that would be VM engineers like me who 
are trying to optimize the speed of class loading. However, due to the 
overhead of the counter vs what it's trying to measure, the results are 
pretty meaningless.


Note that I've not disabled the counter altogether. Instead, I disable 
it only when linking a CDS shared class, and we know that very little is 
happening for this class (e.g., no verification).


I think the class linking timer might have been useful 15 years ago when 
it was introduced, or it might be useful today when CDS is disabled. But 
with CDS enabled, we are paying a constant price that seems to benefit 
no one.


I think we should short-circuit it when it seems appropriate. If this 
indeed causes problems for our users, it's easy to re-enable it. That's 
better than just keeping this forever just because we're afraid to touch 
anything.


I think this seems like well-rounded approach overall, but this assumes
that we're mostly measuring the overhead of measurement here. I don't
doubt that's the case for the scenarios you're excluding here and now,
but it's hard to guarantee this property hold in the future.

Perhaps a diagnostic flag to enable timing unconditionally would be
appropriate? With such a flag we could verify that the time deltas of
running some applications with and without the flag roughly matches the
time delta in reported linking time. If they diverge, we might need to
adjust the conditions.





I find it hard to evaluate whether this short-circuiting of the time 
tracing is reasonable or not. Obviously any monitoring mechanism 
should impose minimal overhead compared to what is being measured, and 
these timers fall short in that regard. But if these stats become 
meaningless then they may as well be removed.


I think the serviceability folk (cc'd) need to evaluate this in the 
context of the M tools.


As a complement (or even alternative) there might be ways we can reduce
time-to-measure overheads. E.g, JFR added
FastUnorderedElapsedCounterSource (share/utilities/ticks.hpp) which uses
rdtsc if available (x86 - fallback to os::elapsed_counter otherwise).

This might be a reasonable alternative for the Perf* timers, which
should be short-running events on a single thread.

/Claes



However, it's quite expensive and it needs to start and stop a bunch 
of timers. With CDS, it's quite often for the overhead of the timer 
itself to be much more than the time it's trying to measure, giving 
unreliable measurement.


In this patch, when it's clear that the init and linking will be very 
quick, I disable the timer and count only the number of invocations. 
This shows a small improvement in start-up


I'm curious if you tried to forcing EagerInitialization to be true to 
see how that improves the baseline. I've always noticed eager_init in 
the code, but hadn't realized it is disabled by default.




I think it cannot be done by default, as it will violate the JLS. A 
class can be initialized only when it's touched by bytecodes.


It can also backfire as we may load many classes without initializing 
them. E.g., during bytecode verification, we load many classes and just 
check that one is a supertype of another.


Thanks
- Ioi


Cheers,
David
-

Results of " perf stat -r 100 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "


59623970 59341935 (-282035)   -  41.774  41.591 ( -0.183) -
59623495 59331646 (-291849)   -  41.696  41.165 ( -0.531) --
59627148 59329526 (-297622)   -  41.249  41.094 ( -0.155) -
59612439 59340760 (-271679)      41.773  40.657 ( -1.116) -
59626438 59335681 (-290757)   -  41.683  40.901 ( -0.782) 
59618436 59338953 (-279483)   -  41.861  41.249 ( -0.612) ---
59608782 59340173 (-268609)      41.198  41.508 (  0.310) +
59614612 59325177 (-289435)   

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[Claes Redestad]

On 2020-03-27 03:18, David Holmes wrote:


Yeah they confuse me. Which makes it hard to see what impact your 
changes may have.


This patch removes some internal, unused code on the JVM end that is not
observable via jstat / jvmstat. I'm happy if serviceability can weigh in
though.

The other RFE[1] I've filed to remove StatSampler[1] might be more
contentious since it changes what gets periodically stored in the
perfdata shared file. I've not yet decided if it's worth the trouble to
move ahead with that at this point.

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8241701

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[Claes Redestad]

On 2020-03-27 00:36, David Holmes wrote:




Okay so can you change the bug synopsis and description to cover this 
more general cleanup and tuneup please.


I filed an addendum RFE and will add this RFE bug id to the single
changeset push:
https://bugs.openjdk.java.net/browse/JDK-8241705



I'm never very clear on the uses of these PerfCounters. It seems SUN_NS 
is unused after this change. The references to jvmstat seem no longer 
correct - these are read via jstat ?


The general confusion about PerfData/-Counters and what they're for is
why I'm trying to untangle this. Generally I think we should pull the
plug on it, but the perfdata shared file is tangled up with
functionality to detect running JVMs used by jcmd etc, so it might
take a few iterations to get there.

/Claes

Re: RFR: 8220682: Heap dumping and inspection fails with JDK-8214712

[Claes Redestad]

On 2019-03-25 23:10, serguei.spit...@oracle.com wrote:

Hi Claes,

It looks good.
I've targeted the bug to 13.


Thanks, Serguei!

/Claes

Re: RFR: 8220682: Heap dumping and inspection fails with JDK-8214712

[Claes Redestad]

On 2019-03-25 17:51, Jiangli Zhou wrote:

Hi Claes,

The updated change also matches with the state of shared klasses, so it 
seems good to me. 


Thanks!

Sorry for the delay. 


No worries, I was on vacation last week. :-)

It's probably a good idea to add 
an assert to make sure k->is_shared() in both places. No new webrev is 
necessary for me for the assert change.


Ok, will do and run some tests.

Thanks!

/Claes

Re: RFR: 8220682: Heap dumping and inspection fails with JDK-8214712

[Claes Redestad]

[Adding hotspot-runtime-dev]

On 2019-03-15 17:26, Claes Redestad wrote:

Hi,

I ran into a few additional test issues, one exposing a crash in jmap
-histo:live in a test that seems to have been problem listed when I
ran tests earlier.

Also filtering out archived-but-not-yet-loaded classes in
KlassInfoBucket::lookup makes tests exercising jmap etc work as intended
in the presence of archived classes:

http://cr.openjdk.java.net/~redestad/8220682/open.01/

Testing: tier1-3 with 8214712 ongoing

/Claes

On 2019-03-15 16:28, Jiangli Zhou wrote:

+1

Thanks,
Jiangli

On Fri, Mar 15, 2019 at 7:56 AM Jean Christophe Beyler 
mailto:jcbey...@google.com>> wrote:


    Hi Claes,

    Looks good to me. For those who would want to see the patch change
    done in the serviceability-agent:
    http://hg.openjdk.java.net/jdk/jdk/rev/91f06b86c0da

    Thanks,
    Jc

Re: RFR: 8220682: Heap dumping and inspection fails with JDK-8214712

[Claes Redestad]

Ping.

Still passes tests and is necessary to unblock work to more aggressively
archive invariant heap graphs.

/Claes

On 2019-03-15 17:26, Claes Redestad wrote:

Hi,

I ran into a few additional test issues, one exposing a crash in jmap
-histo:live in a test that seems to have been problem listed when I
ran tests earlier.

Also filtering out archived-but-not-yet-loaded classes in
KlassInfoBucket::lookup makes tests exercising jmap etc work as intended
in the presence of archived classes:

http://cr.openjdk.java.net/~redestad/8220682/open.01/

Testing: tier1-3 with 8214712 ongoing

/Claes

On 2019-03-15 16:28, Jiangli Zhou wrote:

+1

Thanks,
Jiangli

On Fri, Mar 15, 2019 at 7:56 AM Jean Christophe Beyler 
mailto:jcbey...@google.com>> wrote:


    Hi Claes,

    Looks good to me. For those who would want to see the patch change
    done in the serviceability-agent:
    http://hg.openjdk.java.net/jdk/jdk/rev/91f06b86c0da

    Thanks,
    Jc

Re: RFR: 8220682: Heap dumping and inspection fails with JDK-8214712

[Claes Redestad]

Hi,

I ran into a few additional test issues, one exposing a crash in jmap
-histo:live in a test that seems to have been problem listed when I
ran tests earlier.

Also filtering out archived-but-not-yet-loaded classes in
KlassInfoBucket::lookup makes tests exercising jmap etc work as intended
in the presence of archived classes:

http://cr.openjdk.java.net/~redestad/8220682/open.01/

Testing: tier1-3 with 8214712 ongoing

/Claes

On 2019-03-15 16:28, Jiangli Zhou wrote:

+1

Thanks,
Jiangli

On Fri, Mar 15, 2019 at 7:56 AM Jean Christophe Beyler 
mailto:jcbey...@google.com>> wrote:


Hi Claes,

Looks good to me. For those who would want to see the patch change
done in the serviceability-agent:
http://hg.openjdk.java.net/jdk/jdk/rev/91f06b86c0da

Thanks,
Jc

Re: RFR: 8220682: runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryErrorInMetaspace.java fails with JDK-8214712

[Claes Redestad]

Hi Jc,

On 2019-03-15 15:55, Jean Christophe Beyler wrote:

Hi Claes,

Looks good to me.


thanks!

/Claes

For those who would want to see the patch change done 
in the serviceability-agent:

http://hg.openjdk.java.net/jdk/jdk/rev/91f06b86c0da

RFR: 8220682: runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryErrorInMetaspace.java fails with JDK-8214712

[Claes Redestad]

Hi,

to allow for archiving objects in CDS that might not be loaded (e.g.,
JDK-8214712), we need to ignore objects at heap dump time that haven't
actually been loaded. This patch fixes the VM heapDumper implementation,
and is a mirror of a fix already done in the serviceability-agent heap
dumper[1].

Bug: https://bugs.openjdk.java.net/browse/JDK-8220682
Patch:
diff -r 082cd1e3c441 src/hotspot/share/services/heapDumper.cpp
--- a/src/hotspot/share/services/heapDumper.cpp	Mon Dec 03 16:25:27 2018 
+0100
+++ b/src/hotspot/share/services/heapDumper.cpp	Fri Mar 15 13:11:28 2019 
+0100

@@ -958,6 +958,11 @@
 // creates HPROF_GC_INSTANCE_DUMP record for the given object
 void DumperSupport::dump_instance(DumpWriter* writer, oop o) {
   Klass* k = o->klass();
+  if (k->java_mirror() == NULL) {
+// Ignoring this object since the corresponding java mirror is not 
loaded.

+// Might be a dormant archive object.
+return;
+  }

   writer->write_u1(HPROF_GC_INSTANCE_DUMP);
   writer->write_objectID(o);


Thanks!

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8214756

Re: RFR: JDK-8214756: SA should ignore archived java heap objects that are not in use

[Claes Redestad]

Hi Jini,

changes looks good, and thanks for verifying your fix unblock 8214712!

/Claes

On 2019-02-24 06:01, Jini George wrote:
Thank you very much, Jiangli! Yes, I did test 
open/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java 
after applying the changes from:


http://cr.openjdk.java.net/~redestad/8214712/jdk.00/

and it passed after this change.

Thanks,
Jini.

On 2/24/2019 9:38 AM, Jiangli Zhou wrote:

Hi Jini,

The change reflects the states of archived java objects and klass 
metadata properly at runtime and looks good to me.


You might have already done so, for additional testing try running 
open/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java 
with the planned changes for JDK-8214712 
.


Thanks,
Jiangli

On Sat, Feb 23, 2019 at 10:00 AM Jini George > wrote:


    Requesting reviews for a very small change to fix:

    https://bugs.openjdk.java.net/browse/JDK-8214756

    Webrev: 
http://cr.openjdk.java.net/~jgeorge/8214756/webrev.00/index.html


    The proposed change is to ignore objects whose associated classes are
    unloaded while walking the heap to create a heapdump in SA.

    Thanks,
    Jini.

Re: RFR(S) 8218751 Do not store original classfiles inside the CDS archive

[Claes Redestad]

Hi,

in my opinion it's way too easy to add flags compared to how hard they
are to get rid of, so I agree with postponing the addition of the flag
until it's requested based on a real need.

Thanks!

/Claes

On 2019-02-20 23:17, serguei.spit...@oracle.com wrote:

Hi Ioi,


Sorry that I missed that this discussion moved to the runtime-dev only.

In fact, I like the first webrev. It makes sense to safe on CDS footprint.
What about to postpone adding option EnableOptionalDataSharedSpace until 
real customers request it?

We need to understand real use cases first.

In general, I'm not convinced there is a real performance problem at 
startup

when CDS is used with native or java agents which enable CFLH events.
At least, it seems to be a rare use case, and it is not performance 
critical.


Thanks,
Serguei


On 2/18/19 22:58, Ioi Lam wrote:

Hi Jiangli,

I think you're right that apps that use CFLH but rarely redefine 
classes might see a slight speed up with the OD space. I don't know 
how prevalent such uses cases are, but I won't object to making the OD 
space optional.


So, I added a new flag to enable the OD space. Instead of the name you 
suggested, I used EnableOptionalDataSharedSpace so that it's related 
to other XXXSharedSpace flags. I also added a sanity test case, and 
fixed a Misplaced ResourceMark in klassFactory.cpp


http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02/ 

http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02-delta/ 



Please let me know what you think.

Thanks
- Ioi

On 2/18/19 8:12 PM, Jiangli Zhou wrote:


On Mon, Feb 18, 2019 at 11:33 AM Ioi Lam > wrote:




    On 2/15/19 7:11 PM, Jiangli Zhou wrote:

    On Fri, Feb 15, 2019 at 6:56 PM Ioi Lam mailto:ioi@oracle.com>> wrote:

    On 2/15/19 4:54 PM, Jiangli Zhou wrote:

    Hi,

    To answer your use case question, one of the case reported
    last year in OpenJDK was JRebel (please go back to the
    hotspot-dev mail list 2018 Oct. archive). That is an
    existing example as I've tried to point out in my earlier 
email.



    First of all, if you read the email carefully, CDS was not
    even in their usual test matrix. Also, if you're modifying
    classes on the fly, you're slowing down start-up big time. A
    few ms spent in decoding the classfile data will be
    completely drown out by the overhead of the classfile parsing
    and patching code in the JVMTI agent.

    So no, JRebel will not start up a few % faster if CDS stores
    the classfile data. It will most likely be not measurable.


    My understanding of the case is that their users generated a CDS
    archive and used it together with JRebel. That's why the issue
    was unnoticed until their users reported. There are use cases out
    there in the community that we simply are not aware of, so it
    would not be wise to assume there is no usage.


    Just to confirm one thing -- you do not dispute my claim that this
    "optimization" has no benefits even for those that actually use
    CDS and CFLH together. Correct?


That's actually the part that I have a different opinion. With the 
proposed change in the current webrev, the performance hit is across 
the board for loading shared classes regardless if there is a class 
being redefined/retransformed, as long as the 
can_generate_all_class_hook_events capability is enabled.



    We have plenty of nice-to-have harmless optimizations in HotSpot
    that probably weren't vigorously validated. However, this is not
    one of them.

    Here we have clear evidence of harm (50% footprint increase), with
    no evidence of benefit (theoretical or real-life). This code was
    checked into HotSpot without proper validation. That was wrong,
    and that's why I am taking it out now.

    It's easy to prove me wrong. Just supply a proper benchmark that
    shows benefits, and I will change my mind. That's the minimal
    standard for an optimization that has harmful side effects. I have
    created JDK-8219255 for tracking this.


Looks like there is a deadlock in the discussion. To summarize and 
help move this forward. Here are your reasons for dropping the 'od' 
space:


  * static footprint increase due to 'od' space
  * no benefit
  * maintenance of the code

Regarding static footprint, the change proposed in the current webrev 
is a short-term and temporary solution. For a long-term solution, the 
original class files can be removed from the final image created by 
jlink when jlink can work with CDS to produce a single runtime image.


On the benefit side, the 'od' space provides both startup speedup and 
runtime memory saving (when can_generate_all_class_hook_events 
capability is enabled), which I've already pointed those out in 
earlier emails.


For maintenance, it is also not an issue with the support from the 
community (I'll for sure to continue 

Re: RFR (M) 8213003: Remove more assignments in ifs for vmTestbase/[a-s]

[Claes Redestad]

Hi,

here's a case that your script seems to miss:

http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html

 if (!NSK_JNI_VERIFY(jni, (testedFieldID =
 jni->GetStaticFieldID(testedClass, FIELD_NAME, FIELD_SIGNATURE)) 
!= NULL))

I'd note that with some of your changes here you're unconditionally executing 
logic outside of NSK_JNI_VERIFY macros. Does care need be taken to wrap the 
code blocks in equivalent macros/ifdefs? Or are the NSK_JNI_VERIFY macros 
always-on in these tests (and essentially pointless)?

/Claes

On 2018-10-31 05:42, JC Beyler wrote:

Hi all,

I worked on updating my script and handling more assignments so I 
redid a second pass on the same files to get all the cases. Now, on 
those files the regex "if.* = " no longer shows any cases we would 
want to fix.


Could I get a review for this webrev:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003

I tested this on my dev machine by passing all the tests that were 
modified.


Thanks!
Jc

Re: RFR: 8202417: [TESTBUG] Broken hard-coded dependency in serviceability/sa/ClhsdbJhisto.java

[Claes Redestad]

On 2018-04-30 11:33, Alan Bateman wrote:
This looks okay, at least as a short term fix. I agree it's way too 
fragile to depend on such classes and needs to stick to classes that 
are closer to the area that it tests.


Thanks, Alan!

/Claes

RFR: 8202417: [TESTBUG] Broken hard-coded dependency in serviceability/sa/ClhsdbJhisto.java

[Claes Redestad]

Hi,

ClhsdbJhisto has a hard-coded dependency on an 
ImmutableCollections$SetN$1 appearing in the jhisto output, which is 
fragile and sometimes break after JDK-8201650. I suggest removing it 
from the test:


diff -r 96d4658eb7f2 test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java
--- a/test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java  Mon Apr 30 
09:15:44 2018 +0200
+++ b/test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java  Mon Apr 30 
11:28:08 2018 +0200

@@ -62,8 +62,7 @@
 "java.nio.HeapByteBuffer",
 "java.net.URI",
 "LingeredAppWithInterface",
-    "ParselTongue",
-    "ImmutableCollections$SetN$1"));
+    "ParselTongue"));

 test.run(theApp.getPid(), cmds, expStrMap, null);
 } catch (Exception ex) {

Re: RFR: JDK-8151815: Could not parse core image with JSnap.

[Claes Redestad]

Hi,

it seems destroy() used nullness of _prologue to determine that
the counter has already been destroyed, should this use _destroyed
now?

 165   if (_prologue == NULL) return;


/Claes

On 2017-09-26 17:01, Yasumasa Suenaga wrote:

Hi all,

I uploaded new webrev to be adapted to jdk10/hs:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.04/


Thanks,

Yasumasa


On 2017/09/21 7:45, Yasumasa Suenaga wrote:

PING:

Have you checked this issue?


http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/



Yasumasa


On 2017/07/01 23:43, Yasumasa Suenaga wrote:

PING:

Have you checked this issue?


Yasumasa


On 2017/06/13 14:10, Yasumasa Suenaga wrote:

Hi all,

I want to discuss about JDK-8151815: Could not parse core image 
with JSnap.



In last year, I found JSnap cannot parse coredump and I've sent review
request for it as JDK-8151815. However it has not been reviewed yet
[1].

We've discussed about safety implementation, but we could not get 
consensus.

IMHO all SA tools should be handled java processes and core images,
and PerfCounter value is useful. So I fix this issue.

I uploaded new webrev for this issue. I think this patch is safety
because new flag PerfMemory::_destroyed guards double free, and all
members in PerfMemory is accessible (they are not munmap'ed)

http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/


Can you cooperate?


Thanks,

Yasumasa


[1] 
http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-April/019480.html

Re: Review Request JDK-8171468: sun/management/jmxremote/bootstrap/CustomLauncherTest.java fails as lib/$ARCH no longer exists

[Claes Redestad]

Oops, seems you fixed it, nevermind :-)

/Claes

On 2016-12-19 22:43, Claes Redestad wrote:

Hi,

seems you forgot to remove the code using LIBARCH around line 82

/Claes

On 2016-12-19 22:41, Mandy Chung wrote:

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171468/webrev.00/

The test is updated to check the new location of shared libraries.
lib/$ARCH has been removed.

Mandy

Re: Review Request JDK-8171468: sun/management/jmxremote/bootstrap/CustomLauncherTest.java fails as lib/$ARCH no longer exists

[Claes Redestad]

Hi,

seems you forgot to remove the code using LIBARCH around line 82

/Claes

On 2016-12-19 22:41, Mandy Chung wrote:

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171468/webrev.00/

The test is updated to check the new location of shared libraries. lib/$ARCH 
has been removed.

Mandy

Re: ThreadMXBean.getThreadAllocatedBytes() allocates memory

[Claes Redestad]

Hi Bengt,

I'm not Serviceability, but you know I can't leave them micro-
optimizations alone! :-)

So, reusing cached arrays could be made to work but would require
some synchronization to keep things thread-safe and tidy[1].

This will complicate the code, especially since there's another implied
allocation in getThreadAllocatedBytes. Not to mention that caching
objects which are cheap to allocate is a bit of an performance
anti-pattern.

Adding synchronization also comes with it's own risks, especially as
we're calling into JNI and the VM code takes a somewhat shady mutex
already (Threads_lock).

Generally I don't think there's ever any behavioral guarantees about how
much - or little - a  method won't allocate anything, so calling this a
bug is a bit of a stretch IMO, although it's a bit unfortunate in this
particular case.

TL;DR: I'm a bit skeptic, but if it's important to you to fix this, I
wouldn't think it's impossible.

Thanks!

/Claes

[1] Alternatively we could of course implement a JNI method taking a
long rather than a long[], which would be consistent with other methods
in ThreadImpl.java, but I think we want to avoid going that far.

On 2016-09-18 23:14, Bengt Rutisson wrote:


Hi Serviceability,

Not sure, but I hope this is the correct list to post this on.

I wanted to use the ThreadMXBean.getThreadAllocatedBytes() method to get
some information about how much memory some Java code allocated.

When I dug into the results they didn't properly add up until I realized
that the call to getThreadAllocatedBytes() actually allocates memory.
This was a surprise to me.

I'm attaching a small example to illustrate what I mean.

Running the example renders this output:

$ javac AllocMeasure.java
$ java AllocMeasure
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48
Bytes allocated: 48

What I would have expected was that it would say "Bytes allocated: 0"
since I would like to add my own code between line 9 and 10 in the
example and get the value for how much memory it allocates. As it is now
I have to deduct the bytes that the getThreadAllocatedBytes() allocates
to get the correct result.

The problem is that getThreadAllocatedBytes() is implemented this way:

 public long getThreadAllocatedBytes(long id) {
 long[] ids = new long[1];
 ids[0] = id;
 final long[] sizes = getThreadAllocatedBytes(ids);
 return sizes[0];
 }

http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/32d957185656/src/java.management/share/classes/sun/management/ThreadImpl.java#l345

I was surprised to see the "new long[1]". I realize that it is nice to
reuse getThreadAllocatedBytes(long []) method, but maybe a pre-allocated
array can be used instead of allocating a new one for each call?

I know the specification for this method is kind of fuzzy, but is this
to be considered a bug or does it work as intended?

Thanks,
Bengt

Re: 8164523: Clean up metadata for event based tracing

[Claes Redestad]

+1

/Claes

On 2016-08-20 20:49, Erik Gahlin wrote:

Hi,

Could I have review of this fix to the event tracing metadata. The patch
touches many files, but it doesn't change the control flow, only class
and field names.

Webrev:
http://cr.openjdk.java.net/~egahlin/8164523/

Bug:
https://bugs.openjdk.java.net/browse/JDK-8164523

Thanks
Erik

RFR(XS): 8134583: sun.management.HotspotCompilation should handle absence of per-thread perf counters

[Claes Redestad]

Hi,

please review this patch to clean up and make 
sun.management.HotspotCompilation
behave nice if the VM would decide to no longer expose per-compiler 
thread perf counters:


webrev: http://cr.openjdk.java.net/~redestad/jdk9/8134583/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8134583

/Claes

Re: RFR(XS): 8134583: sun.management.HotspotCompilation should handle absence of per-thread perf counters

[Claes Redestad]

Updated webrev after comments and discussion with Jaroslav:

http://cr.openjdk.java.net/~redestad/jdk9/8134583/webrev.03

Changes:
- convert 'threads' from array to list
- simplified further by removing old code dealing with adapterThread

/Claes

On 2015-08-27 15:21, Jaroslav Bachorik wrote:

On 27.8.2015 15:14, Claes Redestad wrote:



On 2015-08-27 15:04, Jaroslav Bachorik wrote:

Hi,

On 27.8.2015 14:41, Claes Redestad wrote:

Hi,

please review this patch to clean up and make
sun.management.HotspotCompilation
behave nice if the VM would decide to no longer expose per-compiler
thread perf counters:

webrev: http://cr.openjdk.java.net/~redestad/jdk9/8134583/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8134583


When already changing this wouldn't it be easier to convert the
'threads' variable to ListCompilerThreadStat and only add the info
for existing compilers threads (eg. not leaving NULL slots in the 
array).


In 'getCompilerThreadStats' method the 'threads' array is converted to
a list anyway.


The CompilerThreadStat object needs to be created on demand (since it
polls the underlying counters), thus we still need to maintain either an
array or list of CompilerThreadInfo. Converting CompilerThreadInfo[] to
a compact (or empty) ListCompilerThreadInfo may or may not save a few
bytes, but we'd still have to create a new list every time
getCompilerThreadStats() is called.


Right. Still could save some null value juggling by storing 
CompilerThreadInfo instances into a list instead of an array.


-JB-



/Claes



-JB-



/Claes

Re: RFR(XS): 8134583: sun.management.HotspotCompilation should handle absence of per-thread perf counters

[Claes Redestad]

On 2015-08-27 15:04, Jaroslav Bachorik wrote:

Hi,

On 27.8.2015 14:41, Claes Redestad wrote:

Hi,

please review this patch to clean up and make
sun.management.HotspotCompilation
behave nice if the VM would decide to no longer expose per-compiler
thread perf counters:

webrev: http://cr.openjdk.java.net/~redestad/jdk9/8134583/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8134583


When already changing this wouldn't it be easier to convert the 
'threads' variable to ListCompilerThreadStat and only add the info 
for existing compilers threads (eg. not leaving NULL slots in the array).


In 'getCompilerThreadStats' method the 'threads' array is converted to 
a list anyway.


The CompilerThreadStat object needs to be created on demand (since it 
polls the underlying counters), thus we still need to maintain either an 
array or list of CompilerThreadInfo. Converting CompilerThreadInfo[] to 
a compact (or empty) ListCompilerThreadInfo may or may not save a few 
bytes, but we'd still have to create a new list every time 
getCompilerThreadStats() is called.


/Claes



-JB-



/Claes

Poll: Remove per-compiler thread perf. counters?

[Claes Redestad]

Hi,

I want to raise the question if there are any known users
of these per-compiler thread perf. counters, or if they
should be removed?

sun.ci.compilerThread.#.compiles
sun.ci.compilerThread.#.method
sun.ci.compilerThread.#.time
sun.ci.compilerThread.#.type

For detailed information about compilation there are better
tools available (JFR, PrintCompilation), whereas the older
perf.counters are useful mostly for their aggregated
values.

/Claes

Re: RFR: 8028357: Unnecessary allocation in AliasFileParser

[Claes Redestad]

Hi Jaroslav, thanks for looking at this!

On 2014-12-30 14:43, Jaroslav Bachorik wrote:


While it's good to see the dead code gone I would prefer keeping the 
logging code just commented out - the logging code could be useful 
when investigating any future failures.


Do you mean all of it, or just the utility methods? I'm in the habit of 
not checking in commented out code (due to potential for bit rot etc) 
and instead rely on recreating from VCS history, but I have no strong 
preference.


How about leaving the non-trivial dumpAll/dump_entry_fixed-methods 
(which use the DEBUG flag but not the log/logln methods), but removing 
all log/logln calls?




Converting to j.u.l. wouldn't probably completely remove the 
unnecessary allocation (due to object arrays for varargs) unless you 
guard each log call for the required logging level.


Guards or simply using only single-parameter log methods in j.u.l is 
generally doable workarounds when performance matters so much that even 
the varargs allocation matters. I can only hope 
https://bugs.openjdk.java.net/browse/JDK-8013269 or similar will deal 
with removing that overhead generally, though.


/Claes



-JB- 

Re: RFR: 8028357: Unnecessary allocation in AliasFileParser

[Claes Redestad]

On 2014-12-30 18:35, Jaroslav Bachorik wrote:
The affected files are pretty stable so we could remove *all* the 
logging related methods. When a necessity arises to have a 
configurable logging we would just need to re-introduce it properly. 


Right, I'll leave the patch as-is when it comes to code changes.

This got me thinking - a comment warning about the possible 
performance issues when adding logging could be added as a courtesy to 
the later maintainers.


If so, what should we write? I'm not sure we really need to be this 
cautious in this particular code, and I hope future maintainers will 
care about performance at least as much as we are without well-meaning 
warnings.


I guess something like // 8028357 removed old, inefficient debug 
logging in place of the DEBUG declaration in each affected file 
wouldn't be too busy and also give future maintainers a handle to this 
changeset and thus this discussion. Would that suffice?


/Claes



-JB- 

RFR: 8028357: Unnecessary allocation in AliasFileParser

[Claes Redestad]

Hi,

some classes in jvmstat/perfdata code contains debug logging code 
predating the logging APIs, which

provokes some unnecessary allocation in certain applications.

Since the debug logging can't have been used for quite some time, I 
propose to remove it altogether

rather than converting it to the j.u.l. framework.

bug: https://bugs.openjdk.java.net/browse/JDK-8028357
webrev: http://cr.openjdk.java.net/~redestad/8028357/webrev.01/

/Claes