Re: RFR: 8339134: Callers of Exceptions::fthrow should ensure exception message lengths avoid the INT_MAX limits of os::vsnprintf [v3]
On Wed, 20 Nov 2024 00:33:31 GMT, David Holmes wrote: >> This is mostly an audit of the callers of `Exceptions::fthrow` to ensure >> unbounded strings can't appear. >> >> There is a code change in DiagnosticCmd parsing to extend the string length >> limit already used in part of that code. >> >> Just to clarify the issue. The size 1024 is an internal buffer limit that >> `fthrow` uses - it is an implementation detail and not something the caller >> should think about. It is also not relevant to the underlying problem, which >> is the size of the buffer needed for the fully expanded format string, which >> `os::vsnprintf` will try to calculate and report. The intent is to check >> callers can't hit that underlying `vsnprintf` INT_MAX limit. When your >> format string only deals with a few symbols and symbols are always < 64K >> then we know we are nowhere near that INT_MAX limit. If your format string >> can take a potentially arbitrary (usually from outside) string then it needs >> to put its own size guard in place using `%*s`. >> >> Testing: >> - tier 1-3 (sanity) >> >> Thanks > > David Holmes 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 8339134-fthrow > - Restore previous behaviour for zero length strings > - 8339134: Callers of Exceptions::fthrow should ensure exception message > lengths avoid the INT_MAX limits of os::vsnprintf OK, LGTM - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21867#pullrequestreview-2459486538
Re: RFR: 8330606: Redefinition doesn't but should verify the new klass [v3]
On Fri, 15 Nov 2024 15:30:05 GMT, Coleen Phillimore wrote: >> This change adds a couple of comments, removes some ancient bootstrapping >> code, and adds an old test that we call the verifier for redefining a class, >> even one in the bootclass path. >> >> The fix for always verifying redefined classes was actually pushed with >> JDK-8341094, where the verifier code respected the parameter >> "should_verify_class". By default this parameter follows the -Xverify >> setting. For redefinition, this is passed as true. The rest of the fix >> removes the special bootstrap loader cases that may have failed early on in >> the JDK development with -Xverify:all but now no longer do. If someone >> tries to redefine these classes, they should also do the verification on the >> redefined bytecodes. >> >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Reduce test, fix bug in verifier, move and add comments to > is_eligible_for_verification. LGTM - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22116#pullrequestreview-2451178078
Re: RFR: 8330606: Redefinition doesn't but should verify the new klass [v2]
On Thu, 14 Nov 2024 21:35:52 GMT, Coleen Phillimore wrote: >> This change adds a couple of comments, removes some ancient bootstrapping >> code, and adds an old test that we call the verifier for redefining a class, >> even one in the bootclass path. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Updated the test with some comments. Hi Coleen, I agree with David here, I've no clue how this fixes the problem :). Could you expand on how this fix works? src/hotspot/share/classfile/verifier.cpp line 114: > 112: } > 113: > 114: // This method determines whether we allow package access in access > checks in reflection. Style: "This method" prefix isn't necessary. Maybe just "Determine whether..."? or "Do we ...?" or "Returns true if and only if ..." - PR Review: https://git.openjdk.org/jdk/pull/22116#pullrequestreview-2438240723 PR Review Comment: https://git.openjdk.org/jdk/pull/22116#discussion_r1843558145
Withdrawn: 8335701: Make GrowableArray templated by an Index
On Thu, 4 Jul 2024 11:34:23 GMT, Johan Sjölen wrote: > Hi, > > Today the GrowableArray has a set index type of `int`, this PR makes it so > that you can set your own index type through a template parameter. > > This opens up for a few new design choices: > > - Do you know that you have a very small array? Use an `uint8_t` for len and > cap, each. > - Do you have a very large one? Use an `uint64_t`. > > The code has opted for `int` being default, as to keep identical semantics > for all existing code and to let users not have to worry about the index if > they don't care. > > One "major" change that I don't want to get lost in the review: I've changed > the mid-point calculation to be overflow insensitive without casting. > > > > // Old > mid = ((max + min) / 2); > // New > mid = min + ((max - min) / 2); > > Some semi-rigorous thinking: > min \in [0, len) > max \in [0, len) > min <= max > max - min / 2 \in [0, len/2) > Maximizing min and max => len + 0 > Maximizing max, minimizing min => len/2 > Minimizing max, maximizing min => max = min => min > > > // Proof that they're identical when m, h, l \in N > (1) m = l + (h - l) / 2 <=> > 2m = 2l + h - l = h + l > > (2) m = (h + l) / 2 <=> > 2m = h + l > (1) = (2) > QED This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/20031
Re: RFR: 8342860: Fix more NULL usage backsliding
On Fri, 1 Nov 2024 13:35:56 GMT, theoweidmannoracle wrote: > - Changed several "NULL" in comments to "null" > - Changed several `NULL` in code to `nullptr` Thank you, these changes looks good to me. - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21826#pullrequestreview-241584
Re: RFR: 8304824: NMT should not use ThreadCritical [v9]
On Mon, 4 Nov 2024 14:52:06 GMT, Robert Toyonaga wrote: >>>This include is not needed because there are no uses that require the >>>definition of Thread. >> >> Right, seems like the forward declaration used to be provided by >> `memory/allocation.hpp`. Let's get rid of the include and use a forward >> declaration of our own instead. > > Should I open another PR to remove it? I can also remove the locking related > to NMT in arena.cpp in the same PR as well (as per [this > comment](https://github.com/openjdk/jdk/pull/20852#issuecomment-2450515494)). I've fixed that particular bug and integrated it already :-). - PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1827882920
Re: RFR: 8304824: NMT should not use ThreadCritical [v9]
On Fri, 1 Nov 2024 21:56:36 GMT, Markus Grönlund wrote: >This include is not needed because there are no uses that require the >definition of Thread. Right, seems like the forward declaration used to be provided by `memory/allocation.hpp`. Let's get rid of the include and use a forward declaration of our own instead. - PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1827311285
Re: RFR: 8304824: NMT should not use ThreadCritical [v9]
On Thu, 31 Oct 2024 10:53:12 GMT, Thomas Stuefe wrote: > I had to analyze this again, to understand why we need this locking, since my > mind slips. > > I updated my findings in https://bugs.openjdk.org/browse/JDK-8325890 . Please > see details there. > > But I don't think the current locking makes much sense, tbh (even before this > patch). Or I don't get it. > > We have three counters in play here: A) the `malloc[mtChunk]` malloc counter > for the mtChunk tag B) the global malloc counter C) the `arena[xxx]` arena > counter for the mtXXX tag the arena is tagged with > > Upon arena growth, we incremement all four. In two steps - first (A) and (B) > under os::malloc, then (C) in the arena code. > > Upon arena death, we may delete the chunk, in which case we adjust all three > counters as well. > > But since we don't want (B) to include arena sizes, we have several places > where we lazily adjust (B) and (A) from the sum of all arena counters (see > JBS issue). These adjustments must be synchronized with arena allocations. > But we don't do this. The lock does not include (C). For that, the lock would > have to be at a different place, inside arena code. We only lock around (A) > and (B). > > I am not sure what the point of this whole thing is. I am also not convinced > that it always works correctly. This whole "updating counters lazily" > business makes the code brittle. > > Also, the current locking does not guarantee correctness anyway. There are > several places in the coding where we calculate e.g. the sum of all mallocs, > but malloc counters are modified (rightly so) out of lock protection. > > To make matters even more complex, we have not two locks here but three: > > * ThreadCritical > > * the new `NmtVirtualMemory_lock` > > * we also have the `NMTQuery_lock`, which guards NMT reports from jcmd > exclusively > > > Not sure what the point of the latter even is. It certainly does not prevent > us from getting reports while underlying data structures are being changed, > unless I am overlooking something. > > It would be very nice if someone other than me could analyze this. If this is so brittle and complex, then I wonder what you even get out of us doing the `ThreadCritical` trick. In other words, if we just removed it, would anyone notice and be able to discern what's occurred? Open question, I might do that change and run the tests on it. - PR Comment: https://git.openjdk.org/jdk/pull/20852#issuecomment-2449601108
Re: RFR: 8304824: NMT should not use ThreadCritical [v3]
On Wed, 30 Oct 2024 13:39:00 GMT, Robert Toyonaga wrote: >I'm not certain, but looking at it again, it seems that the ThreadCritical >uses in ChunkPool::deallocate_chunk and ChunkPool::prune() are only needed for >NMT and are independent of the other ThreadCritical uses in that code. At least for prune, that's needed for the chunk pool itself as it would otherwise be accessed concurrently. >Also, those uses of ThreadCritical are not protecting NMT's model of virtual >memory. They are only protecting the reporting of malloc info specifically. >Although, it may still make sense to replace those instances with >NmtVirtualMemory_lock since it's is still related to NMT. OK, I understand. That seems reasonable. - PR Comment: https://git.openjdk.org/jdk/pull/20852#issuecomment-2447477881
Re: RFR: 8304824: NMT should not use ThreadCritical [v3]
On Wed, 18 Sep 2024 05:53:05 GMT, Thomas Stuefe wrote:
>> Hi @roberttoyonaga
>>
>> (Pinging @afshin-zafari and @jdksjolen)
>>
>> First off, it is good that you ran into this since it highlights a possible
>> real deadlock. Both locks are quite coarse. I cannot think from the top of
>> my head of a scenario where we hold the tty lock and need NMT locking, but
>> would not bet money on it. E.g. I bet we call malloc under ttylock (if only
>> as a side effect of calling functions that return resource area). Malloc
>> does not need locking; but its brittle and small changes can cause deadlocks
>> (e.g. folks were thinking about moving ResourceArea memory to mmap, which
>> needs NMT locks for tracking).
>>
>>> Hi @tstuefe, it looks like calling
>>> [`os::print_memory_mappings`](https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L3785)
>>> from the windows implementation of `os::pd_release_memory` is causing the
>>> rank conflict between `tty_lock` and `NMT_lock`. This is getting called
>>> from `os::release_memory` [**after** we've already acquired the lock for
>>> NMT](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/os.cpp#L2185-L2189).
>>>
>>> gtest `release_bad_ranges` --> `os::release_memory` and acquire `NMT_lock`
>>> --> `os::pd_release_memory` --> `os::print_memory_mappings` and acquire
>>> `tty_lock`
>>>
>>
>> Ah thank you. I think I added that printing ages ago. I should not have used
>> tty. The immediate solution to get you going may be to just change this from
>> tty to `fileStream fs(stderr);`
>>
>>> Is there a reason we cast a wider net and lock from outside of NMT code in
>>> `os::release_memory`, `os::uncommit_memory`, and `os::unmap_memory`? By
>>> contrast, in other functions such as `os::commit_memory` and
>>> `os::reserve_memory` we lock inside of `MemTracker` instead. For example,
>>> `pd_release_memory` is protected by the NMT lock, but `pd_reserve_memory`
>>> is not. Is the lock meant to synchronize the actual memory operations as
>>> well as NMT recording in some cases? If not, then maybe it makes sense to
>>> delay and move the lock acquisition inside `MemTracker` for consistency and
>>> to reduce surface area for lock conflicts.
>>
>> The problem is that both tty_lock and the NMT lock are quite coarse. I
>> always maintained that tty_lock is too coarse - we have many places where
>> there is a ttyLocker somewhere at the start of a printing function, then the
>> function goes and prints tons of infos it *retrieves under lock protection*.
>> The better way to solve this is to assemble the text outside of lock
>> protection, then only do the printing...
>
>> Hi @tstuefe,
>>
>> > Ah thank you. I think I added that printing ages ago. I should not have
>> > used tty. The immediate solution to get you going may be to just change
>> > this from tty to fileStream fs(stderr);
>>
>> Thanks! I adopted your suggestion and replaced tty. The lock conflict is
>> gone now.
>>
>> > Ensuring that we feed NMT the tracking information in the order they
>> > happened. This is only relevant for mmap accounting, since only there do
>> > we need to track address ranges. For malloc accounting, we just
>> > increase/decrease counters, and there, the order of operations does not
>> > matter, just that we do it atomically.
>> > ...
>> > A pragmatic solution would be to move the locking down the stack.
>> > Currently, we lock in the generic layer around { pd_release_memory() +
>> > MemTracker::record }. But we only need to lock around the OS allocation
>> > function (mmap/munmap/VirtualAlloc etc). So we could move the locks
>> > downward to the OS specific layer where the actual allocations happen in
>> > the OS code, e.g. just lock around { munmap() + MemTracker::record }.
>>
>> Ok I see. So the locking should indeed not happen inside of `MemTracker`.
>> Yes, maybe moving the locking down the stack into platform specific code
>> would be a good task for the future. However, if the intention is to feed
>> NMT the tracking info in the order it happened, why are the operations on
>> memory in `os::commit_memory` and `os::reserve_memory` not protected by the
>> lock? In those, we delay and lock inside `MemTracker` instead.
>
> That looks like an error that should be fixed.
Hi @tstuefe, @roberttoyonaga
We saw some test failures in non-generational ZGC due to this change. That GC
is removed now, so there's no worry there. I did however look at all of our
remaining usages of `ThreadCritical` and saw that there are thrre left that are
related to NMT:
src/hotspot/share/nmt/nmtUsage.cpp
56 ThreadCritical tc;
src/hotspot/share/nmt/mallocTracker.cpp
65 // Use ThreadCritical to make sure that mtChunks don't get deallocated
while the
68 ThreadCritical tc;
src/hotspot/share/memory/arena.cpp
178 ThreadCritical tc; // Free chunks under TC lock so that NMT
adjustment is stable.
I am not th
Re: RFR: 8304824: NMT should not use ThreadCritical [v8]
On Wed, 2 Oct 2024 13:28:13 GMT, Robert Toyonaga wrote: >> ### Summary >> This PR just replaces `ThreadCritical` with a lock specific to NMT. >> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. >> I've implemented the new lock with a semaphore so that it can be used early >> before VM init. There is also the possibility of adding assertions in >> places we expect NMT to have synchronization. I haven't added assertions yet >> in many places because some code paths such as the (NMT tests) don't lock >> yet. I think it makes sense to close any gaps in locking in another PR in >> which I can also add more assertions. >> >> Testing: >> - hotspot_nmt >> - gtest:VirtualSpace >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > Update src/hotspot/share/utilities/vmError.cpp > > Co-authored-by: David Holmes <62092539+dholmes-...@users.noreply.github.com> Hi, Big thanks to David for taking the heavy brunt of reviewing this change. This code looks good to me, it's a bit unfortunate that we have to export `MemTracker::reduce_tracking_to_summary` for the limited use case of exiting the VM on error, but OK. Let's wait for Thomas to come back for this change, I think it's not more than fair that he gets to have another look at this. - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20852#pullrequestreview-2356793662
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v5]
On Mon, 1 Jul 2024 12:55:24 GMT, Kevin Walls wrote: >> Sebastian Lövdahl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Adapt code style > > (!havePidNSes && nsPid > 1) > I didn't get this at first, I think it's because PID 1 can't have a parent? > (in the same namespace) > Ah, maybe @kevinjwalls needs to approve the PR again before it can be > integrated? IIRC the only change since that approval was these comment > changes: [#19055 > (comment)](https://github.com/openjdk/jdk/pull/19055#issuecomment-2202761731) Correct. Any changes have to be re-approved nowadays, it's a fairly recent development. - PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2381395550
Re: RFR: 8340363: Tag-specific default decorators for UnifiedLogging [v4]
On Tue, 24 Sep 2024 16:43:57 GMT, Antón Seoane wrote: >> Hi all, >> >> Currently, the Unified Logging framework defaults to three decorators >> (uptime, level, tags) whenever the user does not specify otherwise through >> `-Xlog`. This can result in cumbersome input whenever a specific user that >> relies on a particular tag(s) has some predefined needs. For example, C2 >> developers rarely need decorations, and having to manually specify this >> every time results inconvenient. >> >> To address this, this PR enables the possibility of adding tag-specific >> default decorators to UL. These defaults are in no way overriding user input >> -- they will only act whenever `-Xlog` has no decorators supplied and there >> is a positive match with the pre-specified defaults. Such a match is based >> on the following: >> >> - Inclusion: if `-Xlog:jit+compilation` is provided, a default for `jit` may >> be applied. >> - Specificity: if, for the above line, there is a more specific default for >> `jit+compilation` the latter shall be applied. Upon equal specificity cases, >> both defaults will be applied. >> - Additionally, defaults may target a specific log level. >> >> Decorators are also associated with an output file, so an output device may >> only have one set of decorators. For this reason, if different >> `LogSelection`s trigger defaults, none is to be applied. >> >> In summary, these defaults may be seen as a "tailored" or "flavoured" >> version of the existing "uptime-level-tags" current defaults. >> >> Please consider this PR, and thanks! > > Antón Seoane has updated the pull request incrementally with one additional > commit since the last revision: > > Removed whitespace The main point of importance here is that anything user-specified (from `-Xlog` or `jcmd`) will take priority. We should probably add some information regarding this on the `-Xlog:help` page. - PR Comment: https://git.openjdk.org/jdk/pull/20988#issuecomment-2373534931
Re: RFR: 8340363: User-specified default decorators for UnifiedLogging
On Thu, 19 Sep 2024 01:30:30 GMT, David Holmes wrote: > Finally, this is really subjective. You'd really need to socialise the actual > proposed changes to the defaults independent of any mechanism to allow it. By that you mean the `jit+inlining` default, right? That has been socialized among Oracle's C2 developers if I understand correctly, though it hasn't been done for the wider community. The lack of socialising the changes to the wider the community is an oversight on my part. - PR Comment: https://git.openjdk.org/jdk/pull/20988#issuecomment-2360456868
Re: RFR: 8340363: User-specified default decorators for UnifiedLogging
On Fri, 13 Sep 2024 09:03:55 GMT, Antón Seoane wrote: > Hi all, > > Currently, the Unified Logging framework defaults to three decorators > (uptime, level, tags) whenever the user does not specify otherwise through > `-Xlog`. This can result in cumbersome input whenever a specific user that > relies on a particular tag(s) has some predefined needs. For example, C2 > developers rarely need decorations, and having to manually specify this every > time results inconvenient. > > To address this, this PR enables the possibility of adding tag-specific > default decorators to UL. These defaults are in no way overriding user input > -- they will only act whenever `-Xlog` has no decorators supplied and there > is a positive match with the pre-specified defaults. Such a match is based on > the following: > > - Inclusion: if `-Xlog:jit+compilation` is provided, a default for `jit` may > be applied. > - Specificity: if, for the above line, there is a more specific default for > `jit+compilation` the latter shall be applied. Upon equal specificity cases, > both defaults will be applied. > - Additionally, defaults may target a specific log level. > > Decorators are also associated with an output file, so an output device may > only have one set of decorators. For this reason, if different > `LogSelection`s trigger defaults, none is to be applied. > > In summary, these defaults may be seen as a "tailored" or "flavoured" version > of the existing "uptime-level-tags" current defaults. > > Please consider this PR, and thanks! src/hotspot/share/logging/logDecorators.cpp line 30: > 28: > 29: const LogLevelType AnyLevel = LogLevelType::NotMentioned; > 30: #define DEFAULT_DECORATORS \ I think this should also have the default decorators that UL already have. That is, all data about default decorators is gathered here. - PR Review Comment: https://git.openjdk.org/jdk/pull/20988#discussion_r1766489613
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v18]
On Tue, 17 Sep 2024 09:35:02 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 57 commits:
>
> - fix CompressedClassPointersEncodingScheme yet again for linux aarch64
> - Fixes post-8340184
> - Merge upstream up to and including 8340184
> - Merge remote-tracking branch 'origin/master' into JDK-8305895-v4
> - Fix
> test/hotspot/jtreg/runtime/CompressedOops/CompressedClassPointersEncodingScheme.java
> - Fix loop on aarch64
> - clarify obscure assert in metasapce setup
> - Rework compressedklass encoding
> - remove stray debug output
> - Fixes post 8338526
> - ... and 47 more: https://git.openjdk.org/jdk/compare/7849f252...28a26aed
Hi,
We've gone through the rest of the Metaspace code and looked at the tests. It
looks OK to us. Would like to see some style cleanups in the tests, but that
can wait as a follow up.
test/hotspot/gtest/metaspace/test_clms.cpp line 193:
> 191:
> 192: {
> 193: // Nonclass arena allocation.
The style in this source file isn't really up to scratch, especially *these*
lines. Anyway, it's in the tests, so I'm OK with this being fixed in a follow
up RFE.
-
PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2309360771
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1763005291
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v11]
On Tue, 17 Sep 2024 09:59:49 GMT, Thomas Stuefe wrote:
>> src/hotspot/share/memory/classLoaderMetaspace.hpp line 81:
>>
>>> 79: metaspace::MetaspaceArena* class_space_arena() const { return
>>> _class_space_arena; }
>>> 80:
>>> 81: bool have_class_space_arena() const { return _class_space_arena !=
>>> nullptr; }
>>
>> This is unnecessary. Instead of having this and having to remember to check
>> for nullness each time, just change the `_class_space_arena` to point to the
>> same arena as the `_non_class_space_arena` does when we run with
>> `-XX:-UseCompressedClassPointers`
>
> I'd prefer not to.
>
> This logic (when -UCCP class space arena is NULL, with the implicit
> assumption that both are different entities) has been in there forever, and
> changing that is out of scope for and unrelated to this PR. I am not sure
> what will break if I change this but don't want to risk test errors at this
> point (one example, reporting would have to be adapted to recognize that both
> arenas are the same, and there are plenty of tests that would also need to be
> fixd).
>
> This can be done in a follow-up RFE if necessary.
OK, that's fine.
>> src/hotspot/share/memory/metaspace.cpp line 656:
>>
>>> 654: // Adjust size of the compressed class space.
>>> 655:
>>> 656: const size_t res_align = reserve_alignment();
>>
>> Can you change the name to `root_chunk_size`?
>
> It feels wrong, since this is a deeply hidden implementation detail.\
>
> I will remove this temporary variable, which will also make the diff smaller.
Sounds OK, I wanted the name change to indicate that "hey, deep impl detail
where we use this to mean something else".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1762993568
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1762994772
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
On Tue, 10 Sep 2024 20:53:46 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related function/template >> parameter names and local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with one additional > commit since the last revision: > > Coleen's feedback Hi! I went through all of it and LGTM, one question about copyright notices however. src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.inline.hpp line 2: > 1: /* > 2: * Copyright (c) 2016, 2019, Red Hat, Inc. All rights reserved. I don't think we're meant to update other companies' copyrights? - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20872#pullrequestreview-2299601404 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1756401544
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v11]
On Tue, 10 Sep 2024 19:11:30 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix FullGCForwarding initialization
Hi,
Me and @caspernorrbin are reviewing the Metaspace changes (so anything in the
`memory` and `metaspace` folders). We have found minor improvements that can be
made and some nits, but the code over all looks OK. We are finishing up a first
round of review now, and will have a second one.
Thank you for your hard work and your patience with the review process.
src/hotspot/share/memory/classLoaderMetaspace.cpp line 87:
> 85: klass_alignment_words,
> 86: "class arena");
> 87: }
As per my comment in the header file, change the code to this:
```c++
if (class_context != nullptr) {
// ... Same as in PR
} else {
_class_space_arena = _non_class_space_arena;
}
src/hotspot/share/memory/classLoaderMetaspace.cpp line 115:
> 113: if (wastage.is_nonempty()) {
> 114: non_class_space_arena()->deallocate(wastage);
> 115: }
This code reads a bit strangely. I understand *what* it tries to do. It tries
to give back any wasted memory from either the class space arena *or* the non
class space arena to the non class space arena's freelist. I assume that we do
this since any wastage is presumably too small to be used by our new 22-bit
class pointers. However, this context will be lost on future readers. It should
have at least a comment in the `if (wastage.is_nonempty())` clause explaining
what we expect should happen and why. For example:
```c++
// Any wasted memory is presumably too small for any class.
// Therefore, give it back to the non-class space arena's free list.
src/hotspot/share/memory/classLoaderMetaspace.cpp line 118:
> 116: #ifdef ASSERT
> 117: if (result.is_nonempty()) {
> 118: const bool in_class_arena = class_space_arena() != nullptr ?
> class_space_arena()->contains(result) : false;
Unnecessary nullptr check if you take my suggestion, or you should switch to
`have_class_space_arena`.
src/hotspot/share/memory/classLoaderMetaspace.cpp line 165:
> 163: MetaBlock bl(ptr, word_size);
> 164: // If the block would be reusable for a Klass, add to class arena,
> otherwise to
> 165: // then non-class arena.
Nit: spelling, "the"
src/hotspot/share/memory/classLoader
Re: RFR: 8304824: NMT should not use ThreadCritical
On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga wrote: > ### Summary > This PR just replaces `ThreadCritical` with a lock specific to NMT. > `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. > I've implemented the new lock with a semaphore so that it can be used early > before VM init. There is also the possibility of adding assertions in places > we expect NMT to have synchronization. I haven't added assertions yet in many > places because some code paths such as the (NMT tests) don't lock yet. I > think it makes sense to close any gaps in locking in another PR in which I > can also add more assertions. > > Testing: > - hotspot_nmt > - gtest:VirtualSpace > - tier1 Thank you Robert. I think that the `MemoryFileTracker`'s locker should probably also be replaced with this semaphore, as in the future we have plans to have a globally shared `NativeCallStackStorage`. - PR Comment: https://git.openjdk.org/jdk/pull/20852#issuecomment-2333565898
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) NMTGroup 3 points, NMTCat 2 points are my picks (NMTCategory is even better than NMTCat, imho). No 1 point given, I want these to win :P. Thank you for the effort with this table, may the highest point taker win :-). - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2314753974
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" LGTM - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20006#pullrequestreview-2204728692
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" FWIW, the Python code maps closely to the standard C/POSIX socket API, it was just a bit quicker to write. - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2255536277
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote:
> CmdLine::is_executable() looks wrong, surely an empty line is not executable.
>
> With this change, in DCmd::parse_and_execute() we will avoid needlessly
> entering the code block to try and execute the command.
>
> DCmd tests all good:
> make images test TEST="test/hotspot/jtreg/serviceability/dcmd
> test/jdk/sun/tools/jcmd"
At the point where this is checked, the commandline string is still not parsed.
I do not want any `assert`s regarding the structure of untrusted input. Anyway,
PoC of getting "empty" lines such that `is_empty()` is true.
I read the code:
```c++
// diagnosticFramework.cpp:387
DCmdIter iter(cmdline, '\n'); // <--- Delimiter right there
Hypothesis: Multiple newlines in a message will lead to empty lines.
First attempt: I used `jcmd` directly, this failed, as `jcmd` does some clean
up on its own. So I had to go with connecting to the PID using Python.
My Python 'attacker':
import socket
import os
# My Java PID, connect with JCMD ordinarily to your Java process first to
create the PID file, then
# replace the numbers with your Java PID
socket_path = "/proc/2603121/root/tmp/.java_pid2603121"
client = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
print("Connect")
print(client.connect(socket_path))
# // The request is:
# // 0
# // where is the protocol version (1), is the command
# // name ("load", "datadump", ...), and is an argument
connect_payload = "1\x00jcmd\x00\n\n\n\x00\n\n\n\x00\n\n\n\x00"
print("Sending payload")
print(client.sendall(connect_payload.encode()))
print("Sent")
# Receive a response from the server
response = client.recv(4096)
print(f'Received response: {response.decode()}')
Later on, in a gdb session for my Java program and where I've connected to the
Java process using my Python program:
(gdb) p line.is_empty()
$6 = true
(gdb) p cmdline
$7 = 0x7fff68240fc9 "\n\n\n"
(gdb) p line
$8 = { = {}, _cmd = 0x7fff68240fc9 "\n\n\n", _cmd_len
= 0,
_args = 0x7fff68240fc9 "\n\n\n", _args_len = 0}
(gdb) p bt
No symbol "bt" in current context.
(gdb) bt
#0 DCmd::parse_and_execute (source=DCmd_Source_AttachAPI, out=0x7fffaa62ecc0,
cmdline=0x7fff68240fc9 "\n\n\n",
delim=32 ' ', __the_thread__=0x7fff8c001010)
at
/home/johan/jdk/open/src/hotspot/share/services/diagnosticFramework.cpp:399
#1 0x758f8748 in jcmd (op=0x7fff68240fb0, out=0x7fffaa62ecc0)
at /home/johan/jdk/open/src/hotspot/share/services/attachListener.cpp:209
#2 0x758f91ae in AttachListenerThread::thread_entry
(thread=0x7fff8c001010, __the_thread__=0x7fff8c001010)
at /home/johan/jdk/open/src/hotspot/share/services/attachListener.cpp:418
#3 0x76036294 in JavaThread::thread_main_inner (this=0x7fff8c001010)
at /home/johan/jdk/open/src/hotspot/share/runtime/javaThread.cpp:757
#4 0x76036129 in JavaThread::run (this=0x7fff8c001010)
at /home/johan/jdk/open/src/hotspot/share/runtime/javaThread.cpp:742
#5 0x767c5fb7 in Thread::call_run (this=0x7fff8c001010)
at /home/johan/jdk/open/src/hotspot/share/runtime/thread.cpp:225
#6 0x76549e28 in thread_native_entry (thread=0x7fff8c001010)
at /home/johan/jdk/open/src/hotspot/os/linux/os_linux.cpp:858
#7 0x77c94ac3 in start_thread (arg=) at
./nptl/pthread_create.c:442
#8 0x77d26850 in clone3 () at
../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb)
Oh, and also, the output of my Python running:
Connect
None
Sending payload
None
Sent
Received response: -1
java.lang.IllegalArgumentException: Unknown diagnostic command
-
PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2254454215
PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2254454540
Re: RFR: 8335701: Make GrowableArray templated by an Index [v3]
On Fri, 5 Jul 2024 07:38:12 GMT, Glavo wrote: >> Johan Sjölen has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - Fix >> - Apparently this(!) >> - This? >> - Use COMMA > > src/hotspot/share/classfile/classFileParser.hpp line 46: > >> 44: class ConstMethod; >> 45: class FieldInfo; >> 46: template > > Suggestion: > > template > > > Is it possible to reduce the changes by providing default parameters? Unfortunately, no. Forward decl.s may not re-define the default template argument, even though they are the same as the definition. - PR Review Comment: https://git.openjdk.org/jdk/pull/20031#discussion_r1666460897
Re: RFR: 8335701: Make GrowableArray templated by an Index [v2]
On Thu, 4 Jul 2024 13:35:36 GMT, Johan Sjölen wrote: >> Hi, >> >> Today the GrowableArray has a set index type of `int`, this PR makes it so >> that you can set your own index type through a template parameter. >> >> This opens up for a few new design choices: >> >> - Do you know that you have a very small array? Use an `uint8_t` for len and >> cap, each. >> - Do you have a very large one? Use an `uint64_t`. >> >> The code has opted for `int` being default, as to keep identical semantics >> for all existing code and to let users not have to worry about the index if >> they don't care. >> >> One "major" change that I don't want to get lost in the review: I've changed >> the mid-point calculation to be overflow insensitive without casting. >> >> >> >> // Old >> mid = ((max + min) / 2); >> // New >> mid = min + ((max - min) / 2); >> >> Some semi-rigorous thinking: >> min \in [0, len) >> max \in [0, len) >> min <= max >> max - min / 2 \in [0, len/2) >> Maximizing min and max => len + 0 >> Maximizing max, minimizing min => len/2 >> Minimizing max, maximizing min => max = min => min >> >> >> // Proof that they're identical when m, h, l \in N >> (1) m = l + (h - l) / 2 <=> >> 2m = 2l + h - l = h + l >> >> (2) m = (h + l) / 2 <=> >> 2m = h + l >> (1) = (2) >> QED > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Attempt at fixing GA VMStruct Compiler issue in linux-x86 seems unrelated to this change. - PR Comment: https://git.openjdk.org/jdk/pull/20031#issuecomment-2252687347
Re: RFR: 8335701: Make GrowableArray templated by an Index [v2]
On Thu, 4 Jul 2024 14:07:57 GMT, Thomas Stuefe wrote: > If this is for src/hotspot/share/nmt/arrayWithFreeList.hpp, would it not be a > lot simpler to just implement it there, and give it another backing store? > > In particular because after doing all this work it still won't even support > the feature I was hoping for, mainly the ability to put an indexed free list > atop of existing memory. I did that first and it sure is simpler, but I'm not sure whether it's a good idea to have to support such a backing storage. See `resizable_array` in here: https://github.com/openjdk/jdk/pull/20002 Still, it does do what you asked for, kind of, see: `GrowableArrayFromArray`. I can adapt AWFL to be able to use either `GAFA` or `GA`. It's also not the only reason to do this. - PR Comment: https://git.openjdk.org/jdk/pull/20031#issuecomment-2209118236
Re: RFR: 8335701: Make GrowableArray templated by an Index [v3]
> Hi, > > Today the GrowableArray has a set index type of `int`, this PR makes it so > that you can set your own index type through a template parameter. > > This opens up for a few new design choices: > > - Do you know that you have a very small array? Use an `uint8_t` for len and > cap, each. > - Do you have a very large one? Use an `uint64_t`. > > The code has opted for `int` being default, as to keep identical semantics > for all existing code and to let users not have to worry about the index if > they don't care. > > One "major" change that I don't want to get lost in the review: I've changed > the mid-point calculation to be overflow insensitive without casting. > > > > // Old > mid = ((max + min) / 2); > // New > mid = min + ((max - min) / 2); > > Some semi-rigorous thinking: > min \in [0, len) > max \in [0, len) > min <= max > max - min / 2 \in [0, len/2) > Maximizing min and max => len + 0 > Maximizing max, minimizing min => len/2 > Minimizing max, maximizing min => max = min => min > > > // Proof that they're identical when m, h, l \in N > (1) m = l + (h - l) / 2 <=> > 2m = 2l + h - l = h + l > > (2) m = (h + l) / 2 <=> > 2m = h + l > (1) = (2) > QED Johan Sjölen has updated the pull request incrementally with four additional commits since the last revision: - Fix - Apparently this(!) - This? - Use COMMA - Changes: - all: https://git.openjdk.org/jdk/pull/20031/files - new: https://git.openjdk.org/jdk/pull/20031/files/b5a87422..937f6eb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20031&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20031&range=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20031.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20031/head:pull/20031 PR: https://git.openjdk.org/jdk/pull/20031
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]
On Mon, 22 Jul 2024 20:02:40 GMT, Sonia Zaldana Calles wrote: >> @SoniaZaldana Note that this is very much optional. > > Hi folks, thanks for the pointers! I wasn't familiar with X macros and after > some time toying around with them, I'm sad to report that I am not a fan > (yet!). > > I implemented it and ended up breaking part of the tests. I quickly realized > that debugging these is a bit harder for less experienced c++ developers > (like myself). > > So, just wanted to note: > - I cleaned up the indentation in this function as it was all wrong. > - I didn't get rid of the repetition. Tried to but quickly realized we can't > pull the DCmdArgument out of the if statements as they're different types. > > And note to self, to keep reviewing X macros because they did shorten the > code a lot when I implemented them. Perhaps I'll give it another go in a > different RFE. > > Sorry it's not what either of you hoped for! That's fine, thanks for having a go! - PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1687949210
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]
On Sat, 20 Jul 2024 12:06:33 GMT, Thomas Stuefe wrote:
>> On top: We hug the `*`s next to the type in Hotspot, not next to the var
>> name. So `DCmdArgument* argument`. This is something to check
>> for all new code.
>>
>> Pre-existing: The indentation of the if-block is wrong.
>>
>> Also, @SoniaZaldana, would you mind changing the code to this (does not
>> include your change), the repetition just made me cringe 😅.
>>
>> ```c++
>> DCmdArgument* argument = nullptr;
>> if (strcmp(type, "STRING") == 0) {
>> argument = new DCmdArgument(name, desc, "STRING", mandatory,
>> default_value);
>> } else if (strcmp(type, "NANOTIME") == 0) {
>> DCmdArgument* argument = new
>> DCmdArgument(name, desc, "NANOTIME", mandatory,
>> default_value);
>> } else if (strcmp(type, "JLONG") == 0) {
>> DCmdArgument* argument = new DCmdArgument(name, desc,
>> "JLONG", mandatory, default_value);
>> } else if (strcmp(type, "BOOLEAN") == 0) {
>> DCmdArgument* argument = new DCmdArgument(name, desc,
>> "BOOLEAN", mandatory, default_value);
>> } else if (strcmp(type, "MEMORYSIZE") == 0) {
>> DCmdArgument* argument = new
>> DCmdArgument(name, desc, "MEMORY SIZE", mandatory,
>> default_value);
>> } else if (strcmp(type, "STRINGARRAY") == 0) {
>> DCmdArgument* argument = new
>> DCmdArgument(name, desc, "STRING SET", mandatory);
>> }
>>
>> if (argument != nullptr) {
>> if (isarg) {
>> parser->add_dcmd_argument(argument);
>> } else {
>> parser->add_dcmd_option(argument);
>> }
>> }
>
> @jdksjolen
>
>> Also, @SoniaZaldana, would you mind changing the code to this
>
> Even simpler (did not test, but you get my drift):
>
>
> #define ALL_TYPES_DO_XX(what) \
> what(char*, "STRING") \
> what(NanoTimeArgument, NANOTIME) \
> what(jlong, "JLONG")
> ... etc
>
> then
>
>
> #define XX(TYPE, NAME) \
> if (strcmp(type, NAME) == 0) { \
> DCmdArgument* argument = new DCmdArgument(name, desc, NAME,
> mandatory, mandatory, default_value); \
> }
> ALL_TYPES_DO_XX(XX)
> #undef XX
>
>
> ;-)
Sonia, my bad if you already know this stuff but since it's fairly esoteric
knowledge nowadays I'd like to help you out in advance: Thomas is proposing the
usage of a X macro https://en.wikipedia.org/wiki/X_macro
These can be found throughout Hotspot, you can find an example definition and
usage in `logTag.hpp` and `logTag.cpp`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685688287
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]
On Fri, 19 Jul 2024 19:17:54 GMT, Leonid Mesnik wrote:
>> Sonia Zaldana Calles has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Missing copyright header update
>
> src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 132:
>
>> 130:} else if (strcmp(type, "FILE") == 0) {
>> 131: DCmdArgument *argument =
>> 132: new DCmdArgument(name, desc, "FILE", mandatory);
>
> Please check indentation.
On top: We hug the `*`s next to the type in Hotspot, not next to the var name.
So `DCmdArgument* argument`. This is something to check for all
new code.
Pre-existing: The indentation of the if-block is wrong.
Also, @SoniaZaldana, would you mind changing the code to this (does not include
your change), the repetition just made me cringe 😅.
```c++
DCmdArgument* argument = nullptr;
if (strcmp(type, "STRING") == 0) {
argument = new DCmdArgument(name, desc, "STRING", mandatory,
default_value);
} else if (strcmp(type, "NANOTIME") == 0) {
DCmdArgument* argument = new
DCmdArgument(name, desc, "NANOTIME", mandatory,
default_value);
} else if (strcmp(type, "JLONG") == 0) {
DCmdArgument* argument = new DCmdArgument(name, desc,
"JLONG", mandatory, default_value);
} else if (strcmp(type, "BOOLEAN") == 0) {
DCmdArgument* argument = new DCmdArgument(name, desc,
"BOOLEAN", mandatory, default_value);
} else if (strcmp(type, "MEMORYSIZE") == 0) {
DCmdArgument* argument = new
DCmdArgument(name, desc, "MEMORY SIZE", mandatory,
default_value);
} else if (strcmp(type, "STRINGARRAY") == 0) {
DCmdArgument* argument = new
DCmdArgument(name, desc, "STRING SET", mandatory);
}
if (argument != nullptr) {
if (isarg) {
parser->add_dcmd_argument(argument);
} else {
parser->add_dcmd_option(argument);
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685384131
Re: RFR: 8335701: Make GrowableArray templated by an Index [v2]
On Thu, 4 Jul 2024 13:35:36 GMT, Johan Sjölen wrote: >> Hi, >> >> Today the GrowableArray has a set index type of `int`, this PR makes it so >> that you can set your own index type through a template parameter. >> >> This opens up for a few new design choices: >> >> - Do you know that you have a very small array? Use an `uint8_t` for len and >> cap, each. >> - Do you have a very large one? Use an `uint64_t`. >> >> The code has opted for `int` being default, as to keep identical semantics >> for all existing code and to let users not have to worry about the index if >> they don't care. >> >> One "major" change that I don't want to get lost in the review: I've changed >> the mid-point calculation to be overflow insensitive without casting. >> >> >> >> // Old >> mid = ((max + min) / 2); >> // New >> mid = min + ((max - min) / 2); >> >> Some semi-rigorous thinking: >> min \in [0, len) >> max \in [0, len) >> min <= max >> max - min / 2 \in [0, len/2) >> Maximizing min and max => len + 0 >> Maximizing max, minimizing min => len/2 >> Minimizing max, maximizing min => max = min => min >> >> >> // Proof that they're identical when m, h, l \in N >> (1) m = l + (h - l) / 2 <=> >> 2m = 2l + h - l = h + l >> >> (2) m = (h + l) / 2 <=> >> 2m = h + l >> (1) = (2) >> QED > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Attempt at fixing GA VMStruct Always fun to grapple with vmStructs, moving this back to draft. - PR Comment: https://git.openjdk.org/jdk/pull/20031#issuecomment-2209022728
Re: RFR: 8335701: Make GrowableArray templated by an Index [v2]
> Hi, > > Today the GrowableArray has a set index type of `int`, this PR makes it so > that you can set your own index type through a template parameter. > > This opens up for a few new design choices: > > - Do you know that you have a very small array? Use an `uint8_t` for len and > cap, each. > - Do you have a very large one? Use an `uint64_t`. > > The code has opted for `int` being default, as to keep identical semantics > for all existing code and to let users not have to worry about the index if > they don't care. > > One "major" change that I don't want to get lost in the review: I've changed > the mid-point calculation to be overflow insensitive without casting. > > > > // Old > mid = ((max + min) / 2); > // New > mid = min + ((max - min) / 2); > > Some semi-rigorous thinking: > min \in [0, len) > max \in [0, len) > min <= max > max - min / 2 \in [0, len/2) > Maximizing min and max => len + 0 > Maximizing max, minimizing min => len/2 > Minimizing max, maximizing min => max = min => min > > > // Proof that they're identical when m, h, l \in N > (1) m = l + (h - l) / 2 <=> > 2m = 2l + h - l = h + l > > (2) m = (h + l) / 2 <=> > 2m = h + l > (1) = (2) > QED Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Attempt at fixing GA VMStruct - Changes: - all: https://git.openjdk.org/jdk/pull/20031/files - new: https://git.openjdk.org/jdk/pull/20031/files/7407a151..b5a87422 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20031&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20031&range=00-01 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20031.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20031/head:pull/20031 PR: https://git.openjdk.org/jdk/pull/20031
RFR: 8335701: Make GrowableArray templated by an Index
Hi, Today the GrowableArray has a set index type of `int`, this PR makes it so that you can set your own index type through a template parameter. This opens up for a few new design choices: - Do you know that you have a very small array? Use an `uint8_t` for len and cap, each. - Do you have a very large one? Use an `uint64_t`. The code has opted for `int` being default, as to keep identical semantics for all existing code and to let users not have to worry about the index if they don't care. One "major" change that I don't want to get lost in the review: I've changed the mid-point calculation to be overflow insensitive without casting. // Old mid = ((max + min) / 2); // New mid = min + ((max - min) / 2); Some semi-rigorous thinking: min \in [0, len) max \in [0, len) min <= max max - min / 2 \in [0, len/2) Maximizing min and max => len + 0 Maximizing max, minimizing min => len/2 Minimizing max, maximizing min => max = min => min // Proof that they're identical when m, h, l \in N (1) m = l + (h - l) / 2 <=> 2m = 2l + h - l = h + l (2) m = (h + l) / 2 <=> 2m = h + l (1) = (2) QED - Commit messages: - Fix spelling and actually include the growableArray is it used in cpp file - Move - Handle unhandled oops - Make GrowableArray receive an Index type optionally Changes: https://git.openjdk.org/jdk/pull/20031/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20031&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335701 Stats: 262 lines in 32 files changed: 19 ins; 25 del; 218 mod Patch: https://git.openjdk.org/jdk/pull/20031.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20031/head:pull/20031 PR: https://git.openjdk.org/jdk/pull/20031
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga wrote:
> ### Summary
> This change ensures we don't get undefined behavior when
> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html).
> `isspace` accepts an `int` argument that "the application shall ensure is a
> character representable as an unsigned char or equal to the value of the
> macro EOF.".
>
> Previously, there was no checking of the values passed to `isspace`. I've
> replaced direct calls with a new wrapper `os::is_space` that performs a range
> check and prevents the possibility of undefined behavior from happening. For
> instances outside of Hotspot, I've added casts to `unsigned char`.
>
> **Testing**
> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check
> `os::is_space` is working correctly.
> - tier1
Hi Robert,
Here's a third opinion:
The wrapper is fine, but it should mimic the name of the function it wraps
exactly: `os::isspace`. It's also good that it does the range check as regular
code as opposed to an assert, as this function is used to parse external input.
It's fine to be excessive when parsing strings as those parts of the code are
not really performance critical.
All the best,
Johan
src/hotspot/share/runtime/os.cpp line 96:
> 94: DEBUG_ONLY(bool os::_mutex_init_done = false;)
> 95:
> 96: int os::is_space(int c) {
`os::isspace`
test/hotspot/gtest/runtime/test_os.cpp line 43:
> 41:
> 42: # include
> 43:
Not necessary.
-
PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2107152152
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1632873691
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1632873309
Re: RFR: 8333566: Remove unused methods
On Tue, 4 Jun 2024 20:51:52 GMT, Cesar Soares Lucas wrote: > Please, consider this patch to remove unused methods from the code base. To > the best of my knowledge, these methods are only defined but never used. > > Here is a list with names of delete methods: > https://gist.github.com/JohnTortugo/fccc29781a1b584c03162aa4e160e874 > > Tested with Linux x86_64 tier1-4, GHA, and only cross building to other > platforms. Hi, Removing dead code is great, but it has to be done with reasoning behind it and preferably in smaller batches, so as to be reviewable. I also don't understand why you comment out tests. If you can make these into smaller and more localized PRs, then I'll be happy to take a look if I think I'm the right person to review it. All the best, Johan - PR Comment: https://git.openjdk.org/jdk/pull/19550#issuecomment-2154676801
Re: RFR: 8333047: Remove arena-size-workaround in jvmtiUtils.cpp
On Tue, 28 May 2024 12:36:41 GMT, Thomas Stuefe wrote: > In `JvmtiUtil::single_threaded_resource_area()`, we create a resource area > that is supposed to work even if the current thread is not attached yet and > there is no associated Thread or the Thread has no valid ResourceArea. > > It contains a workaround: > > > // lazily create the single threaded resource area > // pick a size which is not a standard since the pools don't exist yet > _single_threaded_resource_area = new (mtInternal) > ResourceArea(Chunk::non_pool_size); > > > It specifies a non-standard chunk size to circumvent the chunk-pool-based > allocation in the RA constructor, ensuring that only malloc is used. This is > because in the old days the ChunkPools had been allocated from C-Heap and > there was a time window when no chunk pools were live yet. > > This is quirky and a bit ugly. It is also unnecessary since > [JDK-8272112](https://bugs.openjdk.org/browse/JDK-8272112) (since JDK 18). We > now create chunk pools as global objects, so they are live as soon as the > libjvm C++ initialization ran. We can remove this workaround and the comment. > > --- > > Tests: GHAs. > I also manually called this function, and allocated from the resulting > ResourceArea, at the very beginning of CreateJavaVM. I made sure that both > allocations and follow-up-chunk-allocation worked even this early in VM life. Today, the ChunkPools are allocated before main through static initialization. That means that the ChunkPools exists when main starts executing, so this is safe. - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19425#pullrequestreview-2084558186
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. Please wait 24 hours before attempting to integrate. In other words: Do not sponsor this until 24 hours has passed since opening the PR. - PR Comment: https://git.openjdk.org/jdk/pull/19254#issuecomment-2114911425
Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]
On Mon, 13 May 2024 04:55:24 GMT, Thomas Stuefe wrote: >> MEMFLAGS, as well as its enum constants, should live in its own include. >> >> The constants are used throughout the code base, often without needing the >> allocation APIs exposed through allocation.hpp. >> >> The MEMFLAGS enum def is often needed within NMT itself, again often without >> needing allocation.hpp. >> >> --- >> >> This patch moves the enum to its new file. >> >> It fixes those `allocation.hpp` includes that where only needed to get >> MEMFLAGS. It does not fix other includes. >> >> For backward compatibility, until we straightened out the dependencies >> (e.g., fixing all places where we rely on indirect includes), I added >> memflags.hpp to allocation.hpp. >> >> I tested (built) on: >> - MacOS aarch64, no precompiled headers, fastdebug >> - Linux x64, no precompiled headers, fastdebug, release, fastdebug >> crossbuild to aarch64, fastdebug minimal > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Update mallocLimit.hpp Seems reasonable and LGTM, thanks. - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19172#pullrequestreview-2051788348
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
On Wed, 20 Dec 2023 12:18:36 GMT, Jaikiran Pai wrote:
>> Jaikiran Pai 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:
>>
>> - remove redundant if block
>> - merge latest from master branch
>> - David's review comments - reduce if blocks and release the array outside
>> if block
>> - David's review comment - punctuation
>> - 8321971: Improve the user name detection logic in perfMemory
>> get_user_name_slow
>
> Just a note - I have incorporated the review comments, except from Johan
> which I'm still investigating and will update this PR soon.
Hi @jaikiran,
I looked at the `snprintf` issue, the easiest way of fixing this is this:
```c++
{
char* name = NEW_C_HEAP_ARRAY(char, nbytes, mtInternal);
int bytes_required = snprintf(name, nbytes, "%s/%d", dirname, pid);
if (bytes_required >= nbytes) {
// Our output was truncated
FREE_C_HEAP_ARRAY(name);
nbytes = bytes_required;
name = NEW_C_HEAP_ARRAY(char, nbytes, mtInternal);
bytes_required = snprintf(name, nbytes, "%s/%d", dirname, pid);
assert(bytes_required < nbytes, "must be");
}
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/17104#issuecomment-1953095597
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Tue, 19 Dec 2023 16:59:05 GMT, Emanuel Peter wrote: > [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the > `GrowableArrayCHeap`. This duplicates the current C-Heap allocation > capability in `GrowableArray`. I now remove that from `GrowableArray` and > move all usages to `GrowableArrayCHeap`. > > This has a few advantages: > - Clear separation between arena (and resource area) allocating array and > C-heap allocating array. > - We can prevent assigning / copying between arrays of different allocation > strategies already at compile time, and not only with asserts at runtime. > - We should not have multiple implementations of the same thing (C-Heap > backed array). > - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now > know that C-Heap backed arrays do not get copied unknowingly. > > **Bonus** > We can now restrict `GrowableArray` element type `E` to be > `std::is_trivially_destructible::value == true`. The idea is that arena / > resource allocated arrays get abandoned, often without being even cleared. > Hence, the elements in the array are never destructed. But if we only use > elements that are trivially destructible, then it makes no difference if the > destructors are ever called, or the elements simply abandoned. > > For `GrowableArrayCHeap`, we expect that the user eventually calls the > destructor for the array, which in turn calls the destructors of the > remaining elements. Hence, it is up to the user to ensure the cleanup. And so > we can allow non-trivial destructors. > > **Testing** > Tier1-3 + stress testing: pending Not yet bot! - PR Comment: https://git.openjdk.org/jdk/pull/17160#issuecomment-1929617168
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Thu, 21 Dec 2023 10:48:30 GMT, Johan Sjölen wrote: >> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the >> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation >> capability in `GrowableArray`. I now remove that from `GrowableArray` and >> move all usages to `GrowableArrayCHeap`. >> >> This has a few advantages: >> - Clear separation between arena (and resource area) allocating array and >> C-heap allocating array. >> - We can prevent assigning / copying between arrays of different allocation >> strategies already at compile time, and not only with asserts at runtime. >> - We should not have multiple implementations of the same thing (C-Heap >> backed array). >> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now >> know that C-Heap backed arrays do not get copied unknowingly. >> >> **Bonus** >> We can now restrict `GrowableArray` element type `E` to be >> `std::is_trivially_destructible::value == true`. The idea is that arena / >> resource allocated arrays get abandoned, often without being even cleared. >> Hence, the elements in the array are never destructed. But if we only use >> elements that are trivially destructible, then it makes no difference if the >> destructors are ever called, or the elements simply abandoned. >> >> For `GrowableArrayCHeap`, we expect that the user eventually calls the >> destructor for the array, which in turn calls the destructors of the >> remaining elements. Hence, it is up to the user to ensure the cleanup. And >> so we can allow non-trivial destructors. >> >> **Testing** >> Tier1-3 + stress testing: pending > > src/hotspot/share/utilities/growableArray.hpp line 621: > >> 619: // - Arena >> 620: // >> 621: // Itself, it can be embedded, on stack, resource_arena or arena >> allocated. > > "Itself can be allocated on stack, resource area or arena allocated." That it can be embedded into another class/struct is a given, imho. - PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433904533
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Wed, 20 Dec 2023 20:39:27 GMT, Kim Barrett wrote:
>> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the
>> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation
>> capability in `GrowableArray`. I now remove that from `GrowableArray` and
>> move all usages to `GrowableArrayCHeap`.
>>
>> This has a few advantages:
>> - Clear separation between arena (and resource area) allocating array and
>> C-heap allocating array.
>> - We can prevent assigning / copying between arrays of different allocation
>> strategies already at compile time, and not only with asserts at runtime.
>> - We should not have multiple implementations of the same thing (C-Heap
>> backed array).
>> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now
>> know that C-Heap backed arrays do not get copied unknowingly.
>>
>> **Bonus**
>> We can now restrict `GrowableArray` element type `E` to be
>> `std::is_trivially_destructible::value == true`. The idea is that arena /
>> resource allocated arrays get abandoned, often without being even cleared.
>> Hence, the elements in the array are never destructed. But if we only use
>> elements that are trivially destructible, then it makes no difference if the
>> destructors are ever called, or the elements simply abandoned.
>>
>> For `GrowableArrayCHeap`, we expect that the user eventually calls the
>> destructor for the array, which in turn calls the destructors of the
>> remaining elements. Hence, it is up to the user to ensure the cleanup. And
>> so we can allow non-trivial destructors.
>>
>> **Testing**
>> Tier1-3 + stress testing: pending
>
> src/hotspot/share/memory/heapInspection.cpp line 282:
>
>> 280: KlassInfoHisto::KlassInfoHisto(KlassInfoTable* cit) :
>> 281: _cit(cit) {
>> 282: _elements = new GrowableArrayCHeap> mtServiceability>(_histo_initial_size);
>
> pre-existing: Why is this initialization separate from the ctor-initializer?
> And this looks like an example of
> where it would be better as a direct GA member rather than a pointer to GA.
Can name be changed to `_histo_initial_capacity`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433897055
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Tue, 19 Dec 2023 16:59:05 GMT, Emanuel Peter wrote:
> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the
> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation
> capability in `GrowableArray`. I now remove that from `GrowableArray` and
> move all usages to `GrowableArrayCHeap`.
>
> This has a few advantages:
> - Clear separation between arena (and resource area) allocating array and
> C-heap allocating array.
> - We can prevent assigning / copying between arrays of different allocation
> strategies already at compile time, and not only with asserts at runtime.
> - We should not have multiple implementations of the same thing (C-Heap
> backed array).
> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now
> know that C-Heap backed arrays do not get copied unknowingly.
>
> **Bonus**
> We can now restrict `GrowableArray` element type `E` to be
> `std::is_trivially_destructible::value == true`. The idea is that arena /
> resource allocated arrays get abandoned, often without being even cleared.
> Hence, the elements in the array are never destructed. But if we only use
> elements that are trivially destructible, then it makes no difference if the
> destructors are ever called, or the elements simply abandoned.
>
> For `GrowableArrayCHeap`, we expect that the user eventually calls the
> destructor for the array, which in turn calls the destructors of the
> remaining elements. Hence, it is up to the user to ensure the cleanup. And so
> we can allow non-trivial destructors.
>
> **Testing**
> Tier1-3 + stress testing: pending
Wow! Thank you for this Emanuel.
I went through your changes and I am happy with them. There are some spelling
issues and my opinions on how to write the doc strings. I also asked for some
"length"/"size" naming to be changed to "capacity", you don't have to do this
as it's pre-existing, but it would make that code clearer.
src/hotspot/share/jfr/leakprofiler/chains/edgeStore.cpp line 287:
> 285: assert(edge != nullptr, "invariant");
> 286: if (_leak_context_edges == nullptr) {
> 287: _leak_context_edges = new GrowableArrayCHeap mtTracing>(initial_size);
Pre-existing: `initial_capacity` is a better name.
src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp line
55:
> 53: template
> 54: static GrowableArrayCHeap* c_heap_allocate_array(int size =
> initial_array_size) {
> 55: return new GrowableArrayCHeap(size);
`capacity` instead of `size`
src/hotspot/share/jfr/recorder/checkpoint/types/jfrThreadGroup.cpp line 266:
> 264:
> 265: JfrThreadGroup::JfrThreadGroup() :
> 266: _list(new GrowableArrayCHeap mtTracing>(initial_array_size)) {}
`capacity`
src/hotspot/share/jfr/recorder/jfrRecorder.cpp line 151:
> 149: assert(length >= 1, "invariant");
> 150: assert(dcmd_recordings_array == nullptr, "invariant");
> 151: dcmd_recordings_array = new
> GrowableArrayCHeap(length);
`capacity`
src/hotspot/share/jfr/support/jfrKlassUnloading.cpp line 38:
> 36:
> 37: template
> 38: static GrowableArrayCHeap* c_heap_allocate_array(int size =
> initial_array_size) {
`capacity`
src/hotspot/share/utilities/growableArray.hpp line 618:
> 616:
> 617: // The GrowableArray internal data is allocated from either:
> 618: // - Resrouce area (default)
Spelling
src/hotspot/share/utilities/growableArray.hpp line 621:
> 619: // - Arena
> 620: //
> 621: // Itself, it can be embedded, on stack, resource_arena or arena
> allocated.
"Itself can be allocated on stack, resource area or arena allocated."
src/hotspot/share/utilities/growableArray.hpp line 629:
> 627: // For C-Heap allocation use GrowableArrayCHeap.
> 628: //
> 629: // Note, that with GrowableArray does not deallocate the allocated
> memory from
"that the" not "that with"
src/hotspot/share/utilities/growableArray.hpp line 638:
> 636: // GrowableArray is copyable, but it only creates a shallow copy. Hence,
> one has
> 637: // to be careful not to duplicate the state and then diverge while
> sharing the
> 638: // underlying data.
Sad but true :-(
src/hotspot/share/utilities/growableArray.hpp line 644:
> 642: friend class GrowableArrayWithAllocator >;
> 643:
> 644: // Since GrowableArray is arena / resource area allocated, it is a
> custom to
"it is a custom to" can basically be removed
"Since Growable array is arena/resource area allocated it does not destruct its
elements. Therefore, ..." is sufficient.
-
PR Review: https://git.openjdk.org/jdk/pull/17160#pullrequestreview-1792708591
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433894629
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433894947
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433895133
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433895741
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433896219
PR Re
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow
On Thu, 14 Dec 2023 10:13:51 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which proposes to improve the code
> in `get_user_name_slow` function, which is used to identify the target JVM
> owner's user name? This addresses https://bugs.openjdk.org/browse/JDK-8321971.
>
> As noted in that JBS issue, in its current form, the nested loop ends up
> iterating over the directory contents of `hsperfdata_xxx` directory and then
> for each iteration it checks if the name of the entry matches the pid. This
> iteration shouldn't be needed and instead one could look for a file named
> `` within that directory.
>
> No new test has been added, given the nature of this change. Existing tier1,
> tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and
> macosx.
Hi,
A small review on the usage of the string functions. Have you considered using
`stringStream` and passing references to an instance of it instead?
src/hotspot/os/posix/perfMemory_posix.cpp line 447:
> 445:
> 446: return name;
> 447: }
This drops the `snprintf` return value which indicates if an error has
occurred, can this be remediated?
src/hotspot/os/posix/perfMemory_posix.cpp line 617:
> 615:
> 616: if (statbuf.st_ctime > oldest_ctime) {
> 617: char* user = strchr(dentry->d_name, '_') + 1;
Invalid pointer if `strchr` returns null.
-
PR Review: https://git.openjdk.org/jdk/pull/17104#pullrequestreview-1786373167
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1429831911
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1429840030
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v12]
On Thu, 23 Nov 2023 16:11:32 GMT, Afshin Zafari wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> Afshin Zafari has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/pr/15418' into pr_15418
> - Suggested cleanups and tests
Still LGTM.
-
PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1825422568
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v12]
On Fri, 10 Nov 2023 07:38:24 GMT, Thomas Stuefe wrote: >> Analysts and supporters often use /proc/xx/maps to make sense of the memory >> footprint of a process. >> >> Interpreting the memory map correctly can help when used as a complement to >> other tools (e.g. NMT). There even exist tools out there that attempt to >> annotate the process memory map with JVM information. >> >> That, however, can be more accurately done from within the JVM, at least for >> mappings originating from hotspot. After all, we can correlate the mapping >> information in NMT with the VMA map. >> >> Example output (from a spring petstore run): >> >> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt) >> >> This patch adds the VM annotations for VMAs that are *mmaped*. I also have >> an experimental patch that works with malloc'ed memory, but it is not ready >> yet for consumption and I leave that part of for now. > > Thomas Stuefe has updated the pull request incrementally with three > additional commits since the last revision: > > - add test to test dumping to specific location > - change filename arg from -f to -F to prevent conflicts with jcmd -f > - print errno if dump file cannot be opened Hi Thomas, Thanks for your hard work on this! LGTM. - Marked as reviewed by jsjolen (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1724406291
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v9]
On Wed, 8 Nov 2023 11:24:43 GMT, Thomas Stuefe wrote: > > As this adds a JCmd, doesn't this need both a CSR and a manual entry? > > * CSR: not sure; there are precedences for going with CSR and without > CSR. Since we get awfully close to JDK22 freeze, I would prefer for a CSR not > to be needed. Also would make backporting a lot easier. > > * Manpage: not sure either; IIRC last time I tried, I was told that > Oracle maintains these internally, because there is internal documentation > that needs updating too? @dholmes-ora, would you mind helping us out here with regards to the process? Would a new JCmd require a CSR or would it be acceptable to merge without one? Thank you. - PR Comment: https://git.openjdk.org/jdk/pull/16301#issuecomment-1801718561
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v9]
On Thu, 2 Nov 2023 17:42:29 GMT, Thomas Stuefe wrote: >> Analysts and supporters often use /proc/xx/maps to make sense of the memory >> footprint of a process. >> >> Interpreting the memory map correctly can help when used as a complement to >> other tools (e.g. NMT). There even exist tools out there that attempt to >> annotate the process memory map with JVM information. >> >> That, however, can be more accurately done from within the JVM, at least for >> mappings originating from hotspot. After all, we can correlate the mapping >> information in NMT with the VMA map. >> >> Example output (from a spring petstore run): >> >> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt) >> >> This patch adds the VM annotations for VMAs that are *mmaped*. I also have >> an experimental patch that works with malloc'ed memory, but it is not ready >> yet for consumption and I leave that part of for now. > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Fix another windows error As this adds a JCmd, doesn't this need both a CSR and a manual entry? - PR Comment: https://git.openjdk.org/jdk/pull/16301#issuecomment-1801457657
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Wed, 1 Nov 2023 10:04:30 GMT, Johan Sjölen wrote: >> Thomas Stuefe has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix various builds > > OK, went through the cache. Will continue later. > @jdksjolen anything more needed from my side? I'll work through this tomorrow, just to make sure I didn't miss anything. - PR Comment: https://git.openjdk.org/jdk/pull/16301#issuecomment-1799002931
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Wed, 1 Nov 2023 10:13:03 GMT, Thomas Stuefe wrote: >> src/hotspot/share/nmt/memMapPrinter.cpp line 105: >> >>> 103: _ranges[_count - 1].to = to; >>> 104: return true; >>> 105: } >> >> I'm pretty sure that the virtual memory tracker already gives you the >> minimal set of regions, have you observed this branch being taken? > > Yes, it resulted in a significant performance gain for a test run where I > interleaved non-committed and committed memory of the same tag. I may have > been mistaken, of course. It being committed or reserved shouldn't matter (I assume non-committed = reserved), as committed just means that there's a pointer to the committed memory region in the reserved memory region. Well, all I can say is that I'm surprised. - PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378643276
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Wed, 1 Nov 2023 09:52:24 GMT, Thomas Stuefe wrote:
>> src/hotspot/share/nmt/memMapPrinter.cpp line 99:
>>
>>> 97: }
>>> 98:
>>> 99: bool add(const void* from, const void* to, MEMFLAGS f) {
>>
>> Please mention that we're `add`ing in sorted order, that is that `forall R
>> \in _ranges: R.to <= from` holds.
>
> I wasn't sure about that. Do we always? Are NMT regions guaranteed to be
> sorted? (Dimly remember cases where that weren't so)
It's always sorted in some way, we save the linked lists in some different
sorting orders for baselining, but in the `VirtualMemoryTracker` they're
guaranteed to be sorted by address.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378609954
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Sat, 28 Oct 2023 13:04:05 GMT, Thomas Stuefe wrote:
>> Analysts and supporters often use /proc/xx/maps to make sense of the memory
>> footprint of a process.
>>
>> Interpreting the memory map correctly can help when used as a complement to
>> other tools (e.g. NMT). There even exist tools out there that attempt to
>> annotate the process memory map with JVM information.
>>
>> That, however, can be more accurately done from within the JVM, at least for
>> mappings originating from hotspot. After all, we can correlate the mapping
>> information in NMT with the VMA map.
>>
>> Example output (from a spring petstore run):
>>
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>>
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have
>> an experimental patch that works with malloc'ed memory, but it is not ready
>> yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix various builds
OK, went through the cache. Will continue later.
src/hotspot/share/nmt/memMapPrinter.cpp line 105:
> 103: _ranges[_count - 1].to = to;
> 104: return true;
> 105: }
I'm pretty sure that the virtual memory tracker already gives you the minimal
set of regions, have you observed this branch being taken?
src/hotspot/share/nmt/memMapPrinter.cpp line 119:
> 117: assert(_capacity > _count, "Sanity");
> 118: _ranges[_count].from = from;
> 119: _ranges[_count].to = to;
Or just `_ranges[_count] = Range{from, to}`
-
PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1707901319
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378602552
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378604692
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Sat, 28 Oct 2023 13:04:05 GMT, Thomas Stuefe wrote:
>> Analysts and supporters often use /proc/xx/maps to make sense of the memory
>> footprint of a process.
>>
>> Interpreting the memory map correctly can help when used as a complement to
>> other tools (e.g. NMT). There even exist tools out there that attempt to
>> annotate the process memory map with JVM information.
>>
>> That, however, can be more accurately done from within the JVM, at least for
>> mappings originating from hotspot. After all, we can correlate the mapping
>> information in NMT with the VMA map.
>>
>> Example output (from a spring petstore run):
>>
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>>
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have
>> an experimental patch that works with malloc'ed memory, but it is not ready
>> yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix various builds
src/hotspot/share/nmt/memMapPrinter.cpp line 99:
> 97: }
> 98:
> 99: bool add(const void* from, const void* to, MEMFLAGS f) {
Please mention that we're `add`ing in sorted order, that is that `forall R \in
_ranges: R.to <= from` holds.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378589809
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Wed, 1 Nov 2023 07:32:23 GMT, Thomas Stuefe wrote: >> src/hotspot/share/nmt/memMapPrinter.cpp line 79: >> >>> 77: const void* const min = MAX2(from1, from2); >>> 78: const void* const max = MIN2(to1, to2); >>> 79: return min < max; >> >> I had to rewrite it as: >> >> `return MAX2(from1, from2) < MIN2(to1, to2);` >> >> to make sure I understand it, or better yet, why not have it as a macros? >> >> `#define RANGE_INTERSECT(min, max) (return MAX2(from1, from2) < MIN2(to1, >> to2))` >> >> MAX2 and MIN2 are macros already and we're not doing anything fancy here. > > I'll do the former, that is clearer I agree, but leave the latter out (I > assume with the macro you mean add it to globalDefenitions.hpp). > > I fear that a lot of bikeshedding and general discussions would start, and to > do it properly needs a bit more time. Because we really would benefit from > having a nice templatized utility classes for ranges. Like MemRegion, but > that one is tied to HeapWord* I think. `MAX2` and `MIN2` are not macros, they're `constexpr` free functions, and that's what a `RANGE_INTERSECT` free function should be also. Unexpected because of their names, I know. - PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378579302
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Wed, 1 Nov 2023 09:37:22 GMT, Johan Sjölen wrote: >> I'll do the former, that is clearer I agree, but leave the latter out (I >> assume with the macro you mean add it to globalDefenitions.hpp). >> >> I fear that a lot of bikeshedding and general discussions would start, and >> to do it properly needs a bit more time. Because we really would benefit >> from having a nice templatized utility classes for ranges. Like MemRegion, >> but that one is tied to HeapWord* I think. > > `MAX2` and `MIN2` are not macros, they're `constexpr` free functions, and > that's what a `RANGE_INTERSECT` free function should be also. Unexpected > because of their names, I know. FWIW: Yeah, if it's going to be in globalDefinitions.hpp then do that in a separate RFE. - PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378579763
Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v4]
On Mon, 30 Oct 2023 10:32:17 GMT, Thomas Stuefe wrote: >This code would gain IMHO by being dumbed down Agreed, seems like it's had a lot of "just one more feature" commits over time. - PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1376144551
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Sat, 28 Oct 2023 13:04:05 GMT, Thomas Stuefe wrote:
>> Analysts and supporters often use /proc/xx/maps to make sense of the memory
>> footprint of a process.
>>
>> Interpreting the memory map correctly can help when used as a complement to
>> other tools (e.g. NMT). There even exist tools out there that attempt to
>> annotate the process memory map with JVM information.
>>
>> That, however, can be more accurately done from within the JVM, at least for
>> mappings originating from hotspot. After all, we can correlate the mapping
>> information in NMT with the VMA map.
>>
>> Example output (from a spring petstore run):
>>
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>>
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have
>> an experimental patch that works with malloc'ed memory, but it is not ready
>> yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix various builds
Hi Thomas,
Did another run through. I haven't looked at `CachedNMTInformation` in depth
yet.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 79:
> 77: void MemMapPrinter::pd_iterate_all_mappings(MappingPrintClosure& closure)
> {
> 78: FILE* f = os::fopen("/proc/self/maps", "r");
> 79: if (f != nullptr) {
Suggestion: `if (f == nullptr) return;`, reduces indentation for main body of
code.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 80:
> 78: FILE* f = os::fopen("/proc/self/maps", "r");
> 79: if (f != nullptr) {
> 80: static constexpr size_t linesize = sizeof(ProcMapsInfo);
Make this `constexpr const`, remove `static`.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 82:
> 80: static constexpr size_t linesize = sizeof(ProcMapsInfo);
> 81: char line[linesize];
> 82: while(fgets(line, sizeof(line), f) == line) {
Style: space between `while` and `(`.
test/hotspot/jtreg/serviceability/dcmd/vm/SystemMapTest.java line 3:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
Copyright Redhat
-
PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1703761626
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375986785
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375988375
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375987037
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375994689
Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v4]
On Mon, 30 Oct 2023 09:55:35 GMT, Johan Sjölen wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix Windows build
>
> src/hotspot/share/compiler/compilerOracle.cpp line 654:
>
>> 652: IF_ENUM_STRING("print", {
>> 653: value = (uintx)MemStatAction::print;
>> 654: print_final_memstat_report = true;
>
> Pre-existing/nit: Setting a member variable is not part of parsing the
> `uintx` value, should be done by caller.
>
> Performing this refactoring would also enable us to to have a loop like this
> (`Pair` is available in `utilities/pair.hpp`):
>
> ```c++
> constexpr const int len = 2;
> Pair actions[len] = {
> Pair("collect", MemStatAction::collect),
> Pair("print",MemStatAction::print)
> };
> for (int i = 0; i < len; i++) {
> auto p = actions[i];
> if (strncmp(line, p.first, strlen(p.first)) == 0) {
> value = p.second;
> return true;
> }
> }
Ouch, I just realized that we can't differentiate between being provided with
the literal number 2 and `MemStatAction::print` anymore since you moved the
literal number parsing into this function. That means that we can't set
`print_final_memstat_report` after returning from this function.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1375979272
Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v4]
On Thu, 26 Oct 2023 16:17:14 GMT, Thomas Stuefe wrote:
>> When using 'MemStat' CompileCommand, we accidentally register the command if
>> an invalid suboption had been specified. Fixed, added regression test
>> (verified).
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix Windows build
Hi,
This looks good to me, I've got a couple of comments (one is just a nit). I'll
leave it to the compiler folks for approval.
src/hotspot/share/compiler/compilerOracle.cpp line 654:
> 652: IF_ENUM_STRING("print", {
> 653: value = (uintx)MemStatAction::print;
> 654: print_final_memstat_report = true;
Pre-existing/nit: Setting a member variable is not part of parsing the `uintx`
value, should be done by caller.
Performing this refactoring would also enable us to to have a loop like this
(`Pair` is available in `utilities/pair.hpp`):
```c++
constexpr const int len = 2;
Pair actions[len] = {
Pair("collect", MemStatAction::collect),
Pair("print",MemStatAction::print)
};
for (int i = 0; i < len; i++) {
auto p = actions[i];
if (strncmp(line, p.first, strlen(p.first)) == 0) {
value = p.second;
return true;
}
}
test/hotspot/jtreg/compiler/compilercontrol/commands/MemStatTest.java line 3:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> 3: * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
Thanks! But I think we're OK with Redhat taking credit for this one :).
-
PR Review: https://git.openjdk.org/jdk/pull/16335#pullrequestreview-1703695585
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1375945596
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1375968556
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v3]
On Thu, 26 Oct 2023 15:02:53 GMT, Thomas Stuefe wrote:
>> Analysts and supporters often use /proc/xx/maps to make sense of the memory
>> footprint of a process.
>>
>> Interpreting the memory map correctly can help when used as a complement to
>> other tools (e.g. NMT). There even exist tools out there that attempt to
>> annotate the process memory map with JVM information.
>>
>> That, however, can be more accurately done from within the JVM, at least for
>> mappings originating from hotspot. After all, we can correlate the mapping
>> information in NMT with the VMA map.
>>
>> Example output (from a spring petstore run):
>>
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>>
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have
>> an experimental patch that works with malloc'ed memory, but it is not ready
>> yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains four commits:
>
> - Merge master and solve merge conflicts
> - small fixes
> - start from VM op; show more thread details
> - start
Hi,
Thank you for this PR. These comments are just a first pass, I haven't finished
going through the code.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 32:
> 30: #include "utilities/globalDefinitions.hpp"
> 31:
> 32: struct proc_maps_info_t {
`struct ProcMapsInfo` please. The `_t` is apparently reserved for POSIX, and I
prefer our structs and classes to not look like they're coming from a C library
anyway.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 34:
> 32: struct proc_maps_info_t {
> 33: unsigned long long from = 0;
> 34: unsigned long long to = 0;
Can we not use `uintptr_t` here? scanf directive:
https://stackoverflow.com/questions/12228227/how-to-get-pointer-sized-integer-using-scanf
src/hotspot/os/linux/memMapPrinter_linux.cpp line 39:
> 37: char dev[20 + 1];
> 38: char inode[20 + 1];
> 39: char filename[1024 + 1];
Maybe use `PATH_MAX` here? Potentially +1 to account for null byte.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 47:
> 45: if (items_read < 2) {
> 46: return false;
> 47: }
Not necessary thanks to the other return value!
src/hotspot/os/linux/memMapPrinter_linux.cpp line 72:
> 70: "from to "
> 71: #else
> 72: "from to "
16+1 and 8+1 spaces, depending on how long the addresses are I assume? Your
choice, but might be good to note that.
src/hotspot/os/linux/memMapPrinter_linux.cpp line 81:
> 79: FILE* f = os::fopen("/proc/self/maps", "r");
> 80: if (f != nullptr) {
> 81: char line[1024];
This looks like a bug: A line may at most be 1024 characters, but
`scan_proc_maps_line` may clearly read more than 1024 characters. Yes, it won't
read out of bounds, but it limits the parser unnecessarily.
src/hotspot/share/services/memMapPrinter.hpp line 43:
> 41: const void* to() const { return _to; }
> 42: virtual void print_details_1(outputStream* st) const {} // To be
> printed before VM annotations
> 43: virtual void print_details_2(outputStream* st) const {} // To be
> printed before VM annotations
May we please have better names :)?
test/hotspot/jtreg/serviceability/dcmd/vm/SystemMapTest.java line 48:
> 46: if (output.getOutput().contains("please enable Native Memory
> Tracking")) {
> 47: have_nmt = false;
> 48: }
`boolean have_nmt = output.getOutput().contains`
-
PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1701276028
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374325262
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374327488
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374255012
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374253123
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374318000
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374322993
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374313507
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374311277
Integrated: 8318447: Move NMT source code to own subdirectory
On Thu, 19 Oct 2023 20:06:50 GMT, Johan Sjölen wrote: > I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards This pull request has now been integrated. Changeset: 9864951d Author:Johan Sjölen URL: https://git.openjdk.org/jdk/commit/9864951dceb0ddc4479ced04b6d5a2363f1e307d Stats: 506 lines in 102 files changed: 212 ins; 219 del; 75 mod 8318447: Move NMT source code to own subdirectory Reviewed-by: stefank, dholmes, stuefe - PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory [v8]
On Thu, 26 Oct 2023 09:51:15 GMT, Johan Sjölen wrote: >> I think that NMT is deserving of its own subdirectory. Can we do a review of >> the changes before I fix the merge conflicts? >> >> 1. Moved all the nmt source code from services/ to nmt/ >> 2. Renamed all the include statements and sorted them >> 3. Fixed the include guards > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Stefan's suggestions Thank you for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/16276#issuecomment-1781194359
Re: RFR: 8318447: Move NMT source code to own subdirectory [v7]
On Tue, 24 Oct 2023 11:51:45 GMT, Johan Sjölen wrote: >> I think that NMT is deserving of its own subdirectory. Can we do a review of >> the changes before I fix the merge conflicts? >> >> 1. Moved all the nmt source code from services/ to nmt/ >> 2. Renamed all the include statements and sorted them >> 3. Fixed the include guards > > Johan Sjölen has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge remote-tracking branch 'upstream/master' into move-nmt > - Fix stefank suggestions > - Merge remote-tracking branch 'origin/master' into move-nmt > - Fix messed up include > - Missed this include > - Merge remote-tracking branch 'origin/master' into move-nmt > - Fixed reviewed changes > - Move NMT to its own subdirectory Thanks Stefan, good catch on the ordering requirements. I'll integrate this ASAP. - PR Comment: https://git.openjdk.org/jdk/pull/16276#issuecomment-1780775333
Re: RFR: 8318447: Move NMT source code to own subdirectory [v8]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Stefan's suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/16276/files - new: https://git.openjdk.org/jdk/pull/16276/files/70b39e41..bb72984b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=06-07 Stats: 6 lines in 4 files changed: 2 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory
On Mon, 23 Oct 2023 08:41:56 GMT, Stefan Karlsson wrote: >> Hi, >> >> Thank you for looking through these changes. I applied your comments and >> also did a run through to look for incorrectly ordered includes. For the >> gtest source files I separated the includes in a consistent manner, they all >> look like this pattern now: >> >> ```c++ >> #include "precompiled.hpp" >> #include "memory/allocation.hpp" >> #include "nmt/mallocHeader.inline.hpp" >> #include "nmt/memTracker.hpp" >> #include "runtime/os.hpp" >> >> #include "testutils.hpp" >> #include "unittest.hpp" > >> For the gtest source files I separated the includes in a consistent manner, >> they all look like this pattern now: > > That's not what I see in the latest patch. Could you revert that separation > and then we can consider that style change in a separate RFE? @stefank, I believe that I've applied your suggestions, would you mind having another look? Thanks - PR Comment: https://git.openjdk.org/jdk/pull/16276#issuecomment-1779260132
Re: RFR: 8318447: Move NMT source code to own subdirectory
On Mon, 23 Oct 2023 08:41:56 GMT, Stefan Karlsson wrote: > > For the gtest source files I separated the includes in a consistent manner, > > they all look like this pattern now: > > That's not what I see in the latest patch. Could you revert that separation > and then we can consider that style change in a separate RFE? Sure, this could be done for all tests (not only NMT). - PR Comment: https://git.openjdk.org/jdk/pull/16276#issuecomment-1777053794
Re: RFR: 8318447: Move NMT source code to own subdirectory [v7]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge remote-tracking branch 'upstream/master' into move-nmt - Fix stefank suggestions - Merge remote-tracking branch 'origin/master' into move-nmt - Fix messed up include - Missed this include - Merge remote-tracking branch 'origin/master' into move-nmt - Fixed reviewed changes - Move NMT to its own subdirectory - Changes: https://git.openjdk.org/jdk/pull/16276/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=06 Stats: 508 lines in 102 files changed: 214 ins; 219 del; 75 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory [v5]
On Mon, 23 Oct 2023 08:23:44 GMT, Stefan Karlsson wrote: >> Johan Sjölen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix messed up include > > src/hotspot/share/nmt/nmtPreInit.hpp line 35: > >> 33: #include "utilities/macros.hpp" >> 34: >> 35: #ifdef ASSERT > > The blank line at 34 is not following the style for our conditional includes. > Remove it, or better yet skip conventionalize the include of > runtime/atomic.hpp since it just adds to noise to the file. Removed the blank line, I think the noise is meaningful in this case. - PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1369924184
Re: RFR: 8318447: Move NMT source code to own subdirectory [v6]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Merge remote-tracking branch 'origin/master' into move-nmt - Fix messed up include - Missed this include - Merge remote-tracking branch 'origin/master' into move-nmt - Fixed reviewed changes - Move NMT to its own subdirectory - Changes: https://git.openjdk.org/jdk/pull/16276/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=05 Stats: 507 lines in 102 files changed: 214 ins; 212 del; 81 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory [v5]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Fix messed up include - Changes: - all: https://git.openjdk.org/jdk/pull/16276/files - new: https://git.openjdk.org/jdk/pull/16276/files/1647b41e..4dfb027e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=03-04 Stats: 4 lines in 1 file changed: 2 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory [v4]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Missed this include - Changes: - all: https://git.openjdk.org/jdk/pull/16276/files - new: https://git.openjdk.org/jdk/pull/16276/files/c2b14b34..1647b41e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory [v3]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge remote-tracking branch 'origin/master' into move-nmt - Fixed reviewed changes - Move NMT to its own subdirectory - Changes: https://git.openjdk.org/jdk/pull/16276/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=02 Stats: 502 lines in 100 files changed: 211 ins; 210 del; 81 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory [v2]
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Fixed reviewed changes - Changes: - all: https://git.openjdk.org/jdk/pull/16276/files - new: https://git.openjdk.org/jdk/pull/16276/files/2fd7c355..08e6f4bf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=00-01 Stats: 43 lines in 15 files changed: 19 ins; 15 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8318447: Move NMT source code to own subdirectory
On Thu, 19 Oct 2023 20:06:50 GMT, Johan Sjölen wrote: > I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Hi, Thank you for looking through these changes. I applied your comments and also did a run through to look for incorrectly ordered includes. For the gtest source files I separated the includes in a consistent manner, they all look like this pattern now: ```c++ #include "precompiled.hpp" #include "memory/allocation.hpp" #include "nmt/mallocHeader.inline.hpp" #include "nmt/memTracker.hpp" #include "runtime/os.hpp" #include "testutils.hpp" #include "unittest.hpp" - PR Comment: https://git.openjdk.org/jdk/pull/16276#issuecomment-1772570292
RFR: 8318447: Move NMT source code to own subdirectory
I think that NMT is deserving of its own subdirectory. Can we do a review of the changes before I fix the merge conflicts? 1. Moved all the nmt source code from services/ to nmt/ 2. Renamed all the include statements and sorted them 3. Fixed the include guards - Commit messages: - Move NMT to its own subdirectory Changes: https://git.openjdk.org/jdk/pull/16276/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16276&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8318447 Stats: 485 lines in 100 files changed: 204 ins; 206 del; 75 mod Patch: https://git.openjdk.org/jdk/pull/16276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16276/head:pull/16276 PR: https://git.openjdk.org/jdk/pull/16276
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v3]
On Mon, 28 Aug 2023 11:03:51 GMT, Afshin Zafari wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> find_from_end and its caller are also updated.
I still approve of this patch as it's better than what we had before. There are
a lot of suggested improvements that can be done either in this PR or in a
future RFE. `git blame` shows that this hasn't been touched since 2008, so I
don't think applying all suggestions now is in any sense critical :-).
-
Marked as reviewed by jsjolen (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15418#pullrequestreview-1599922400
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method
On Fri, 25 Aug 2023 08:55:29 GMT, Stefan Karlsson wrote:
>> It's arbitrary and chosen by the caller through `f`, so I can't say :-). The
>> best use case we have now is when you only have an `int` which uniquely
>> describes an `LGRPSpace`.
>
> This code looks similar to a capturing lambda. Would an alternative be to use
> that instead and let the calling code be changed to:
>
>
> int i = lgrp_spaces()->find([](LGRPSpace* space) {
> return space->lgrp_id() == lgrp_id;
> });
>
>
> Alternatively:
>
> auto matches_lgrp_id = [](LGRPSpace* space) {
> return space->lgrp_id() == lgrp_id;
> };
>
> int i = lgrp_spaces()->find(matches_lgrp_id);
We could just as well do a capturing lambda here, yes. Then we'd have:
```c++
template
int find(F finder);
It'd be a template instead of function pointer since it's a capturing lambda
and `std::function` is not permitted in Hotspot AFAIK.
As an aside, to clarify for readers: There's a `&` missing in the capture list
of your examples.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1305442361
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method
On Fri, 25 Aug 2023 02:41:19 GMT, David Holmes wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> src/hotspot/share/utilities/growableArray.hpp line 213:
>
>> 211:
>> 212: template
>> 213: int find(T* token, bool f(T*, E)) const {
>
> Pardon my ignorance here, but what is the type relationship between T and E?
It's arbitrary and chosen by the caller through `f`, so I can't say :-). The
best use case we have now is when you only have an `int` which uniquely
describes an `LGRPSpace`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1305363537
Re: RFR: 8314502: GrowableArray: Make find with comparator take template
On Thu, 24 Aug 2023 14:09:46 GMT, Afshin Zafari wrote:
> The `find` method now is
> ```C++
> template
> int find(T* token, bool f(T*, E)) const {
> ...
>
> Any other functions which use this are also changed.
> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
> Windows passed.
Looks good to me, thank you. A couple of style issues that needs to be fixed,
some pre-existing.
src/hotspot/share/prims/jvmtiImpl.cpp line 126:
> 124: assert(e2 != nullptr, "e2 != nullptr");
> 125:
> 126: return v->equals(e2);
Please rename the `v` parameter to `e1`
src/hotspot/share/prims/jvmtiImpl.hpp line 91:
> 89: void (*_listener_fun)(void *, address*);
> 90:
> 91: static bool equals(GrowableElement *, GrowableElement *);
Remove spacing between `GrowableElement` and `*`
-
Marked as reviewed by jsjolen (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15418#pullrequestreview-1594030652
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1304600705
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1304602700
Re: RFR: 8314502: GrowableArray: Make find with comparator take template
On Thu, 24 Aug 2023 16:37:44 GMT, Johan Sjölen wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> src/hotspot/share/prims/jvmtiImpl.cpp line 126:
>
>> 124: assert(e2 != nullptr, "e2 != nullptr");
>> 125:
>> 126: return v->equals(e2);
>
> Please rename the `v` parameter to `e1`
And since you're here: Could you change the code style so that it's
`GrowableElement* e2` and not `GrowableElement *e2`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1304601987
Re: RFR: JDK-8293114: JVM should trim the native heap
On Thu, 6 Jul 2023 06:54:22 GMT, Thomas Stuefe wrote: > This is a continuation of https://github.com/openjdk/jdk/pull/10085. I closed > https://github.com/openjdk/jdk/pull/10085 because it had accumulated too much > comment history and got confusing. For a history of this issue, see previous > discussions [1] and the comment section of 10085. > > --- > > This RFE adds the option to trim the Glibc heap periodically. This can > recover a significant memory footprint if the VM process suffers from > high-but-rare malloc spikes. It does not matter who causes the spikes: the > JDK or customer code running in the JVM process. > > ### Background: > > The Glibc is reluctant to return memory to the OS. Temporary malloc spikes > often carry over as permanent RSS increase. Note that C-heap retention is > difficult to observe. Since it is freed memory, it won't appear in NMT; it is > just a part of RSS. > > This is, effectively, caching - a performance tradeoff by the glibc. It makes > a lot of sense with applications that cause high traffic on the C-heap. The > JVM, however, clusters allocations and often rolls its own memory management > based on virtual memory for many of its use cases. > > To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18 [2], > we added a new jcmd command to *manually* trim the C-heap on Linux (`jcmd > System.trim_native_heap`). We then observed customers running this command > periodically to slim down process sizes of container-bound jvms. That is > cumbersome, and the JVM can do this a lot better - among other things because > it knows best when *not* to trim. > > GLIBC internals > > The following information I took from the glibc source code and experimenting. > > # Why do we need to trim manually? Does the Glibc not trim on free? > > Upon `free()`, glibc may return memory to the OS if: > - the returned block was mmap'ed > - the returned block was not added to tcache or to fastbins > - the returned block, possibly merged with its two immediate neighbors, had > they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in > that case: > a) for the main arena, glibc attempts to lower the brk() > b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the heap. > In both cases, (a) and (b), only the top portion of the heap is reclaimed. > "Holes" in the middle of other in-use chunks are not reclaimed. > > So: glibc *may* automatically reclaim memory. In normal configurations, with > typical C-heap allocation granularity, it is unlikely. > > To increase the chance of auto-reclamation happening, one can do one or more > t... src/hotspot/share/memory/arena.cpp line 105: > 103: } > 104: return true; > 105: } Something seems wrong here? Having only empty pools means that `::prune()` is a no-op. - PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1254275721
Re: RFR: JDK-8293114: JVM should trim the native heap
On Thu, 6 Jul 2023 06:54:22 GMT, Thomas Stuefe wrote: > This is a continuation of https://github.com/openjdk/jdk/pull/10085. I closed > https://github.com/openjdk/jdk/pull/10085 because it had accumulated too much > comment history and got confusing. For a history of this issue, see previous > discussions [1] and the comment section of 10085. > > --- > > This RFE adds the option to trim the Glibc heap periodically. This can > recover a significant memory footprint if the VM process suffers from > high-but-rare malloc spikes. It does not matter who causes the spikes: the > JDK or customer code running in the JVM process. > > ### Background: > > The Glibc is reluctant to return memory to the OS. Temporary malloc spikes > often carry over as permanent RSS increase. Note that C-heap retention is > difficult to observe. Since it is freed memory, it won't appear in NMT; it is > just a part of RSS. > > This is, effectively, caching - a performance tradeoff by the glibc. It makes > a lot of sense with applications that cause high traffic on the C-heap. The > JVM, however, clusters allocations and often rolls its own memory management > based on virtual memory for many of its use cases. > > To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18 [2], > we added a new jcmd command to *manually* trim the C-heap on Linux (`jcmd > System.trim_native_heap`). We then observed customers running this command > periodically to slim down process sizes of container-bound jvms. That is > cumbersome, and the JVM can do this a lot better - among other things because > it knows best when *not* to trim. > > GLIBC internals > > The following information I took from the glibc source code and experimenting. > > # Why do we need to trim manually? Does the Glibc not trim on free? > > Upon `free()`, glibc may return memory to the OS if: > - the returned block was mmap'ed > - the returned block was not added to tcache or to fastbins > - the returned block, possibly merged with its two immediate neighbors, had > they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in > that case: > a) for the main arena, glibc attempts to lower the brk() > b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the heap. > In both cases, (a) and (b), only the top portion of the heap is reclaimed. > "Holes" in the middle of other in-use chunks are not reclaimed. > > So: glibc *may* automatically reclaim memory. In normal configurations, with > typical C-heap allocation granularity, it is unlikely. > > To increase the chance of auto-reclamation happening, one can do one or more > t... >And app's malloc load can fluctuate wildly, with temporary spikes and long >idle periods. Are you talking about allocations into native memory that a Java application does on its own accord and not as a consequence of the JVM doing its own allocs? For compiling, for example. - PR Comment: https://git.openjdk.org/jdk/pull/14781#issuecomment-1623304064
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Thu, 1 Jun 2023 05:23:25 GMT, Tobias Hartmann wrote: > What's the plan now to prevent re-introducing `NULL`? Hi Tobias. The only plan in place is social, the reviewers have to look out for it. I am however researching how to do this through machine. I'm currently researching ways of preventing any re-introductions by machine. These include poisoning the NULL macro by re-defining it and finding a tool which is capable of parsing C++ code which is yet to go through the pre-processor. - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571722147
Integrated: 8309044: Replace NULL with nullptr, final sweep of hotspot code
On Mon, 29 May 2023 10:09:15 GMT, Johan Sjölen wrote: > A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes > I'd appreciate if this was considered trivial. This pull request has now been integrated. Changeset: 4f161616 Author:Johan Sjölen URL: https://git.openjdk.org/jdk/commit/4f16161607edbf69f423ced1d3c24f7af058d46b Stats: 114 lines in 67 files changed: 0 ins; 0 del; 114 mod 8309044: Replace NULL with nullptr, final sweep of hotspot code Reviewed-by: stefank, dholmes, kvn, amitkumar - PR: https://git.openjdk.org/jdk/pull/14198
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Right, seems like the Windows CI fails at fetching JTReg. I'm merging this, thank you all for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1569815714
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code
On Mon, 29 May 2023 10:09:15 GMT, Johan Sjölen wrote: > A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes > I'd appreciate if this was considered trivial. Hi, thank you all for the suggestions! I've now applied them. I'll look at integrating tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1568940197
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes > I'd appreciate if this was considered trivial. Johan Sjölen has updated the pull request incrementally with two additional commits since the last revision: - Align - Suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/14198/files - new: https://git.openjdk.org/jdk/pull/14198/files/c7e28571..c9d9c30c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14198&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14198&range=00-01 Stats: 9 lines in 8 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/14198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14198/head:pull/14198 PR: https://git.openjdk.org/jdk/pull/14198
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code
On Mon, 29 May 2023 10:19:02 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > src/hotspot/share/utilities/globalDefinitions.cpp line 162: > >> 160: static_assert((size_t)HeapWordSize >= sizeof(juint), >> 161: "HeapWord should be at least as large as juint"); >> 162: static_assert(sizeof(nullptr) == sizeof(char*), "null must be same >> size as pointer"); > > T Add both? - PR Review Comment: https://git.openjdk.org/jdk/pull/14198#discussion_r1209167397
RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code
A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes I'd appreciate if this was considered trivial. - Commit messages: - Fix remaining work Changes: https://git.openjdk.org/jdk/pull/14198/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14198&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309044 Stats: 110 lines in 63 files changed: 0 ins; 0 del; 110 mod Patch: https://git.openjdk.org/jdk/pull/14198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14198/head:pull/14198 PR: https://git.openjdk.org/jdk/pull/14198
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code
On Mon, 29 May 2023 10:09:15 GMT, Johan Sjölen wrote:
> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes
> I'd appreciate if this was considered trivial.
All of the stuff to actually keep.
src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp line 125:
> 123:
> 124: void CodeInstaller::pd_relocate_JavaMethod(CodeBuffer &cbuf,
> methodHandle& method, jint pc_offset, JVMCI_TRAPS) {
> 125: NativeCall* call = nullptr;
T
src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp line 158:
> 156: // Check for proper post_call_nop
> 157: NativePostCallNop* nop =
> nativePostCallNop_at(call->next_instruction_address());
> 158: if (nop == nullptr) {
T
src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 2297:
> 2295: __ ld_ptr(method, array_base_offset +
> in_bytes(ResolvedIndyEntry::method_offset()), cache);
> 2296:
> 2297: // The invokedynamic is unresolved iff method is null
T
src/hotspot/cpu/riscv/gc/shared/barrierSetAssembler_riscv.cpp line 374:
> 372: // Make sure klass is 'reasonable', which is not zero.
> 373: __ load_klass(obj, obj, tmp1); // get klass
> 374: __ beqz(obj, error); // if klass is null it is broken
T
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 4019:
> 4017: StubRoutines::_forward_exception_entry =
> generate_forward_exception();
> 4018:
> 4019: if (UnsafeCopyMemory::_table == nullptr) {
T
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 4077:
> 4075:
> 4076: BarrierSetNMethod* bs_nm =
> BarrierSet::barrier_set()->barrier_set_nmethod();
> 4077: if (bs_nm != nullptr) {
T
src/hotspot/cpu/x86/gc/shared/barrierSetAssembler_x86.cpp line 489:
> 487: __ load_klass(obj, obj, tmp1); // get klass
> 488: __ testptr(obj, obj);
> 489: __ jcc(Assembler::zero, error); // if klass is null it is broken
T
src/hotspot/cpu/x86/interp_masm_x86.cpp line 303:
> 301: jcc(Assembler::equal, L);
> 302: stop("InterpreterMacroAssembler::call_VM_base:"
> 303: " last_sp != null");
T
src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp line 191:
> 189: // Check for proper post_call_nop
> 190: NativePostCallNop* nop =
> nativePostCallNop_at(call->next_instruction_address());
> 191: if (nop == nullptr) {
T
src/hotspot/share/adlc/output_c.cpp line 279:
> 277: int max_stage = 0;
> 278: i = 0;
> 279: for (pipeline->_reslist.reset(); (resource =
> pipeline->_reslist.iter()) != nullptr;) {
T
src/hotspot/share/adlc/output_c.cpp line 305:
> 303: templen = 0;
> 304: i = 0;
> 305: for (pipeline->_reslist.reset(); (resource =
> pipeline->_reslist.iter()) != nullptr;) {
T
src/hotspot/share/adlc/output_c.cpp line 368:
> 366: const char* resource;
> 367: i = 0;
> 368: for (pipeline->_reslist.reset(); (resource =
> pipeline->_reslist.iter()) != nullptr;) {
T
src/hotspot/share/adlc/output_c.cpp line 393:
> 391:
> 392: i = 0;
> 393: for (pipeline->_reslist.reset(); (resource =
> pipeline->_reslist.iter()) != nullptr;) {
T
src/hotspot/share/adlc/output_c.cpp line 1009:
> 1007: const char* resource;
> 1008: i = 0;
> 1009: for (_pipeline->_reslist.reset(); (resource =
> _pipeline->_reslist.iter()) != nullptr;) {
T
src/hotspot/share/cds/filemap.cpp line 363:
> 361:
> 362: void SharedClassPathEntry::copy_from(SharedClassPathEntry* ent,
> ClassLoaderData* loader_data, TRAPS) {
> 363: assert(ent != nullptr, "sanity");
T
src/hotspot/share/classfile/stringTable.hpp line 150:
> 148: static size_t shared_entry_count() NOT_CDS_JAVA_HEAP_RETURN_(0);
> 149: static void allocate_shared_strings_array(TRAPS)
> NOT_CDS_JAVA_HEAP_RETURN;
> 150: static oop init_shared_table(const DumpedInternedStrings*
> dumped_interned_strings) NOT_CDS_JAVA_HEAP_RETURN_(nullptr);
T
src/hotspot/share/code/compiledIC.hpp line 93:
> 91: CompiledICHolder*claim_cached_icholder() {
> 92: assert(_is_icholder, "");
> 93: assert(_cached_value != nullptr, "must be non-null");
T
src/hotspot/share/code/compiledIC.hpp line 342:
> 340: // Code
> 341:
> 342: // Returns null if CodeBuffer::expand fails
T
src/hotspot/share/compiler/compileBroker.cpp line 388:
> 386:
> 387: MonitorLocker locker(MethodCompileQueue_lock);
> 388: // If _first is null we have no more compile jobs. There are two
> reasons for
T
src/hotspot/share/gc/x/xBarrier.cpp line 242:
> 240: oop XBarrier::weak_load_barrier_on_oop_field_preloaded(volatile
> narrowOop* p, oop o) {
> 241: ShouldNotReachHere();
> 242: return nullptr;
T
src/hotspot/share/gc/x/xBarrierSet.inline.hpp line 187:
> 185: // No check cast, bulk barri
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v8]
On Mon, 8 May 2023 10:09:29 GMT, Johan Sjölen wrote: >> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory >> share/jfr/. Unfortunately the script that does the change isn't perfect, and >> so we >> need to comb through these manually to make sure nothing has gone wrong. I >> also review these changes but things slip past my eyes sometimes. >> >> Here are some typical things to look out for: >> >> No changes but copyright header changed (probably because I reverted >> some changes but forgot the copyright). >> Macros having their NULL changed to nullptr, these are added to the >> script when I find them. They should be NULL. >> nullptr in comments and logs. We try to use lower case "null" in these >> cases as it reads better. An exception is made when code expressions are in >> a comment. >> >> An example of this: >> >> // This function returns null >> void* ret_null(); >> // This function returns true if *x == nullptr >> bool is_nullptr(void** x); >> >> Note how nullptr participates in a code expression here, we really are >> talking about the specific value nullptr. >> >> Thanks! > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Dead assert Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/12034#issuecomment-1542129940
Integrated: JDK-8300245: Replace NULL with nullptr in share/jfr/
On Tue, 17 Jan 2023 11:26:12 GMT, Johan Sjölen wrote: > Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory > share/jfr/. Unfortunately the script that does the change isn't perfect, and > so we > need to comb through these manually to make sure nothing has gone wrong. I > also review these changes but things slip past my eyes sometimes. > > Here are some typical things to look out for: > > No changes but copyright header changed (probably because I reverted some > changes but forgot the copyright). > Macros having their NULL changed to nullptr, these are added to the > script when I find them. They should be NULL. > nullptr in comments and logs. We try to use lower case "null" in these > cases as it reads better. An exception is made when code expressions are in a > comment. > > An example of this: > > // This function returns null > void* ret_null(); > // This function returns true if *x == nullptr > bool is_nullptr(void** x); > > Note how nullptr participates in a code expression here, we really are > talking about the specific value nullptr. > > Thanks! This pull request has now been integrated. Changeset: cc396895 Author:Johan Sjölen URL: https://git.openjdk.org/jdk/commit/cc396895e5a1dac49f4e341ce91c04b8c092d0af Stats: 2070 lines in 125 files changed: 1 ins; 5 del; 2064 mod 8300245: Replace NULL with nullptr in share/jfr/ Reviewed-by: mgronlun, coleenp - PR: https://git.openjdk.org/jdk/pull/12034
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v8]
On Mon, 8 May 2023 10:09:29 GMT, Johan Sjölen wrote: >> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory >> share/jfr/. Unfortunately the script that does the change isn't perfect, and >> so we >> need to comb through these manually to make sure nothing has gone wrong. I >> also review these changes but things slip past my eyes sometimes. >> >> Here are some typical things to look out for: >> >> No changes but copyright header changed (probably because I reverted >> some changes but forgot the copyright). >> Macros having their NULL changed to nullptr, these are added to the >> script when I find them. They should be NULL. >> nullptr in comments and logs. We try to use lower case "null" in these >> cases as it reads better. An exception is made when code expressions are in >> a comment. >> >> An example of this: >> >> // This function returns null >> void* ret_null(); >> // This function returns true if *x == nullptr >> bool is_nullptr(void** x); >> >> Note how nullptr participates in a code expression here, we really are >> talking about the specific value nullptr. >> >> Thanks! > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Dead assert Passes tier1. - PR Comment: https://git.openjdk.org/jdk/pull/12034#issuecomment-1538178989
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v7]
On Mon, 8 May 2023 10:00:45 GMT, Johan Sjölen wrote: >> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory >> share/jfr/. Unfortunately the script that does the change isn't perfect, and >> so we >> need to comb through these manually to make sure nothing has gone wrong. I >> also review these changes but things slip past my eyes sometimes. >> >> Here are some typical things to look out for: >> >> No changes but copyright header changed (probably because I reverted >> some changes but forgot the copyright). >> Macros having their NULL changed to nullptr, these are added to the >> script when I find them. They should be NULL. >> nullptr in comments and logs. We try to use lower case "null" in these >> cases as it reads better. An exception is made when code expressions are in >> a comment. >> >> An example of this: >> >> // This function returns null >> void* ret_null(); >> // This function returns true if *x == nullptr >> bool is_nullptr(void** x); >> >> Note how nullptr participates in a code expression here, we really are >> talking about the specific value nullptr. >> >> Thanks! > > Johan Sjölen has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 11 commits: > > - Missing fixes > - Merge branch 'master' into JDK-8300245 > - It's impossible for an array to be nullptr, remove asserts. > >Fails build on Clang systems > - Merge remote-tracking branch 'origin/master' into JDK-8300245 > - Fix outdated copyright > - Manual fix > - Fix two faulty NULL_STRING misses > - More manual fixes > - Merge remote-tracking branch 'origin/master' into JDK-8300245 > - Manual fixes > - ... and 1 more: https://git.openjdk.org/jdk/compare/5c7ede94...cb705720 Alright, @mgronlun, @egahlin, looks like the JFR null conversion is done. I'm currently running the tier1 tests. I removed a few asserts that didn't do anything (Clang complains about unnecessary comparisons), see here: https://github.com/openjdk/jdk/pull/12034/commits/2da97d578cf008170a0881d44844516370a6f456 - PR Comment: https://git.openjdk.org/jdk/pull/12034#issuecomment-1538107444
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v8]
> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory > share/jfr/. Unfortunately the script that does the change isn't perfect, and > so we > need to comb through these manually to make sure nothing has gone wrong. I > also review these changes but things slip past my eyes sometimes. > > Here are some typical things to look out for: > > No changes but copyright header changed (probably because I reverted some > changes but forgot the copyright). > Macros having their NULL changed to nullptr, these are added to the > script when I find them. They should be NULL. > nullptr in comments and logs. We try to use lower case "null" in these > cases as it reads better. An exception is made when code expressions are in a > comment. > > An example of this: > > // This function returns null > void* ret_null(); > // This function returns true if *x == nullptr > bool is_nullptr(void** x); > > Note how nullptr participates in a code expression here, we really are > talking about the specific value nullptr. > > Thanks! Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Dead assert - Changes: - all: https://git.openjdk.org/jdk/pull/12034/files - new: https://git.openjdk.org/jdk/pull/12034/files/cb705720..6018ab38 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12034&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12034&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12034.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12034/head:pull/12034 PR: https://git.openjdk.org/jdk/pull/12034
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v7]
> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory > share/jfr/. Unfortunately the script that does the change isn't perfect, and > so we > need to comb through these manually to make sure nothing has gone wrong. I > also review these changes but things slip past my eyes sometimes. > > Here are some typical things to look out for: > > No changes but copyright header changed (probably because I reverted some > changes but forgot the copyright). > Macros having their NULL changed to nullptr, these are added to the > script when I find them. They should be NULL. > nullptr in comments and logs. We try to use lower case "null" in these > cases as it reads better. An exception is made when code expressions are in a > comment. > > An example of this: > > // This function returns null > void* ret_null(); > // This function returns true if *x == nullptr > bool is_nullptr(void** x); > > Note how nullptr participates in a code expression here, we really are > talking about the specific value nullptr. > > Thanks! Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Missing fixes - Merge branch 'master' into JDK-8300245 - It's impossible for an array to be nullptr, remove asserts. Fails build on Clang systems - Merge remote-tracking branch 'origin/master' into JDK-8300245 - Fix outdated copyright - Manual fix - Fix two faulty NULL_STRING misses - More manual fixes - Merge remote-tracking branch 'origin/master' into JDK-8300245 - Manual fixes - ... and 1 more: https://git.openjdk.org/jdk/compare/5c7ede94...cb705720 - Changes: https://git.openjdk.org/jdk/pull/12034/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12034&range=06 Stats: 2070 lines in 125 files changed: 1 ins; 4 del; 2065 mod Patch: https://git.openjdk.org/jdk/pull/12034.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12034/head:pull/12034 PR: https://git.openjdk.org/jdk/pull/12034
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v6]
> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory > share/jfr/. Unfortunately the script that does the change isn't perfect, and > so we > need to comb through these manually to make sure nothing has gone wrong. I > also review these changes but things slip past my eyes sometimes. > > Here are some typical things to look out for: > > No changes but copyright header changed (probably because I reverted some > changes but forgot the copyright). > Macros having their NULL changed to nullptr, these are added to the > script when I find them. They should be NULL. > nullptr in comments and logs. We try to use lower case "null" in these > cases as it reads better. An exception is made when code expressions are in a > comment. > > An example of this: > > // This function returns null > void* ret_null(); > // This function returns true if *x == nullptr > bool is_nullptr(void** x); > > Note how nullptr participates in a code expression here, we really are > talking about the specific value nullptr. > > Thanks! Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: It's impossible for an array to be nullptr, remove asserts. Fails build on Clang systems - Changes: - all: https://git.openjdk.org/jdk/pull/12034/files - new: https://git.openjdk.org/jdk/pull/12034/files/9fc99f4a..2da97d57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12034&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12034&range=04-05 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12034.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12034/head:pull/12034 PR: https://git.openjdk.org/jdk/pull/12034
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v5]
> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory > share/jfr/. Unfortunately the script that does the change isn't perfect, and > so we > need to comb through these manually to make sure nothing has gone wrong. I > also review these changes but things slip past my eyes sometimes. > > Here are some typical things to look out for: > > No changes but copyright header changed (probably because I reverted some > changes but forgot the copyright). > Macros having their NULL changed to nullptr, these are added to the > script when I find them. They should be NULL. > nullptr in comments and logs. We try to use lower case "null" in these > cases as it reads better. An exception is made when code expressions are in a > comment. > > An example of this: > > // This function returns null > void* ret_null(); > // This function returns true if *x == nullptr > bool is_nullptr(void** x); > > Note how nullptr participates in a code expression here, we really are > talking about the specific value nullptr. > > Thanks! Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge remote-tracking branch 'origin/master' into JDK-8300245 - Fix outdated copyright - Manual fix - Fix two faulty NULL_STRING misses - More manual fixes - Merge remote-tracking branch 'origin/master' into JDK-8300245 - Manual fixes - Replace NULL with nullptr in share/jfr/ - Changes: https://git.openjdk.org/jdk/pull/12034/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12034&range=04 Stats: 2065 lines in 125 files changed: 0 ins; 0 del; 2065 mod Patch: https://git.openjdk.org/jdk/pull/12034.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12034/head:pull/12034 PR: https://git.openjdk.org/jdk/pull/12034
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v4]
On Tue, 24 Jan 2023 21:10:31 GMT, Johan Sjölen wrote: >> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory >> share/jfr/. Unfortunately the script that does the change isn't perfect, and >> so we >> need to comb through these manually to make sure nothing has gone wrong. I >> also review these changes but things slip past my eyes sometimes. >> >> Here are some typical things to look out for: >> >> No changes but copyright header changed (probably because I reverted >> some changes but forgot the copyright). >> Macros having their NULL changed to nullptr, these are added to the >> script when I find them. They should be NULL. >> nullptr in comments and logs. We try to use lower case "null" in these >> cases as it reads better. An exception is made when code expressions are in >> a comment. >> >> An example of this: >> >> // This function returns null >> void* ret_null(); >> // This function returns true if *x == nullptr >> bool is_nullptr(void** x); >> >> Note how nullptr participates in a code expression here, we really are >> talking about the specific value nullptr. >> >> Thanks! > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Fix outdated copyright Not yet. - PR Comment: https://git.openjdk.org/jdk/pull/12034#issuecomment-1477547098
