Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v3]
On Thu, 9 May 2024 18:31:24 GMT, Matias Saavedra Silva
wrote:
>> The beginning of the RW region contains pointers to c++ vtables which are
>> always located at a fixed offset from the shared base address at runtime.
>> This offset can be calculated at dumptime and stored with the read-only
>> tables at the top of the RO region. As a further improvement, all the
>> pointers to RO tables are replaced with offsets as well.
>>
>> These changes will reduce the number of pointers in the RW and RO regions
>> and will allow for the relocation bitmap size optimizations to be more
>> effective. Verified with tier 1-5 tests.
>
> Matias Saavedra Silva 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 12 additional
> commits since the last revision:
>
> - Merge branch 'master' into pointer_to_offset_8329418
> - Ioi comments
> - Chris comments and cleanup
> - Merge branch 'master' into pointer_to_offset_8329418
> - Cleanup
> - Corrected SA
> - Editing SA
> - Fixed dynamic dumping
> - Now works with -Xshare:on
> - Adjusted serialization
> - ... and 2 more: https://git.openjdk.org/jdk/compare/c6bab5bc...11f39483
LGTM. Just one small nit.
src/hotspot/share/cds/serializeClosure.hpp line 52:
> 50:
> 51: // Iterate on the pointers from p[0] through p[num_pointers-1]
> 52: void do_ptrs(u_char* start, size_t size) {
I think it will be more consistent if we use the same `void** p` parameter as
in `do_ptr()`. The `u_char*` here is a historical oddity and should be fixed.
-
Marked as reviewed by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19107#pullrequestreview-2048708441
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1595908706
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v2]
On Tue, 7 May 2024 16:38:23 GMT, Matias Saavedra Silva
wrote:
>> The beginning of the RW region contains pointers to c++ vtables which are
>> always located at a fixed offset from the shared base address at runtime.
>> This offset can be calculated at dumptime and stored with the read-only
>> tables at the top of the RO region. As a further improvement, all the
>> pointers to RO tables are replaced with offsets as well.
>>
>> These changes will reduce the number of pointers in the RW and RO regions
>> and will allow for the relocation bitmap size optimizations to be more
>> effective. Verified with tier 1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Chris comments and cleanup
Looks good. I will suggest making some clean up to improve the readability of
the existing code.
src/hotspot/share/cds/archiveUtils.cpp line 325:
> 323: assert((intptr_t)obj >= 0 || (intptr_t)obj < -100,
> 324: "hit tag while initializing ptrs.");
> 325: *p = obj != 0 ? (void*)(SharedBaseAddress + obj) : (void*)obj;
`nullptr` should be used instead of `0`. E.g., `obj != (void*)nullptr`
src/hotspot/share/cds/cppVtables.cpp line 236:
> 234: if (!soc->reading()) {
> 235: _vtables_serialized_base = soc->region_top();
> 236: }
The new `region_top()` API may be confusing with the existing `do_region()`
API, which has a completely different meaning for `region`.
I think it's better to rename `do_region()` to
// iterate on the pointers from p[0] through p[num_pointers-1]
SerializeClosure do_ptrs(void** p, int num_pointers);
Also, there's no need to add a new `region_top()` API -- there are already too
many functions that deal with a "region" of different types, and you need to
wonder what this particular "region" is.
`soc->region_top()` can be replaced with
`ArchiveBuilder::current()->current_dump_space()->top()`
Also, in archiveBuilder.hpp, `dump_space` means the same thing as
`dump_region`. All of the former should be changed to the latter for uniformity.
-
Changes requested by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19107#pullrequestreview-2046836828
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1594750309
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1594766914
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v4]
On Tue, 2 Apr 2024 20:18:34 GMT, Kim Barrett wrote: > > Thanks for reviewing, Kim. Is your suggestion to not have a JVMFlag object > > for develop flags in PRODUCT builds? Presumably to save some footprint? I'm > > not sure we would win fighting the macros to accomplish this. > > Yes, that's the suggestion and the rationale for it. It should also remove > the need for is_constant_in_binary. I don't know how hard it would actually > be to accomplish this. I agree it might not be worth the effort, but we won't > know until someone looks, which I haven't done. It might even be easy. Currently the VM prints an error message for non-product flags, so we need to keep some information about them. We can probably skip the type information, etc, to save a little space, but the space saving would be minimal. $ java -XX:+LoomDeoptAfterThaw --version Error: VM option 'LoomDeoptAfterThaw' is develop and is available only in debug version of VM. Improperly specified VM option 'LoomDeoptAfterThaw' Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2033183718
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v2]
On Tue, 2 Apr 2024 16:21:15 GMT, Coleen Phillimore wrote: >> Remove the notproduct distinction for command line options, rather than >> trying to wrestle the macros to fix the bug that they've been treated as >> develop options for some time now. This simplifies the command line option >> macros. >> >> Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Clean up notproduct from tests. LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18541#pullrequestreview-1974322553
Re: RFR: 8236736: Change notproduct JVM flags to develop flags
On Thu, 28 Mar 2024 22:53:22 GMT, Coleen Phillimore wrote: > Remove the notproduct distinction for command line options, rather than > trying to wrestle the macros to fix the bug that they've been treated as > develop options for some time now. This simplifies the command line option > macros. > > Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah. LGTM. For the past 15 years, "notproduct" flags haven't been working as they claim to be in globals.hpp. That doesn't seem to have bothered anyone. This definitely looks like a design that no one needs and should be removed for simplcity. There are some references to "notproduct" in test/hotspot/jtreg/runtime/CommandLine that need to be removed. - Changes requested by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18541#pullrequestreview-1974267126
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Tue, 30 Jan 2024 12:13:00 GMT, Andrew Haley wrote: > > The only "perfect" solution is putting the HotSpot code in a namespace. > > This is going to be a huge undertaking. I don't think we have enough > > interest in the OpenJDK community to make such a change now. > > I don't think that putting all of the HotSpot code in a namespace. At least, > I hope not: it'll mess up debugging so much that it'll be intolerable, IMO, > and there will be other side effects. I forgot to qualify "perfect" only in the sense of isolating the HotSpot symbols. It's obviously not perfect at all in other aspects. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1916772980
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Tue, 30 Jan 2024 10:27:08 GMT, Andrew Haley wrote: > > > > Maybe we could live with symbol redefinition using #define > > > > (conditionally for static linking in OpenJDK, as Coleen suggested > > > > earlier) for now, until the tooling can support symbol localizing > > > > better. Then localizing symbols using tools like `objcopy` can be the > > > > longer term and cleaner solution, instead of using namespace. What's > > > > your thoughts on that? > > > > > > > > > I suppose so, but why? > > > Why should any of this have to work on old systems? If their binutils is > > > broken, static linking of openjdk won't work there. > > > > > > We ran into issues with older gcc on linux-aarch for partial linking, but > > the problem may not be older gcc only(?). At the current stage, limiting > > static/hermetic Java runtime support to only the platforms that support > > partial linking and `objcopy` seems to be overly restrictive (it does > > simplify the requirements significantly however :-)): > > The duplicate symbol problems are mostly found in JDK natives and have been > > resolved already. We've found very few symbol issues with hotspot code so > > far. As there are portable alternative solutions that can resolve the > > symbol issues in hotspot, choosing a less portable solution seems not too > > attractive currently. > > I believe this to be a mistake. HotSpot, by design, exports only the symbols > intended for use by other components. Many of the symbol names are highly > generic, and will conflict with application code. > > Sure, you have enough to be able to do some prototyping, but for real-world > deployment you must be able to control symbol exports. I agree with Andrew. We don't want the perfect to be the enemy of the good. The only "perfect" solution is putting the HotSpot code in a namespace. This is going to be a huge undertaking. I don't think we have enough interest in the OpenJDK community to make such a change now. I think partial linking with objcopy is a clean solution that's good enough for the actual use cases. If someone wants to use `#define`, they can just make a local branch and add a few `#define` lines in their globalDefinitions.hpp. I suspect the configure script also allows adding C compiler options like `-DThread=HSThread`. `#define` is going to be a whack-a-mole hack. Google may need to isolate the `Thread` symbol, but other people may need to isolate things like `Symbol`, etc. It's not a good idea to add arbitrary `#define` in the HotSpot source code just because someone doesn't like it. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1916692309
Re: RFR: 8323546: Cleanup jcmd docs for Compiler.perfmap and VM.cds filename parameter [v6]
On Wed, 17 Jan 2024 02:26:22 GMT, Chris Plummer wrote: >> The jcmd docs for Compiler.perfmap currently say: >> >> - *filename*: (Optional) The name of the map file (STRING, no default >> value) >> >> However, there is a default, so not only should that be made more clear in >> the above, but also some descriptive text as to how the default is generated >> should be added. >> >> VM.cds has a similar issue, but already has the descriptive text, so just >> the "no default value" part needs to be fixed. >> >> Another change needed is to consistently use *filename* (italics) instead of >> `filename` (monospace). Note this is how html formatting is done. For the >> man page formatting, *filename* does no formatting and `filename` is >> displayed in color if supported. Personally I prefer `filename`, but it >> seems that there is already a strong precedence for using italics in the >> *arguments* list. For example: >> >> *arguments*: >> >> - *flag name*: The name of the flag that you want to set (STRING, no >> default value) >> >> - *string value*: (Optional) The value that you want to set (STRING, no >> default value) > > Chris Plummer has updated the pull request incrementally with two additional > commits since the last revision: > > - Get rid of extra empty line. > - Remove quotes from around filename to be consistent with how jcmd STRING > defaults are printed. LGTM. I think having one test case for perfmap should be enough. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17359#pullrequestreview-1826632142
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v4]
On Mon, 15 Jan 2024 07:32:31 GMT, David Holmes wrote:
>> This check is problematic:
>>
>>
>> if (strncmp(filename, DEFAULT_PERFMAP_FILENAME,
>> strlen(DEFAULT_PERFMAP_FILENAME)) == 0)
>>
>>
>> Because it cannot tell whether `filename` was explicitly given by the user.
>> As a result, if the user specifies:
>>
>>
>> jcmd 1234 perfmap '/tmp/perf-.map'
>>
>>
>> the file will be written as `/tmp/perf-1234.map`, but if the user specifies
>>
>>
>> jcmd 1234 perfmap '/tmp/-perf.map'
>>
>>
>> then the file will be written as `/tmp/-perf.map`.
>>
>> This gives the confusing impression that the string `` is sometimes
>> substituted and sometimes not.
>>
>> I think it's better to do this (similarly for the `VM.cds` case):
>>
>>
>> void PerfMapDCmd::execute(DCmdSource source, TRAPS) {
>> CodeCache::write_perf_map(_filename.is_set() ? _filename.value() :
>> nullptr);
>> }
>>
>>
>> This essentially makes the JVM behavior exactly the same as before, so I
>> think we can change the JBS issue title to something like "Clarify docs for
>> filename parameter to Compiler.perfmap and VM.cds".
>
> @iklam but IIUC that does not address the issue of updationg the help output.
My suggestion is to be used in combination with this change:
_filename("filename", "Name of the map file", "STRING", false,
DEFAULT_PERFMAP_FILENAME)
So the help message will be printed as:
*filename*: (Optional) The name of the map file (STRING /tmp/perf-.map)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17359#discussion_r1452040385
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v4]
On Mon, 15 Jan 2024 01:29:47 GMT, David Holmes wrote:
>> The default value for the argument is what gets displayed in the help text.
>> For example:
>>
>>
>> ThreadDumpToFileDCmd::ThreadDumpToFileDCmd(outputStream* output, bool heap) :
>>DCmdWithParser(output, heap),
>> _overwrite("-overwrite", "May overwrite existing file", "BOOLEAN", false,
>> "false"),
>> _format("-format", "Output format ("plain" or "json")", "STRING", false,
>> "plain"),
>> _filepath("filepath", "The file path to the output file", "STRING", true) {
>> _dcmdparser.add_dcmd_option(&_overwrite);
>> _dcmdparser.add_dcmd_option(&_format);
>> _dcmdparser.add_dcmd_argument(&_filepath);
>> }
>>
>>
>> And the help text:
>>
>>
>> Options: (options must be specified using the or = syntax)
>> -overwrite : [optional] May overwrite existing file (BOOLEAN, false)
>> -format : [optional] Output format ("plain" or "json") (STRING, plain)
>>
>>
>> The help output that indicates that "plain" is the default format comes from
>> the intialization of the _format argument. There is no separate help text.
>
> Ugghh! So the help text is an actual stringification of the actual default
> value of the "field", whereas in this case the real default value comes from
> passing null to `CodeCache::write_perf_map`. So we need this hack to deal
> with that. That is truly awful IMO. The only way to cleanly address that is
> to expand things so that you can set an actual help string to be used.
This check is problematic:
if (strncmp(filename, DEFAULT_PERFMAP_FILENAME,
strlen(DEFAULT_PERFMAP_FILENAME)) == 0)
Because it cannot tell whether `filename` was explicitly given by the user. As
a result, if the user specifies:
jcmd 1234 perfmap '/tmp/perf-.map'
the file will be written as `/tmp/perf-1234.map`, but if the user specifies
jcmd 1234 perfmap '/tmp/-perf.map'
then the file will be written as `/tmp/-perf.map`.
This gives the confusing impression that the string `` is sometimes
substituted and sometimes not.
I think it's better to do this (similarly for the `VM.cds` case):
void PerfMapDCmd::execute(DCmdSource source, TRAPS) {
CodeCache::write_perf_map(_filename.is_set() ? _filename.value() : nullptr);
}
This essentially makes the JVM behavior exactly the same as before, so I think
we can change the JBS issue title to something like "Clarify docs for filename
parameter to Compiler.perfmap and VM.cds".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17359#discussion_r1451918737
Integrated: 8320935: Move CDS config initialization code to cdsConfig.cpp
On Tue, 28 Nov 2023 23:24:53 GMT, Ioi Lam wrote: > This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. This pull request has now been integrated. Changeset: 4c96aac9 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/4c96aac9c0aa450b0b6859ded8dfff856222ad58 Stats: 696 lines in 8 files changed: 346 ins; 327 del; 23 mod 8320935: Move CDS config initialization code to cdsConfig.cpp Reviewed-by: ccheung, matsaave, stuefe - PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v3]
On Sat, 2 Dec 2023 03:36:05 GMT, Calvin Cheung wrote: >> Ioi Lam has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains seven additional commits >> since the last revision: >> >> - Merge branch 'master' into 8320935-move-cds-config-code-from-arguments-cpp >> - fixed indentation >> - code alignment >> - step4 >> - step3 >> - step2 >> - step1 > > Marked as reviewed by ccheung (Reviewer). Thanks @calvinccheung @matias9927 @tstuefe for the review. - PR Comment: https://git.openjdk.org/jdk/pull/16868#issuecomment-1842107804
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v3]
> This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'master' into 8320935-move-cds-config-code-from-arguments-cpp - fixed indentation - code alignment - step4 - step3 - step2 - step1 - Changes: - all: https://git.openjdk.org/jdk/pull/16868/files - new: https://git.openjdk.org/jdk/pull/16868/files/01dd47bc..a080edeb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16868=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16868=01-02 Stats: 84382 lines in 1756 files changed: 39063 ins; 38780 del; 6539 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Wed, 29 Nov 2023 21:53:06 GMT, Calvin Cheung wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed indentation > > src/hotspot/share/cds/cdsConfig.cpp line 34: > >> 32: #include "logging/log.hpp" >> 33: #include "runtime/arguments.hpp" >> 34: #include "runtime/java.hpp" > > I was able to build with your patch without including `java.hpp`. > The #include java.hpp could also be removed from arguments.cpp. cdsConfig.cpp needs the declaration of `vm_exit_during_initialization()` from java.hpp. Although java.hpp is included by arguments.hpp, we usually try to avoid such indirectly inclusions. Otherwise if arguments.hpp is changed to no longer include java.hpp, then cdsConfig.hpp will fail to compile. I am not sure about arguments.cpp -- if java.hpp is already included by arguments.hpp, do we need to explicitly include it in arguments.cpp? I'll leave that alone in this PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412682898
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Fri, 1 Dec 2023 22:04:22 GMT, Matias Saavedra Silva
wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed indentation
>
> src/hotspot/share/cds/cdsConfig.cpp line 101:
>
>> 99: void CDSConfig::extract_shared_archive_paths(const char* archive_path,
>> 100: char** base_archive_path,
>> 101: char** top_archive_path) {
>
> Could you align these arguments?
Fixed.
> src/hotspot/share/cds/cdsConfig.cpp line 125:
>
>> 123: }
>> 124:
>> 125: void CDSConfig::init_shared_archive_paths() {
>
> Now that I see this there is a lot of indentation thanks to the nested
> conditionals. I don't have much to offer but is there a cleaner way to format
> this method? Maybe you can extract the code in `if (archives == 1)` into its
> own method for better readability.
I want to keep the code change minimal while moving code from one file to
another. I'll refactor this function in a follow-on PR. That way it will be
easier to track the code history.
> src/hotspot/share/runtime/arguments.cpp line 1262:
>
>> 1260: }
>> 1261:
>> 1262: CDSConfig::check_system_property(key, value);
>
> I see this is only called once, do you expect this method to be used again?
> It may be unnecessary to extract this code into its own method.
I wanted to move the code from arguments.cpp to cdsConfig.cpp, so I had to put
it in a new function.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683767
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683760
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683786
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
> This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: fixed indentation - Changes: - all: https://git.openjdk.org/jdk/pull/16868/files - new: https://git.openjdk.org/jdk/pull/16868/files/72f3e44c..01dd47bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16868=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16868=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp
This is a simple clean up that moves the code for initializing the CDS config states from arguments.cpp to cdsConfig.cpp I renamed a few functions, but otherwise the code is unchanged. - `get_default_shared_archive_path()` -> `default_archive_path()` - `GetSharedArchivePath()` -> `static_archive_path()` - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is compiled only if CDS is enabled. - Commit messages: - code alignment - step4 - step3 - step2 - step1 Changes: https://git.openjdk.org/jdk/pull/16868/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16868=00 Issue: https://bugs.openjdk.org/browse/JDK-8320935 Stats: 696 lines in 8 files changed: 346 ins; 327 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8318484: Initial version of cdsConfig.hpp [v2]
On Thu, 19 Oct 2023 06:58:22 GMT, David Holmes wrote: >> Ioi Lam has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains two additional commits >> since the last revision: >> >> - Merge branch 'master' into 8318484-initial-version-of-cdsConfig-hpp >> - 8318484: Initial version of cdsConfig.hpp > > Initial refactoring looks good. One query below. > > Thanks Thanks @dholmes-ora @calvinccheung @sspitsyn for the review. - PR Comment: https://git.openjdk.org/jdk/pull/16257#issuecomment-1773834764
Integrated: 8318484: Initial version of cdsConfig.hpp
On Thu, 19 Oct 2023 05:56:53 GMT, Ioi Lam wrote:
> This is the first step for [JDK-8318483 - Move CDS configuration management
> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>
> - Remove `Arguments::is_dumping_archive()` and `Arguments
> assert_is_dumping_archive()`
> - Add the following new APIs
>
>
> class CDSConfig {
> static bool is_dumping_archive();
> static bool is_dumping_static_archive();
> static bool is_dumping_dynamic_archive();
> static bool is_dumping_heap();
> };
>
>
> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
> these new APIs
>
> (More APIs will be added in future sub tasks of
> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
This pull request has now been integrated.
Changeset: ecd25e7d
Author:Ioi Lam
URL:
https://git.openjdk.org/jdk/commit/ecd25e7d6f9d69f9dbdbff0a4a9b9d6b19288593
Stats: 236 lines in 36 files changed: 125 ins; 16 del; 95 mod
8318484: Initial version of cdsConfig.hpp
Reviewed-by: dholmes, ccheung, sspitsyn
-
PR: https://git.openjdk.org/jdk/pull/16257
Re: RFR: 8318484: Initial version of cdsConfig.hpp [v2]
> This is the first step for [JDK-8318483 - Move CDS configuration management
> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>
> - Remove `Arguments::is_dumping_archive()` and `Arguments
> assert_is_dumping_archive()`
> - Add the following new APIs
>
>
> class CDSConfig {
> static bool is_dumping_archive();
> static bool is_dumping_static_archive();
> static bool is_dumping_dynamic_archive();
> static bool is_dumping_heap();
> };
>
>
> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
> these new APIs
>
> (More APIs will be added in future sub tasks of
> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
Ioi Lam has updated the pull request with a new target base due to a merge or a
rebase. The incremental webrev excludes the unrelated changes brought in by the
merge/rebase. The pull request contains two additional commits since the last
revision:
- Merge branch 'master' into 8318484-initial-version-of-cdsConfig-hpp
- 8318484: Initial version of cdsConfig.hpp
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16257/files
- new: https://git.openjdk.org/jdk/pull/16257/files/0a2f78b0..0d729778
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=16257=01
- incr: https://webrevs.openjdk.org/?repo=jdk=16257=00-01
Stats: 5386 lines in 163 files changed: 3808 ins; 820 del; 758 mod
Patch: https://git.openjdk.org/jdk/pull/16257.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16257/head:pull/16257
PR: https://git.openjdk.org/jdk/pull/16257
Re: RFR: 8318484: Initial version of cdsConfig.hpp
On Thu, 19 Oct 2023 22:21:30 GMT, Calvin Cheung wrote:
>> This is the first step for [JDK-8318483 - Move CDS configuration management
>> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>>
>> - Remove `Arguments::is_dumping_archive()` and `Arguments
>> assert_is_dumping_archive()`
>> - Add the following new APIs
>>
>>
>> class CDSConfig {
>> static bool is_dumping_archive();
>> static bool is_dumping_static_archive();
>> static bool is_dumping_dynamic_archive();
>> static bool is_dumping_heap();
>> };
>>
>>
>> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
>> these new APIs
>>
>> (More APIs will be added in future sub tasks of
>> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
>
> src/hotspot/share/cds/cdsConfig.hpp line 39:
>
>> 37:
>> 38: // CDS archived heap
>> 39: static bool is_dumping_heap()
>> NOT_CDS_JAVA_HEAP_RETURN_(false);
>
> Too much blank spaces between the function declarations and NOT_CDS_* macros.
> The function declarations could also be shifted more to the left.
The spaces are for functions that will be added in the next PR that have longer
names:
// Basic CDS features
static bool is_dumping_archive()
NOT_CDS_RETURN_(false);
static bool is_dumping_static_archive()
NOT_CDS_RETURN_(false);
static bool is_dumping_dynamic_archive()
NOT_CDS_RETURN_(false);
// CDS archived heap
static bool is_dumping_heap()
NOT_CDS_JAVA_HEAP_RETURN_(false);
static bool is_loading_heap()
NOT_CDS_JAVA_HEAP_RETURN_(false);
static void disable_dumping_full_module_graph(const char* reason = nullptr)
NOT_CDS_JAVA_HEAP_RETURN;
static bool is_dumping_full_module_graph()
NOT_CDS_JAVA_HEAP_RETURN_(false);
static void disable_loading_full_module_graph(const char* reason = nullptr)
NOT_CDS_JAVA_HEAP_RETURN;
static bool is_loading_full_module_graph()
NOT_CDS_JAVA_HEAP_RETURN_(false);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16257#discussion_r1366367480
Re: RFR: 8318484: Initial version of cdsConfig.hpp
On Thu, 19 Oct 2023 06:54:05 GMT, David Holmes wrote:
>> This is the first step for [JDK-8318483 - Move CDS configuration management
>> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>>
>> - Remove `Arguments::is_dumping_archive()` and `Arguments
>> assert_is_dumping_archive()`
>> - Add the following new APIs
>>
>>
>> class CDSConfig {
>> static bool is_dumping_archive();
>> static bool is_dumping_static_archive();
>> static bool is_dumping_dynamic_archive();
>> static bool is_dumping_heap();
>> };
>>
>>
>> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
>> these new APIs
>>
>> (More APIs will be added in future sub tasks of
>> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
>
> src/hotspot/share/cds/metaspaceShared.cpp line 778:
>
>> 776:
>> 777: #if INCLUDE_CDS_JAVA_HEAP
>> 778: if (CDSConfig::is_dumping_heap()) {
>
> This seems a new condition. Why is it needed now?
This was a bug uncovered during refactoring.
`StringTable::allocate_shared_strings_array()` used to assert
`DumpSharedSpaces`. However, this function is useful only in a more limited
scope (`CDSConfig::is_dumping_heap()` which is a subset of `DumpSharedSpaces`).
So after changing the assert in
`StringTable::allocate_shared_strings_array()`, I have to change the condition
where this function is called.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16257#discussion_r1366041643
RFR: 8318484: Initial version of cdsConfig.hpp
This is the first step for [JDK-8318483 - Move CDS configuration management
into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
- Remove `Arguments::is_dumping_archive()` and `Arguments
assert_is_dumping_archive()`
- Add the following new APIs
class CDSConfig {
static bool is_dumping_archive();
static bool is_dumping_static_archive();
static bool is_dumping_dynamic_archive();
static bool is_dumping_heap();
};
- Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to these
new APIs
(More APIs will be added in future sub tasks of
[JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
-
Commit messages:
- 8318484: Initial version of cdsConfig.hpp
Changes: https://git.openjdk.org/jdk/pull/16257/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=16257=00
Issue: https://bugs.openjdk.org/browse/JDK-8318484
Stats: 236 lines in 36 files changed: 125 ins; 16 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/16257.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16257/head:pull/16257
PR: https://git.openjdk.org/jdk/pull/16257
Re: RFR: 8314550: [macosx-aarch64] serviceability/sa/TestJmapCore.java fails with "sun.jvm.hotspot.debugger.UnmappedAddressException: 801000800"
On Thu, 24 Aug 2023 23:31:56 GMT, Chris Plummer wrote: > On some macosx-aarch64 systems, not all mapped pages are dumped to the core > file. This first turned up with > [JDK-8293563](https://bugs.openjdk.org/browse/JDK-8293563) where large parts > of the ZGC heap would not be in the core file, leading to various SA address > errors. For JDK-8293563 the issue was addressed by having the core file test > always use `-XX:+AlwaysPreTouch` on macosx-aarch64. This seemed to force the > ZGC pages to always end up in the core file. > > A similar issue has been noticed with mapped in pages of the CDS archive. We > are seeing cases where SA references to addresses that are clearly in the CDS > archive (based on info in the hs_err file) are failing to be read from the > core file by SA. This problem has turned up a number of times during CI > testing, but I have yet to be able to reproduce it myself. This PR is an > attempt to address this testing issue by having the CDS archive also pretouch > all mapped in pages when `-XX:+AlwaysPreTouch` is used. > > Tested with tier1 and tier3 and also ran the test about 5,000 times with and > without the fix. It never reproduced for either. Hopefully the problem is > gone with this fix, but it may take a few months of CI testing before we can > be confident it is fixed. Looks good to me. The size of the CDS archive is usually much smaller than the committed heap size. The [GC tuning docs says](https://docs.oracle.com/en/java/javase/11/gctuning/garbage-first-garbage-collector-tuning.html#GUID-0770AB01-E334-4E23-B307-FD2114B16E0E): > You can use -XX:+AlwaysPreTouch to move the operating system work to back > virtual memory with physical memory to VM startup time. Both of these > measures can be particularly desirable in order to make pause-times more > consistent. So committing all the CDS regions to physical memory at VM start-up would be what the user expects when `-XX:+AlwaysPreTouch` is specified. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15423#pullrequestreview-1594875750
Re: RFR: JDK-8311870: Split CompressedKlassPointers from compressedOops.hpp
On Tue, 11 Jul 2023 11:20:19 GMT, Thomas Stuefe wrote: > In preparation for some Lilliput-related changes, I'd like to get some purely > mechanical code moves out of the way. It would also improve separation of > concerns and reduces include header bloat. > > In particular, this patch does: > > 1) Move `CompressedKlassPointers` from `compressedOops.(cpp|hpp|inline.hpp)` > to `compressedKlass.(cpp|hpp|inline.hpp)` > > 2) flatten the `NarrowPtrStruct _narrow_klass` to `address _base; int _shift` > (its implicit null check member is not needed for Klass and it has little > merit otherwise). > > 3) moved `narrowKlass` from `oopsHierarchy.hpp` to `compressedKlass.hpp` > > 4) remove `KlassAlignment` and `LogKlassAlignment` (the word-sized variants, > not xxxInBytes) since they are unused > > 5) Move `KlassEncodingMetaspaceMax`, `LogKlassAlignmentInBytes` and > `KlassAlignmentInBytes` to compressedKlass.hpp > > 6) Fixed all include issues (including existing missing includes) > > 7) Fixed VM struct because of (2) > > Note that nothing functional is changed. Looks reasonable to me. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14826#pullrequestreview-1525290781
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote:
>> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
>> I've added a pointer delta function in globalDefinitions.hpp to use for
>> these pointer diff calculations that return int everywhere. If the name is
>> agreeable, I'll fix the other cases of this like this. It's better than raw
>> casts.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use pointer_delta_as_int for the name that uses pointer_delta, fix negative
> case to just do checked_cast.
LGTM.
src/hotspot/share/utilities/globalDefinitions.hpp line 529:
> 527: template
> 528: inline int pointer_delta_as_int(const volatile T* left, const volatile
> T* right) {
> 529: return checked_cast(pointer_delta(left, right, sizeof(T)));
For clarity, I think you should add a comment saying the returned value is
always non-negative.
-
Marked as reviewed by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14675#pullrequestreview-1503498625
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245463745
Integrated: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
On Mon, 12 Jun 2023 19:52:36 GMT, Ioi Lam wrote: > resolvedIndyEntry.hpp was added in > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included in > the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included in > 807 out of about 1160 hotspot .o files. > > The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion > should be moved from cpCache.hpp to cpCache.inline.hpp. This improves hotspot > build time. > > After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. This pull request has now been integrated. Changeset: 5d193193 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/5d193193a3a4c519e7b3d77b27e6b2bf1b11c7f9 Stats: 86 lines in 35 files changed: 61 ins; 13 del; 12 mod 8309878: Reduce inclusion of resolvedIndyEntry.hpp Reviewed-by: coleenp, sspitsyn, matsaave - PR: https://git.openjdk.org/jdk/pull/14427
Re: RFR: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
On Mon, 12 Jun 2023 22:24:26 GMT, Serguei Spitsyn wrote: >> resolvedIndyEntry.hpp was added in >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included >> in the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included >> in 807 out of about 1160 hotspot .o files. >> >> The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion >> should be moved from cpCache.hpp to cpCache.inline.hpp. This improves >> hotspot build time. >> >> After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. > > Looks good. > Thanks, > Serguei Thanks @sspitsyn @coleenp @matias9927 for the review. - PR Comment: https://git.openjdk.org/jdk/pull/14427#issuecomment-1590238854
RFR: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
resolvedIndyEntry.hpp was added in [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included in the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included in 807 out of about 1160 hotspot .o files. The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion should be moved from cpCache.hpp to cpCache.inline.hpp. This improves hotspot build time. After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. - Commit messages: - fixed copyright - step2 - step1 Changes: https://git.openjdk.org/jdk/pull/14427/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14427=00 Issue: https://bugs.openjdk.org/browse/JDK-8309878 Stats: 86 lines in 35 files changed: 61 ins; 13 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/14427.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14427/head:pull/14427 PR: https://git.openjdk.org/jdk/pull/14427
Re: RFR: 8309673: Refactor ref_at methods in SA ConstantPool [v2]
On Fri, 9 Jun 2023 18:14:18 GMT, Matias Saavedra Silva wrote: >> The accessor methods in constantpool.cpp were previously cleaned up to allow >> for different types of indices to be used, distinguishing them by the >> bytecode. This patch adds the same changes to the hotspot serviceability >> agent code. Verified with tier 1-5 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Removed unnecessary assignment LGTM. I would suggest changing "Serviceability" in the title to "SA" - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14385#pullrequestreview-1472921430
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v4]
On Thu, 12 Jan 2023 16:52:50 GMT, Thomas Stuefe wrote: >> Curious, I always thought we do ArrayAllocator - using mmap for larger >> allocations - to prevent memory retention for libc variants whose allocators >> are "grabby", i.e. which don't promptly return memory to the OS on free(). >> E.g. because they only use sbrk (Solaris, AIX), or are just cautious about >> returning memory (glibc). >> >> Glibc's retention problem is only relevant for fine-grained allocations, so >> for glibc this is probably fine. This leaves at least AIX as a potential >> problem. @backwaterred, does the AIX libc malloc() still exclusively use the >> data segment ? Does free'd memory still stick to the process? >> >> (While writing this, I remember that we at SAP even rewrote Arena allocation >> to use mmap for AIX, because large compile arenas caused lasting RSS >> increase, so it has definitely been a problem in the past) > >> > To follow up on @tstuefe comment - and the one that I tried to say in the >> > bug was that we added this MmapArrayAllocate feature for some G1 marking >> > bits that used so much memory that hit the Solaris _sbrk issue. Maybe >> > @stefank and @tschatzl remember this issue. Maybe it's ok for AIX, then >> > removing this code is a good change. Maybe the G1 usages need a mmap >> > implementation though. >> >> The padding.inline.hpp usage seems to have one caller which is called once. >> The other mmap usage in G1 we can convert to mmap using a similar approach >> to zGranuleMap if that is preferred. That would then be equivalent behavior, >> it looks like the G1 code uses the page allocation granularity anyway so >> maybe keeping it mmap is the better way to go here anyway? > > My uninformed opinion (I'm not the G1 code owner) is that it would be fine to > use explicit mmap. I'd love the complexity reduction this patch brings. @tstuefe @backwaterred I'd like to see this RFE revived. Do we know if anyone is using the `ArrayAllocatorMallocLimit` flag in any production environment today? It seems unlikely to me, as you'd need to explicitly specify `-XX:+UnlockExperimentalVMOptions` in the command-line. And, if this option had been useful (for the AIX port, for example), it would have been changed to a non-experimental (with proper `_pd` support) option over the past 10 years. - PR Comment: https://git.openjdk.org/jdk/pull/11931#issuecomment-1557558015
Re: RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v4]
On Tue, 9 May 2023 14:02:26 GMT, Coleen Phillimore wrote: >> The ResourceHashtable conversion for JDK-8292741 didn't add the resizing >> code. The old hashtable code was tuned for resizing in anticipation of >> large hashtables for JVMTI tags. This patch ports over the old hashtable >> resizing code. It also adds a ResourceHashtable::put_fast() function that >> prepends to the bucket list, which is also reclaims the performance of the >> old hashtable for this test with 10M tags. The ResourceHashtable put >> function is really a put_if_absent. This can be cleaned up in a future >> change. Also, the remove function needed a lambda to destroy the >> WeakHandle, since resizing requires copying entries. >> >> Tested with JVMTI and JDI tests locally, and tier1-4 tests. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > One line and comment making obj null in copy constructor. LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13818#pullrequestreview-1419201972
Re: RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v2]
On Mon, 8 May 2023 13:59:06 GMT, Coleen Phillimore wrote: >> I would suggest `put_when_absent` to complement `put_if_absent` - with >> suitable descriptive comments of course. > > This is a good name. Updated. I cannot tell the difference between `put_when_absent` and `put_if_absent`. Grammatically they mean the same thing to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187952526
Re: RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v2]
On Fri, 5 May 2023 12:07:20 GMT, Coleen Phillimore wrote:
>> The ResourceHashtable conversion for JDK-8292741 didn't add the resizing
>> code. The old hashtable code was tuned for resizing in anticipation of
>> large hashtables for JVMTI tags. This patch ports over the old hashtable
>> resizing code. It also adds a ResourceHashtable::put_fast() function that
>> prepends to the bucket list, which is also reclaims the performance of the
>> old hashtable for this test with 10M tags. The ResourceHashtable put
>> function is really a put_if_absent. This can be cleaned up in a future
>> change. Also, the remove function needed a lambda to destroy the
>> WeakHandle, since resizing requires copying entries.
>>
>> Tested with JVMTI and JDI tests locally, and tier1-4 tests.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove return variable from remove lambda, fix formatting.
I can't comment on the JVMTI changes, but the changes in the hashtable code
seems OK to me.
src/hotspot/share/classfile/stringTable.cpp line 638:
> 636: public:
> 637: size_t _errors;
> 638: VerifyCompStrings() : _table(unsigned(_items_count / 8) + 1, 0 /* do
> not resize */), _errors(0) {}
Shouldn't this use a regular ResourceHashtable instead?
src/hotspot/share/utilities/resizeableResourceHash.hpp line 91:
> 89: // Calculate next "good" hashtable size based on requested count
> 90: int calculate_resize(bool use_large_table_sizes) const {
> 91: const int resize_factor = 2; // by how much we will resize using
> current number of entries
Does this function depend on the template parameters? If not, I think it can be
made a static function -- you may need to pass `BASE::number_of_entries()` in
as a parameter.
src/hotspot/share/utilities/resourceHash.hpp line 147:
> 145: */
> 146: bool put_fast(K const& key, V const& value) {
> 147: unsigned hv = HASH(key);
I think `put_fast` is not clear enough. Maybe `put_must_be_absent()` or
something more concise.
-
PR Review: https://git.openjdk.org/jdk/pull/13818#pullrequestreview-1416091781
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187009635
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187005281
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187009805
Integrated: 8305771: SA ClassWriter.java fails to skip overpass methods
On Tue, 25 Apr 2023 23:49:56 GMT, Ioi Lam wrote: > Please review this trivial fix. > > When checking for bits in `m.getAccessFlags()`, the mask > `JVM_RECOGNIZED_METHOD_MODIFIERS` should be applied (similar to other uses of > `m.getAccessFlags()` in ClassWriter.java This pull request has now been integrated. Changeset: 750bece0 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/750bece0c2f331025590e7358c7b69f4811f0d24 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8305771: SA ClassWriter.java fails to skip overpass methods Reviewed-by: kevinw, cjplummer - PR: https://git.openjdk.org/jdk/pull/13663
Re: RFR: 8305771: SA ClassWriter.java fails to skip overpass methods
On Wed, 26 Apr 2023 17:43:26 GMT, Kevin Walls wrote: >> Please review this trivial fix. >> >> When checking for bits in `m.getAccessFlags()`, the mask >> `JVM_RECOGNIZED_METHOD_MODIFIERS` should be applied (similar to other uses >> of `m.getAccessFlags()` in ClassWriter.java > > Marked as reviewed by kevinw (Committer). Thanks @kevinjwalls and @plummercj for the review. - PR Comment: https://git.openjdk.org/jdk/pull/13663#issuecomment-1524031268
RFR: 8305771: SA ClassWriter.java fails to skip overpass methods
Please review this trivial fix. When checking for bits in `m.getAccessFlags()`, the mask `JVM_RECOGNIZED_METHOD_MODIFIERS` should be applied (similar to other uses of `m.getAccessFlags()` in ClassWriter.java - Commit messages: - 8305771: SA ClassWriter.java fails to skip overpass methods Changes: https://git.openjdk.org/jdk/pull/13663/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13663=00 Issue: https://bugs.openjdk.org/browse/JDK-8305771 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13663.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13663/head:pull/13663 PR: https://git.openjdk.org/jdk/pull/13663
Integrated: 8298048: Combine CDS archive heap into a single block
On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam wrote: > This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1100 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 This pull request has now been integrated. Changeset: 723037a7 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/723037a79d2a43b9a1a247d8f81a47907faadab1 Stats: 3252 lines in 83 files changed: 159 ins; 2446 del; 647 mod 8298048: Combine CDS archive heap into a single block Co-authored-by: Thomas Schatzl Reviewed-by: matsaave, tschatzl - PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v8]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits: - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - Fixed assert in runtime/cds/appcds/SharedArchiveConsistency.java - Removal of JFR custom closed/open archive region types - Remove g1 full gc skip marking optimization - Some comment updates - Move g1collectedheap archive related regions together in the cpp file - Factor out region/range iteration - Fix comment - Ioi fix - ... and 12 more: https://git.openjdk.org/jdk/compare/f6336231...e8041d50 - Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13284=07 Stats: 3252 lines in 83 files changed: 159 ins; 2446 del; 647 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8306123: Move InstanceKlass writeable flags
On Tue, 18 Apr 2023 17:11:25 GMT, Coleen Phillimore wrote: > Please review this patch to move the writeable Klass AccessFlags to > InstanceKlassFlags. > Tested with tier1-4. LGTM. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13515#pullrequestreview-1390984891
Re: RFR: 8298048: Combine CDS archive heap into a single block [v7]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - Fixed assert in runtime/cds/appcds/SharedArchiveConsistency.java - Removal of JFR custom closed/open archive region types - Remove g1 full gc skip marking optimization - Some comment updates - Move g1collectedheap archive related regions together in the cpp file - Factor out region/range iteration - Fix comment - Ioi fix - fixed merge - ... and 11 more: https://git.openjdk.org/jdk/compare/0f3828dd...8a35c7ee - Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13284=06 Stats: 3252 lines in 83 files changed: 159 ins; 2446 del; 647 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967 [v3]
On Tue, 18 Apr 2023 15:22:02 GMT, Markus Grönlund wrote: >> Greetings, >> >> With [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967), much >> refactoring was done to the JVMTI code concerning agents. However, some >> platforms do not have JVMTI support, and tier5 of testing builds an embedded >> build, linux-arm32-open-cmp-baseline, which failed because the refactoring >> did not properly handle conditional compilations for JVMTI. >> >> JDK-8257967 did run tier5, but it used an existing build, so it did not >> cause recompilations of the embedded target :-( >> >> This changeset adds the conditional constructs to let >> linux-arm32-open-cmp-baseline build successfully. >> >> It does not look good, but there you go... >> >> Testing: >> >> Building: linux-arm32-open-cmp-baseline >> Building: regular platforms >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with one > additional commit since the last revision: > > SIZE_FORMAT LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13512#pullrequestreview-1390388100
Re: RFR: 8298048: Combine CDS archive heap into a single block [v6]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - fixed merge - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - removed G1CollectedHeap::fill_archive_regions() -- we no longer have unused space at the start of the "old" regions - Simplified of runtime range of the mapped archive heap - @ashu-mehra comments; some clean up - @ashu-mehra comments - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - more clean up: heap_regions -> heap_region, etc - @matias9927 comments - Remove archive region types from G1 - ... and 2 more: https://git.openjdk.org/jdk/compare/e3ece365...3543dd2a - Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13284=05 Stats: 3152 lines in 77 files changed: 121 ins; 2371 del; 660 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Tue, 11 Apr 2023 21:47:40 GMT, Ashutosh Mehra wrote: > cds changes look good! just few nitpicks. Thanks for the review. I've incorporated your suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1505698259
Re: RFR: 8298048: Combine CDS archive heap into a single block [v5]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra comments; some clean up - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/8ce6953e..30542e53 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13284=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13284=03-04 Stats: 11 lines in 3 files changed: 1 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v4]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra comments - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/b693d27c..8ce6953e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13284=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13284=02-03 Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v3]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - more clean up: heap_regions -> heap_region, etc - @matias9927 comments - Remove archive region types from G1 - clean up (1) - 8298048: Combine CDS archive heap into a single block - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/a1a3cac7..b693d27c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13284=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13284=01-02 Stats: 33051 lines in 911 files changed: 16125 ins; 14331 del; 2595 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v2]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request incrementally with two additional commits since the last revision: - more clean up: heap_regions -> heap_region, etc - @matias9927 comments - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/a852dfbb..a1a3cac7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13284=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13284=00-01 Stats: 116 lines in 12 files changed: 11 ins; 46 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v2]
On Fri, 7 Apr 2023 19:17:46 GMT, Matias Saavedra Silva
wrote:
>> Ioi Lam has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - more clean up: heap_regions -> heap_region, etc
>> - @matias9927 comments
>
> src/hotspot/share/cds/archiveBuilder.cpp line 1086:
>
>> 1084: p2i(to_requested(start)), size_t(end -
>> start));
>> 1085: log_data(start, end, to_requested(start), /*is_heap=*/true);
>> 1086: }
>
> These log messages can be placed inside the else case before the break
Fixed.
> src/hotspot/share/cds/archiveHeapWriter.cpp line 369:
>
>> 367: template void
>> ArchiveHeapWriter::store_requested_oop_in_buffer(T* buffered_addr,
>> 368:
>> oop request_oop) {
>> 369: //assert(is_in_requested_regions(request_oop), "must be");
>
> Some left over commented code. I assume this should be removed or a new
> assert should be here to replace it.
I fixed the assert.
> src/hotspot/share/cds/archiveHeapWriter.cpp line 529:
>
>> 527: num_non_null_ptrs ++;
>> 528:
>> 529: if (max_idx < idx) {
>
> Is there a built in min() function we can use here? Maybe std::min()?
Updated with the `MAX2()` macro.
> src/hotspot/share/cds/filemap.cpp line 1674:
>
>> 1672:
>> 1673: char* buffer = NEW_C_HEAP_ARRAY(char, size_in_bytes, mtClassShared);
>> 1674: size_t written = write_bitmap(ptrmap, buffer, 0);
>
> Maybe add a comment to clarify there is no offset? Constants in method
> parameters can be confusing sometimes.
I changed the code to pass "written" as a parameter similar to the other two
calls. Also added comments.
> src/hotspot/share/cds/filemap.cpp line 2035:
>
>> 2033: }
>> 2034: if (end < e) {
>> 2035: end = e;
>
> Like mentioned before, maybe we have max() and min() methods to use here.
I simplified the code -- there's only one range now so the start/end can be
easily determined.
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 520:
>
>> 518: } else {
>> 519: return true;
>> 520: }
>
> Maybe make this `return reserved.contains(range.start()) &&
> reserved.contains(range.last())`
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163601901
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163601972
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602280
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602339
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602400
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602433
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Sat, 8 Apr 2023 07:14:30 GMT, Thomas Stuefe wrote: > This looks like a nice simplification. Will you also combine all mappings at > OS level to a single one, so that you only need one mmap call? Now there's a single call to mmap() in FileMapInfo::map_heap_regions_impl_inner(). This reminds me that I should remove the "s" from "heap_regions" in the function names. Will do that in my next commit. - PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1503827701
RFR: 8298048: Combine CDS archive heap into a single block
This PR combines the "open" and "closed" regions of the CDS archive heap into a single region. This significantly simplifies the implementation, making it more compatible with non-G1 collectors. There's a net removal of ~1000 lines in src code and another ~1200 lines of tests. **Notes for reviewers:** - Most of the code changes in CDS are removing the handling of "open" vs "closed" objects. - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). - It might be easier to see the diff with whitespaces off. - There are two major changes in the G1 code - The archived objects are now stored in the "old" region (see g1CollectedHeap.cpp in [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) - The majority of the other changes are removal of the "archive" region type (see heapRegionType.hpp). For ease of review, such code is isolated in [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) - Testing changes: - Now the archived java objects can move, along with the "old" regions that contain them. It's no longer possible to test whether a heap object came from CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. - Many tests that uses this API are removed. Most of them were written during early development of CDS archived objects and are no longer relevant. **Testing:** - Mach5 tiers 1 ~ 7 - Commit messages: - Remove archive region types from G1 - clean up (1) - 8298048: Combine CDS archive heap into a single block Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13284=00 Issue: https://bugs.openjdk.org/browse/JDK-8298048 Stats: 2995 lines in 75 files changed: 110 ins; 2271 del; 614 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Integrated: 8305421: Work around JDK-8305420 in CDSJDITest.java
On Sun, 2 Apr 2023 22:11:40 GMT, Ioi Lam wrote: > Please review this trivial work-around that removes logging that would > trigger [JDK-8305420](https://bugs.openjdk.org/browse/JDK-8305420). > > I also fixed an incorrect parameter in the `executeAndLog()` call, which > caused the dumping process's stdout/stderr to be logged in the wrong file. This pull request has now been integrated. Changeset: 9ce5fdc9 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/9ce5fdc96262ac80c5a2ac2d51a149408d3d727a Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod 8305421: Work around JDK-8305420 in CDSJDITest.java Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/13283
Re: RFR: 8305421: Work around JDK-8305420 in CDSJDITest.java
On Mon, 3 Apr 2023 18:59:03 GMT, Chris Plummer wrote: >> Please review this trivial work-around that removes logging that would >> trigger [JDK-8305420](https://bugs.openjdk.org/browse/JDK-8305420). >> >> I also fixed an incorrect parameter in the `executeAndLog()` call, which >> caused the dumping process's stdout/stderr to be logged in the wrong file. > > Change looks good. Per https://bugs.openjdk.org/browse/JDK-8173304, this is a > known issue and tests need to avoid having the JVM produce too much output > before the debugger connection is setup. Thanks @plummercj for the review. - PR Comment: https://git.openjdk.org/jdk/pull/13283#issuecomment-1495013582
RFR: 8305421: Work around JDK-8305420 in CDSJDITest.java
Please review this trivial work-around that removes logging that would trigger [JDK-8305420](https://bugs.openjdk.org/browse/JDK-8305420). I also fixed an incorrect parameter in the `executeAndLog()` call, which caused the dumping process's stdout/stderr to be logged in the wrong file. - Commit messages: - 8305421: Work around JDK-8305420 in CDSJDITest.java Changes: https://git.openjdk.org/jdk/pull/13283/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13283=00 Issue: https://bugs.openjdk.org/browse/JDK-8305421 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13283.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13283/head:pull/13283 PR: https://git.openjdk.org/jdk/pull/13283
Re: RFR: 8228604: StackMapFrames are missing from redefined class bytes of retransformed classes
On Fri, 27 Jan 2023 02:13:28 GMT, David Holmes wrote: > > I'd expect that at least java.util.Date and java.lang.ProcessBuilder have > > the same verification requirement. > > Generally speaking yes - they are both loaded by bootstrap loader and so > would both have verification disabled by default. Bt as you note the > behaviour can change when CDS is involved and only one class gets dumped. > > > But in Date has StackMapTable (starting from JDK12-b15), and ProcessBuilder > > doesn't has StackMapTable. > > This seems odd, but not, IMO, in itself a bug. Perhaps @iklam can comment on > why we treat things differently during dumping. We always enable verification for all classes during -Xshare:dump. That avoid problems where the classes were dumped without verification but at run time you enable verification. - PR: https://git.openjdk.org/jdk/pull/12155
Re: RFC: regarding metaspace(metadata?) dump
On 1/11/2023 6:52 PM, Yi Yang wrote:
Hi Ioi,
> I think there are overlaps between your proposal and existing tools. For
example, there are jcmd options such as VM.class_hierarchy and
VM.classes, etc.
> The Serviceability Agent can also be used to analyze the contents of
the class metadata.
Of course, we can continue to add jcmd commands such as jcmd
VM.method_counter and jcmd VM.aggregtate_by_class_package to help
diagnosing, but another once and for all solution is to implement a
rich and well-formed metadata dump as this proposal described,
third-party parsers and platforms are eligible to analyze well-formed
dump file and provide many grouping/filtering
options(grouping_by_package, filter_linked, filter_force_inline,
essentially VM.class_hierarchy is aggregation of VM.classes).
I'm trying to describe a real use case to illustrate benefits of
well-formed metaspace dump: In our internal DevOps platform, I
observed that the Metaspace utilization rate of my application has
been high. During this period, FGC occurred several times. So I
generate a well-formed metaspace dump through DevOps platform, and
then the dump file will be automatically generated and uploaded to
another internal Java troubleshooting platform, troubleshooting
platform further analyzes and show it with many grouping and filter
options and so on.
> I'd be interested in seeing your implementation and compare it with
the existing tools.
I'm starting to do this, and it may take several months to implement
since it looks more like a JEP level feature, I want to hear some
general discussion before coding, i.e, is it acceptable to use JSON
format? should it be Metadata Dump or keeping the current metaspace
scope? Do you think basic+extend output for internal structure is
acceptable?
Before discussing the output of this tool, I think it's better to first
discuss the goals and intended use
- For Java app developers, I am not sure if they care about the
representation of the classes inside HotSpot. They may want to know what
classes are loaded in what class loaders, or want to trouble shoot
memory leaks (why aren't my classes unloaded, etc). For these, we
already have existing tools.
- For HotSpot developers, it would be nice to have a dump of all the
metadata, but I am not sure how important this is, as people seem to be
able to get by with their own debugging methods.
By the way, there may be multiple ways of creating such a dump. The
least intrusive way would be to program the Serviceability Agent, which
already has a lot of Java APIs to access HotSpot internals. That way,
you can write the dumper without modifying the HotSpot C++ code. It
could even be maintained as a project outside of the JDK repo.
Also you mentioned that "Internally we implemented a metaspace dump that
generates human-readable text". Can you share how this tool was implemented?
Thanks
- Ioi
> This may be quite difficult, because the metadata contains rewritten
Java bytecodes. The rewriting format may be dependent on the JDK
version. Also, the class linkage (the resolution of constant pool
information) will be vastly from one JDK version to another. So using
writing a third party tool that can work with multiple JDK versions
will be quite hard.
Thanks for your input! Maybe display rewrited bytecodes? Anyway, I'll
take a close look at this, and I'll prepare a POC along with dump
parser and a simple UI diagnose web once ready.
> Also, defining a "portable" format for the dump will be difficult,
since we don't know how the internal data structure will evolve in the
future.
Yes, since we don't know how internal data structure will changed in
the future, so I propose reaching a consensus that we can at least
reconstruct Java (rewrited?) source code as much as possible. For
example, the dumped JSON object for InstanceKlass contains two parts,
the first part contains the necessary information to reconstruct the
source code as much as possible, and the second part is extended
information, like this:
{
name:..,
super:..,
flags:...,
method:[]
interface:[]
fields:[],
annotation:[]
bytecode:[],
constantpool:[],
//extend
init_state:...,
init_thread:...,
}
The first part is basically unchanged(or adding new fields only), and
the extended part is subject to change, visualization dump client
checks if fields of JSON objects are defined and displays them further.
--
From:Ioi Lam
Send Time:2023 Jan. 12 (Thu.) 08:15
To:hotspot-runtime-dev ;
serviceability-...@openjdk.java.net
Subject:Re: RFC: regarding metaspace(metadata?) dump
CC-ing serviceability.
Hi Yi,
In general, I think it's good to have tools for understanding the
internal layout of the class metadata layouts.
I think there are overlaps between your proposal and existing
tools. For example, there are jcmd options such as
Re: RFC: regarding metaspace(metadata?) dump
CC-ing serviceability. Hi Yi, In general, I think it's good to have tools for understanding the internal layout of the class metadata layouts. I think there are overlaps between your proposal and existing tools. For example, there are jcmd options such as VM.class_hierarchy and VM.classes, etc. The Serviceability Agent can also be used to analyze the contents of the class metadata. Dd you look at the existing tools and see how they match up with your requirements? I'd be interested in seeing your implementation and compare it with the existing tools. On 1/11/2023 4:56 AM, Yi Yang wrote: Hi, Internally, we often receive feedback from users and ask for help on metaspace-related issues, for example 1. Users are eager to know which GroovyClassLoader loads which classes, why they are not unloaded, and why they are leading to Metaspace OOME. 2. They want to know the class structure of dynamically generated classes in some scenarios such as deserialization 3. Finding memory leaking about duplicated classes ... Internally we implemented a metaspace dump that generates human-readable text, it looks something like this: [Basic Information] Dump Reason : JCMD MaxMetaspaceSize : 18446744073709547520 B CompressedClassSpaceSize : 1073741824 B Class Space Used : 309992 B Class Space Capacity : 395264 B ... [Class Loader Data] ClassLoaderData : loader = 0x8024f928, loader_klass = 0x000800010098, loader_klass_name = sun/misc/Launcher$AppClassLoader, label = N/A Class Used Chunks : * Chunk : [0x00080006, 0x000800060230, 0x000800060800) NonClass Used Chunks : * Chunk : [0x7fd8379c1000, 0x7fd8379c1350, 0x7fd8379c2000) Klasses : Klass : 0x000800060028, name = Test, size = 520 B ConstantPool : 0x7fd8379c1050, size = 296 B ... It has been working effectively for several years and has helped many users solve metaspace-related problems. But a more user-friendly way is that JDK can inherently support this capability. We hope that format of the metaspace dump file can take both flexibility and compatibility into account, and the content of dump file should be detailed enough to meet the needs of both application developers and lower-level developers. Based on above considerations, I think using JSON as its file format is an appropriate solution(But XML or binary format are still not excluded as candidates). Specifically, in earlier thoughts, I thought the format of the metaspace file could be as follows(pretty printed) https://gist.github.com/y1yang0/ab3034b6381b8a9d215602c89af4e9c3 Using the JSON format, we can flexibly add new fields without breaking compatibility. It is debatable as to which data to write. We can reach a consensus that third-party parsers(Metaspace Analyzer Tool) can at least reconstruct Java source code from the dump file. This may be quite difficult, because the metadata contains rewritten Java bytecodes. The rewriting format may be dependent on the JDK version. Also, the class linkage (the resolution of constant pool information) will be vastly from one JDK version to another. So using writing a third party tool that can work with multiple JDK versions will be quite hard. Also, defining a "portable" format for the dump will be difficult, since we don't know how the internal data structure will evolve in the future. Thanks - Ioi Based on this, we can write more useful information for low-level troubleshooting or debugging. (e.g. the init_state of InstanceKlass). In addition, we can even output the native code and associated information with regard to Method, third-party parser can reconstruct the human-readable assembly representation of the compiled method based on dump file. To some extent, we have implemented code cache dump by the way. For this reason, I'm not sure if the title of the RFC proposal should be called metaspace dump, maybe metadata dump? It looks more like a metadata-dump framework. Do you have any thoughts about metaspace/metadata dump? Looking forward to hearing your feedback, any comments are invaluable! Best regards, Yi Yang
Re: RFR: 8286185: The Java manpage can be more platform inclusive [v3]
On Thu, 24 Nov 2022 00:50:22 GMT, David Holmes wrote: >> This is mainly an expansion of the included platforms by changing "linux and >> macOS" to "Non-Windows". There are a few additional examples, and >> clarification that they are just examples. There are also some minor edits >> and corrections I spotted. >> >> One actual fix relates to the "control-break" -> "control-" change. I can >> factor that out if needed (or just add an additional issue to the PR). >> >> This doesn't attempt to give complete platform recognition for all OpenJDK >> platforms. Two areas where anyone interested could file a further RFE is the >> support of DTrace on BSD systems other than macOS; and the use of RTM >> locking on Power8 architecture (existing documentation is all about Intel >> TSX on x86). >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fix formatting LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/11340
Re: RFR: 8295253: Remove kludge from v1_0/PerfDataBuffer.java
On Wed, 23 Nov 2022 19:42:47 GMT, Serguei Spitsyn wrote: >> Remove code for supporting an ancient JDK version (1.4.2). We don't have any >> test cases for this code so it's unclear whether it still works or not. >> >> Testing: tiers 1-4. > > Looks good and reasonable. > Thanks, > Serguei Thanks @sspitsyn @plummercj @dholmes-ora for the review. Passed tiers1-4. - PR: https://git.openjdk.org/jdk/pull/11329
Integrated: 8295253: Remove kludge from v1_0/PerfDataBuffer.java
On Wed, 23 Nov 2022 18:25:59 GMT, Ioi Lam wrote: > Remove code for supporting an ancient JDK version (1.4.2). We don't have any > test cases for this code so it's unclear whether it still works or not. > > Testing: tiers 1-4. This pull request has now been integrated. Changeset: 85ddd8f2 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/85ddd8f2af51fa5ea7f63027285509afb9a5c439 Stats: 149 lines in 1 file changed: 0 ins; 148 del; 1 mod 8295253: Remove kludge from v1_0/PerfDataBuffer.java Reviewed-by: sspitsyn, dholmes, cjplummer - PR: https://git.openjdk.org/jdk/pull/11329
RFR: 8295253: Remove kludge from v1_0/PerfDataBuffer.java
Remove code for supporting an ancient JDK version (1.4.2). We don't have any test cases for this code so it's unclear whether it still works or not. Testing: tiers 1-4. - Commit messages: - 8295253: Remove kludge from v1_0/PerfDataBuffer.java Changes: https://git.openjdk.org/jdk/pull/11329/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11329=00 Issue: https://bugs.openjdk.org/browse/JDK-8295253 Stats: 149 lines in 1 file changed: 0 ins; 148 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11329.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11329/head:pull/11329 PR: https://git.openjdk.org/jdk/pull/11329
Re: RFR: 8295315: [REDO] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
On Fri, 11 Nov 2022 06:13:50 GMT, David Holmes wrote: >> Here's redo for https://github.com/openjdk/jdk/pull/10687. This PR has two >> commits: >> - 739b79afb1965b625b2002187ac3fd43f385a639 is the same as in the original PR >> - 78455c024ec5c00f1a0ce6c0e13df477c3063fe1 fixes the bug in the original PR. >> >> I have run tiers 1 - 4 so hopefully I got it right this time :-) > > Looks good! > > Thanks. Thanks @dholmes-ora @kevinjwalls @sspitsyn for the review. - PR: https://git.openjdk.org/jdk/pull/11094
Integrated: 8295315: [REDO] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
On Thu, 10 Nov 2022 22:42:10 GMT, Ioi Lam wrote: > Here's redo for https://github.com/openjdk/jdk/pull/10687. This PR has two > commits: > - 739b79afb1965b625b2002187ac3fd43f385a639 is the same as in the original PR > - 78455c024ec5c00f1a0ce6c0e13df477c3063fe1 fixes the bug in the original PR. > > I have run tiers 1 - 4 so hopefully I got it right this time :-) This pull request has now been integrated. Changeset: 8c26d029 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/8c26d029b58943a473de1ecb7e33d51ebc9dbdf3 Stats: 81 lines in 3 files changed: 1 ins; 73 del; 7 mod 8295315: [REDO] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files Reviewed-by: dholmes, kevinw, sspitsyn - PR: https://git.openjdk.org/jdk/pull/11094
RFR: 8295315: [REDO] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
Here's redo for https://github.com/openjdk/jdk/pull/10687. This PR has two commits: - 739b79afb1965b625b2002187ac3fd43f385a639 is the same as in the original PR - 78455c024ec5c00f1a0ce6c0e13df477c3063fe1 fixes the bug in the original PR. I have run tiers 1 - 4 so hopefully I got it right this time :-) - Commit messages: - fixed bug in original JDK-8276687 patch - Original patch from JDK-8276687 Changes: https://git.openjdk.org/jdk/pull/11094/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11094=00 Issue: https://bugs.openjdk.org/browse/JDK-8295315 Stats: 81 lines in 3 files changed: 1 ins; 73 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/11094.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11094/head:pull/11094 PR: https://git.openjdk.org/jdk/pull/11094
Re: RFR: 8295320: [BACKOUT] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
On Fri, 14 Oct 2022 09:53:23 GMT, Kevin Walls wrote:
> Hi, are you sure you want to back this out? I'll click approve as backout
> change I think looks OK.
>
> I see test/hotspot/jtreg/serviceability/dcmd/framework/VMVersionTest.java
> failing SOMETIMES for me. I think you already noted that
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataBuffer.java
> no longer handles IllegalArgumentException
>
> If we handle that then for me the test that failed frequently, runs OK 20
> times in a row.
>
> ```
> bash-4.2$ git diff
> diff --git
> a/src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataBuffer.java
>
> b/src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataBuffer.java
> index 3ad35e350e4..056671d7841 100644
> ---
> a/src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataBuffer.java
> +++
> b/src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataBuffer.java
> @@ -63,7 +63,7 @@ public class PerfDataBuffer extends AbstractPerfDataBuffer {
> try {
> ByteBuffer bb = perf.attach(vmid.getLocalVmId());
> createPerfDataBuffer(bb, vmid.getLocalVmId());
> -} catch (IOException e) {
> +} catch (IOException | IllegalArgumentException e) {
> throw new MonitorException("Could not attach to "
> + vmid.getLocalVmId(), e);
> }
> ```
I want to do more testing and then make a REDO PR for the 1.4.1 code removal. I
also want to understand why these tests would sometimes get an
IllegalArgumentException.
-
PR: https://git.openjdk.org/jdk/pull/10711
Integrated: 8295320: [BACKOUT] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
On Fri, 14 Oct 2022 06:20:28 GMT, Ioi Lam wrote: > This PR reverts JDK-8276687, which has caused tier1 test failures. This pull request has now been integrated. Changeset: dfcd9d53 Author: Ioi Lam URL: https://git.openjdk.org/jdk/commit/dfcd9d538eba4b097083abe19d02d6d019236ac7 Stats: 80 lines in 3 files changed: 73 ins; 1 del; 6 mod 8295320: [BACKOUT] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files Reviewed-by: redestad, kevinw - PR: https://git.openjdk.org/jdk/pull/10711
RFR: 8295320: [BACKOUT] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
This PR reverts JDK-8276687, which has caused tier1 test failures. - Commit messages: - 8295320: [BACKOUT] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files Changes: https://git.openjdk.org/jdk/pull/10711/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10711=00 Issue: https://bugs.openjdk.org/browse/JDK-8295320 Stats: 80 lines in 3 files changed: 73 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10711.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10711/head:pull/10711 PR: https://git.openjdk.org/jdk/pull/10711
Integrated: 8276687: Remove support for JDK 1.4.1 PerfData shared memory files
On Thu, 13 Oct 2022 04:06:27 GMT, Ioi Lam wrote: > We have code in jdk.internal.jvmstat for supporting an ancient version (JDK > 1.4.1). There's currently no test case for this code, so it's likely to be > further bit-rotten in the future. Let's remove it now. > > If anyone wants to connect to JDK 1.4.1, they can use tools from JDK 19 or > earlier. This pull request has now been integrated. Changeset: 67046ae4 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/67046ae49a2611644854ed94c1932d518e47854b Stats: 80 lines in 3 files changed: 1 ins; 73 del; 6 mod 8276687: Remove support for JDK 1.4.1 PerfData shared memory files Reviewed-by: dholmes, kevinw, redestad - PR: https://git.openjdk.org/jdk/pull/10687
Re: RFR: 8276687: Remove support for JDK 1.4.1 PerfData shared memory files [v2]
On Thu, 13 Oct 2022 20:42:34 GMT, Kevin Walls wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> made exception message in PerfDataFile.getLocalVmId() more meaningful > > Yes, that's good. OK so there is evidence that v1_0 perfdata was at least > used for some 1.4.2, so yes no rush to remove the v1_0 classes, thanks. Thanks @kevinjwalls @cl4es @dholmes-ora for the review. - PR: https://git.openjdk.org/jdk/pull/10687
Re: RFR: 8276687: Remove support for JDK 1.4.1 PerfData shared memory files [v2]
On Thu, 13 Oct 2022 09:56:42 GMT, Kevin Walls wrote:
>> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataFile.java
>> line 94:
>>
>>> 92: } catch (NumberFormatException e) { }
>>> 93:
>>> 94: throw new IllegalArgumentException("file name does not match
>>> pattern");
>>
>> Pre-existing but it would be nice if this actually reported the file and the
>> pattern.
>
> Maybe "cannot convert 'filename' to VM id". (Or to pid.) We just parse it
> as an Integer now, so showing the problem filename would be nice. (Not sure
> if we would get this far trying a 1.4.1 attach, but if we did, it would help
> diagnose that.)
David and Kevin, thanks for the suggestions. I've update the error message.
Does it look OK now?
-
PR: https://git.openjdk.org/jdk/pull/10687
Re: RFR: 8276687: Remove support for JDK 1.4.1 PerfData shared memory files
On Thu, 13 Oct 2022 10:49:09 GMT, Kevin Walls wrote: > Maybe the 3 classes in > src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/v1_0/ are > redundant...? We parse a version number in > AbstractPerfDataBuffer.createPerfDataBuffer() and create e.g. > sun.jvmstat.perfdata.monitor.v2_0.PerfDataBuffer I don't have evidence of > exactly where version 2 starts (I know it was in jdk5, but maybe it was > 1.4.2), so I'm not saying the removal should be done now. Anyway, looks good. I have filed https://bugs.openjdk.org/browse/JDK-8295253 to remove the "kludge" code for supporting 1.4.1 in the monitor/v1_0/ code. Because of this kludge code, I suspect that v1_0 actually supports versions other than 1.4.1, so I am hesitant of removing the entire v1_0 directory. More investigation is needed. - PR: https://git.openjdk.org/jdk/pull/10687
Re: RFR: 8276687: Remove support for JDK 1.4.1 PerfData shared memory files [v2]
> We have code in jdk.internal.jvmstat for supporting an ancient version (JDK > 1.4.1). There's currently no test case for this code, so it's likely to be > further bit-rotten in the future. Let's remove it now. > > If anyone wants to connect to JDK 1.4.1, they can use tools from JDK 19 or > earlier. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: made exception message in PerfDataFile.getLocalVmId() more meaningful - Changes: - all: https://git.openjdk.org/jdk/pull/10687/files - new: https://git.openjdk.org/jdk/pull/10687/files/2524f87d..8e7b95fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10687=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10687=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10687.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10687/head:pull/10687 PR: https://git.openjdk.org/jdk/pull/10687
RFR: 8276687: Remove support for JDK 1.4.1 PerfData shared memory files
We have code in jdk.internal.jvmstat for supporting an ancient version (JDK 1.4.1). There's currently no test case for this code, so it's likely to be further bit-rotten in the future. Let's remove it now. If anyone wants to connect to JDK 1.4.1, they can use tools from JDK 19 or earlier. - Commit messages: - 8276687: Remove support for JDK 1.4.1 PerfData shared memory files Changes: https://git.openjdk.org/jdk/pull/10687/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10687=00 Issue: https://bugs.openjdk.org/browse/JDK-8276687 Stats: 79 lines in 3 files changed: 1 ins; 73 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/10687.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10687/head:pull/10687 PR: https://git.openjdk.org/jdk/pull/10687
Integrated: 8294740: Add cgroups keyword to TestDockerBasic.java
On Tue, 4 Oct 2022 01:03:25 GMT, Ioi Lam wrote: > Please review this trivial fix. TestDockerBasic.java uses cgroups features > so it should be tagged with `@key cgroups`. This pull request has now been integrated. Changeset: ae79af2a Author: Ioi Lam URL: https://git.openjdk.org/jdk/commit/ae79af2ad67b51a7608f4c9060421dd175cabf3f Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8294740: Add cgroups keyword to TestDockerBasic.java Reviewed-by: mseledtsov, dholmes - PR: https://git.openjdk.org/jdk/pull/10547
Re: RFR: 8294740: Add cgroups keyword to TestDockerBasic.java
On Tue, 4 Oct 2022 01:10:41 GMT, David Holmes wrote: >> Please review this trivial fix. TestDockerBasic.java uses cgroups features >> so it should be tagged with `@key cgroups`. > > Looks good and trivial. Thanks for the quick fix. Thanks @dholmes-ora and @mseledts for the review. - PR: https://git.openjdk.org/jdk/pull/10547
RFR: 8294740: Add cgroups keyword to TestDockerBasic.java
Please review this trivial fix. TestDockerBasic.java uses cgroups features so it should be tagged with `@key cgroups`. - Commit messages: - 8294740: Add cgroups keyword to TestDockerBasic.java Changes: https://git.openjdk.org/jdk/pull/10547/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10547=00 Issue: https://bugs.openjdk.org/browse/JDK-8294740 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10547.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10547/head:pull/10547 PR: https://git.openjdk.org/jdk/pull/10547
Re: RFR: 8293540: [Metrics] Incorrectly detected resource limits with additional cgroup fs mounts [v3]
On Thu, 15 Sep 2022 08:55:41 GMT, Severin Gehwolf wrote:
>> Similar issue to the hotspot change discussed in
>> https://bugs.openjdk.org/browse/JDK-8293472. The Java metrics implementation
>> may get the resource limits wrong if there are additional cgroup fs mounts.
>> Apparently that's more common than one might think. I've reproduced this
>> with these existing tests on cg v2:
>>
>>
>> test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java
>> test/jdk/jdk/internal/platform/docker/TestDockerCpuMetrics.java
>> test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetrics.java
>>
>>
>> I've also added `test/jdk/jdk/internal/platform/docker/TestDockerBasic.java`
>> and amended
>> `test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java`
>> which unconditionally fails (irrespective of cgroup version in use). The fix
>> is fairly straight forward and is an extension which we already do for the
>> `cpuset` controller: Allow duplicates, and if there are any prefer those
>> mounted at `/sys/fs/cgroup`.
>>
>> Testing:
>> - [x] fastdebug build on cgroups v2 and cgroups v1 (before and after the
>> product fix)
>> - [x] added tests fail before, pass after the product fix.
>> - [x] Some manual testing using `cgcreate` and `cgexec` on cg1 and cg2.
>> Still pass.
>> - [x] GHA all pass.
>>
>> Please review! Many thanks in advance.
>
> Severin Gehwolf has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains one commit:
>
> 8293540: [Metrics] Potentially incorrectly detected resource limits with
> additional cgroup fs mounts
The JDK change looks good to me. Some nits for the test cases.
test/jdk/jdk/internal/platform/docker/TestDockerBasic.java line 26:
> 24: /*
> 25: * @test
> 26: * @summary Verify that -XshowSettings:system works
Add `@bug 8293540`
test/jdk/jdk/internal/platform/docker/TestDockerBasic.java line 64:
> 62: opts.addDockerOpts("--memory", "300m");
> 63: if (addCgroupMounts) {
> 64: opts.addDockerOpts("--volume",
> "/sys/fs/cgroup:/cgroup-in:ro");
Add comments that the extra mount should be ignored by the cgroup set-up code.
(Same for other test cases).
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10248
Re: RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v7]
On Wed, 31 Aug 2022 12:39:08 GMT, Coleen Phillimore wrote: >> Please review this simple conversion for the ProtectionDomainCacheTable from >> Old Hashtable to ResourceHashtable. There are specific tests for this table >> in test/hotspot/jtreg/runtime/Dictionary and >> serviceability/dcmd/vm/DictionaryStatsTest.java. >> Also tested with tier1-7. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comments, add assert. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10043
Re: RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v7]
On Thu, 1 Sep 2022 06:51:21 GMT, David Holmes wrote: >> “Mapping to itself” is how this table works internally. I am not sure if >> this information is useful here. But the purpose of this table is really to >> keep track of all PDs that have been registered and not yet garbage >> collected. > > "mapping to itself" is more useful than "mapping ... to a unique Weakhandle" > - which is even more of an internal implementation detail. I found the use of > this table very hard to discern based on the internal use of the hashtable, > as there is no real mapping operation - we simply track if a PD has been seen > or not. The use of the hashtable is purely for lookup convenience - we could > instead have a linked-list of PD's that we traverse for lookup. > So perhaps we drop my second sentence above, and move it to where the > hashtable itself is declared? "mapping a PD to a unique Weakhandle" is not an implementation detail. It's the only useful API provided by this class: WeakHandle obj = ProtectionDomainCacheTable::add_if_absent(protection_domain); and that's the reason I question why this table is needed at all. - PR: https://git.openjdk.org/jdk/pull/10043
Re: RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v7]
On Wed, 31 Aug 2022 12:39:08 GMT, Coleen Phillimore wrote:
>> Please review this simple conversion for the ProtectionDomainCacheTable from
>> Old Hashtable to ResourceHashtable. There are specific tests for this table
>> in test/hotspot/jtreg/runtime/Dictionary and
>> serviceability/dcmd/vm/DictionaryStatsTest.java.
>> Also tested with tier1-7.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix comments, add assert.
I am utterly confused about why we need ProtectionDomainCacheTable at all. The
only interface between this class and the rest of the world is:
DictionaryEntry::add_protection_domain() {
WeakHandle obj =
ProtectionDomainCacheTable::add_if_absent(protection_domain);
// Additions and deletions hold the SystemDictionary_lock, readers are
lock-free
ProtectionDomainEntry* new_head = new ProtectionDomainEntry(obj, _pd_set);
}
(and there's code elsewhere for cleaning up this table, but that wouldn't be
necessary if no one calls `add_if_absent`!).
Why doesn't DictionaryEntry::add_protection_domain allocate the WeakHandle
itself?
I am looking at the JDK 8 code. It seems like ProtectionDomainCacheTable was
needed before we had WeakHandle, so we had to do all the reference management
by hand:
https://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/classfile/dictionary.hpp#l137
But for today, is ProtectionDomainCacheTable a relic that can be thrown away?
-
PR: https://git.openjdk.org/jdk/pull/10043
Re: RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v7]
On Thu, 1 Sep 2022 04:12:32 GMT, David Holmes wrote: >> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix comments, add assert. > > src/hotspot/share/classfile/protectionDomainCache.hpp line 33: > >> 31: >> 32: // The ProtectionDomainCacheTable maps all >> java.security.ProtectionDomain objects that are >> 33: // registered by DictionaryEntry::add_protection_domain() to a unique >> WeakHandle. > > Now that I understand what this table does, this comment is confusing to me. > The table maps each PD to itself (using the WeakHandle as the actual key and > value) as a means to track which PDs have been seen/registered. I would > describe it thus: > > // The ProtectionDomainCacheTable records all of the > java.security.ProtectionDomain instances that have > // been registered by DictionaryEntry::add_protection_domain(). We use a > hashtable to > map each PD > // instance to itself (using WeakHandles) to allow for fast lookup. “Mapping to itself” is how this table works internally. I am not sure if this information is useful here. But the purpose of this table is really to keep track of all PDs that have been registered and not yet garbage collected. - PR: https://git.openjdk.org/jdk/pull/10043
Re: RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v4]
On Mon, 29 Aug 2022 12:24:33 GMT, Coleen Phillimore wrote:
>> Please review this simple conversion for the ProtectionDomainCacheTable from
>> Old Hashtable to ResourceHashtable. There are specific tests for this table
>> in test/hotspot/jtreg/runtime/Dictionary and
>> serviceability/dcmd/vm/DictionaryStatsTest.java.
>> Also tested with tier1-7.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> David's comments
Looks good overall and seems to be equivalent to the old code. Just a couple of
nits.
src/hotspot/share/classfile/protectionDomainCache.cpp line 162:
> 160: // Purge any deleted entries outside of the SystemDictionary_lock.
> 161: purge_deleted_entries();
> 162: int oops_removed = purge_entries_from_table(); // reacquires SD lock
It's confusing to have two similarly named functions (purge_deleted_entries and
purge_entries_from_table). Maybe the two functions should be combined into a
single `purge()` function that performs the two steps?
src/hotspot/share/classfile/protectionDomainCache.hpp line 35:
> 33: // The amount of different protection domains used is typically
> magnitudes smaller
> 34: // than the number of system dictionary entries (loaded classes).
> 35: class ProtectionDomainCacheTable : public AllStatic {
How about adding a comment to say what this table maps from/to? Something like:
// The ProtectionDomainCacheTable maps all java.security.ProtectionDomain
objects that are
// registered by DictionaryEntry::add_protection_domain() to a unique
WeakHandle.
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10043
Re: RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v3]
On Mon, 29 Aug 2022 12:10:48 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/classfile/protectionDomainCache.cpp line 43:
>>
>>> 41:
>>> 42: unsigned int ProtectionDomainCacheTable::compute_hash(const WeakHandle&
>>> protection_domain) {
>>> 43: return (unsigned int)(protection_domain.resolve()->identity_hash());
>>
>> And if it is a `WeakHandle` can't `resolve` now return NULL?
>
> compute_hash() is always called on a live WeakHandle so will never return
> null.
I think you should add an assert that the `protection_domain.resolve()` never
returns null, with a comment explaining why.
-
PR: https://git.openjdk.org/jdk/pull/10043
Re: RFR: 8282684: Obsolete UseContainerCpuShares and PreferContainerQuotaForCPUCount flags
On Fri, 19 Aug 2022 18:41:12 GMT, Harold Seigel wrote: > Please review this fix to obsolete two container JVM flags related to using > CPU shares to compute active processor count within containers. This fix > obsoletes the flags and removes the use of CPU shares from the calculations. > The fix was tested by running the container tests on Linux x64 and Linux > aarch64 using Mach5 > > Thanks, Harold LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/9948
Integrated: 8292267: Clean up synchronizer.hpp
On Fri, 12 Aug 2022 00:42:52 GMT, Ioi Lam wrote: > Fix two problems with synchronizer.hpp: > > - It's unnecessarily included in two popular headers: monitorChunk.hpp and > frame_.hpp. The latter is most problematic because it includes > synchronizer.hpp in the middle of a class declaration! > - synchronizer.hpp includes linkedlist.hpp. This can be avoid by moving the > definition of Synchronizer::PtrList into synchronizer.cpp. > > These headers are included much less after this PR (out of ~1000 hotspot .o > files) > > > linkedlist.hpp857 -> 101 > synchronizer.hpp 848 -> 122 > resourceHash.hpp 850 -> 270 This pull request has now been integrated. Changeset: 7244dd6f Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/7244dd6fab0c516ed76af594593b8378512620c8 Stats: 54 lines in 21 files changed: 28 ins; 21 del; 5 mod 8292267: Clean up synchronizer.hpp Reviewed-by: coleenp, dcubed, dholmes - PR: https://git.openjdk.org/jdk/pull/9849
Re: RFR: 8292267: Clean up synchronizer.hpp [v3]
On Fri, 12 Aug 2022 13:20:32 GMT, Coleen Phillimore wrote: >> Ioi Lam has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into 8292267-clean-up-synchronizer-hpp >> - @dcubed-ojdk review comments - reorder class forward declarations >> - added back inlined functions in case their performance matters >> - fixed copyright >> - 8292267: Clean up synchronizer.hpp > > Very nice. Thanks @coleenp @dholmes-ora @dcubed-ojdk for the review. Tiers 1,2 and build-tier5 passed on top of #9927 - PR: https://git.openjdk.org/jdk/pull/9849
Re: RFR: 8292267: Clean up synchronizer.hpp [v2]
> Fix two problems with synchronizer.hpp: > > - It's unnecessarily included in two popular headers: monitorChunk.hpp and > frame_.hpp. The latter is most problematic because it includes > synchronizer.hpp in the middle of a class declaration! > - synchronizer.hpp includes linkedlist.hpp. This can be avoid by moving the > definition of Synchronizer::PtrList into synchronizer.cpp. > > These headers are included much less after this PR (out of ~1000 hotspot .o > files) > > > linkedlist.hpp857 -> 101 > synchronizer.hpp 848 -> 122 > resourceHash.hpp 850 -> 270 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dcubed-ojdk review comments - reorder class forward declarations - Changes: - all: https://git.openjdk.org/jdk/pull/9849/files - new: https://git.openjdk.org/jdk/pull/9849/files/9b372650..f2dfac39 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9849=01 - incr: https://webrevs.openjdk.org/?repo=jdk=9849=00-01 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/9849.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9849/head:pull/9849 PR: https://git.openjdk.org/jdk/pull/9849
RFR: 8292267: Clean up synchronizer.hpp
Fix two problems with synchronizer.hpp: - It's unnecessarily included in two popular headers: monitorChunk.hpp and frame_.hpp. The latter is most problematic because it includes synchronizer.hpp in the middle of a class declaration! - synchronizer.hpp includes linkedlist.hpp. This can be avoid by moving the definition of Synchronizer::PtrList into synchronizer.cpp. These headers are included much less after this PR (out of ~1000 hotspot .o files) linkedlist.hpp857 -> 101 synchronizer.hpp 848 -> 122 resourceHash.hpp 850 -> 270 - Commit messages: - added back inlined functions in case their performance matters - fixed copyright - 8292267: Clean up synchronizer.hpp Changes: https://git.openjdk.org/jdk/pull/9849/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9849=00 Issue: https://bugs.openjdk.org/browse/JDK-8292267 Stats: 54 lines in 21 files changed: 27 ins; 20 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/9849.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9849/head:pull/9849 PR: https://git.openjdk.org/jdk/pull/9849
Re: RFR: 8292006: Move thread accessor classes to threadJavaClasses.hpp
On Mon, 8 Aug 2022 01:26:11 GMT, David Holmes wrote: > My issue with these kinds of changes in general is that they: > > * undermine institutional memory > * make it harder to do backports due to the shuffling > * make it harder to track changes due to the shuffling > > So the benefit to me has to be more than just a small decrease in some build > times which most people are never going to notice anyway. Sorry. Just my 2c. While I understand the concerns, the above reason can be used against any sort of refactoring of the source code. We have a few decades of development, resulting slow but steady source code rot. We need to refactor the code when it makes sense to keep the repo healthy. Yes, the above are valid concerns, but we have been successful in dealing with them in past refactoring exercises. Build time is important for HotSpot developers. I figure that the header file refactoring that happened in the last year or so (by myself and others) have resulted in 20% or so reduction in build time. So of that was taken back, though, due to new features such as Loom. I tried to limit the amount of code to be moved. However, when a file like javaClasses.hpp grows to over 2000 lines or mostly unrelated declarations, it's a sign that it needs to be broken up (which I started doing since JDK-8288623). - PR: https://git.openjdk.org/jdk/pull/9788
RFR: 8292006: Move thread accessor classes to threadJavaClasses.hpp
To improve modularity and build time, move the declaration of the following accessor from classfile/javaClasses.hpp to runtime/threadJavaClasses.hpp: + java_lang_Thread_FieldHolder + java_lang_Thread_Constants + java_lang_ThreadGroup + java_lang_VirtualThread Also move javaThreadStatus.hpp from share/classfile to share/runtime, where it belongs. - Commit messages: - 8292006: Move thread accessor classes to threadJavaClasses.hpp Changes: https://git.openjdk.org/jdk/pull/9788/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9788=00 Issue: https://bugs.openjdk.org/browse/JDK-8292006 Stats: 1912 lines in 33 files changed: 1016 ins; 891 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/9788.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9788/head:pull/9788 PR: https://git.openjdk.org/jdk/pull/9788
Integrated: 8286030: Avoid JVM crash when containers share the same /tmp dir
On Thu, 7 Jul 2022 06:01:58 GMT, Ioi Lam wrote: > Some Kubernetes setups share the /tmp directory across multiple containers. > On rare occasions, the JVM may crash when it tries to write to > `/tmp/hsperfdata_/` when a process in a separate container decides > to do the same thing (because they happen to have the same namespaced pid). > > This patch avoids the crash by using `flock()` to allow only one of these > processes to write to the file. All other competing processes that fail to > grab the lock will give up the file and run with PerfMemory disabled. We will > try to enable PerfMemory for the failed processes in a follow-up RFE: > [JDK-8289883](https://bugs.openjdk.org/browse/JDK-8289883) > > Thanks to Vitaly Davidovich and Nico Williams for coming up with the idea of > using `flock()`. > > I kept the use of `kill()` for stale file detection to be compatible with > older JVMs. > > I also took the opportunity to clean up the comments and remove dead code. > The old code was using "shared memory resources" which sounds unclear and > odd. I changed the terminology to say "shared memory file" instead. This pull request has now been integrated. Changeset: 84f23149 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/84f23149e22561173feb0e34bca31a7345b43c89 Stats: 324 lines in 3 files changed: 267 ins; 27 del; 30 mod 8286030: Avoid JVM crash when containers share the same /tmp dir Reviewed-by: stuefe, sgehwolf - PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v6]
On Tue, 12 Jul 2022 18:56:37 GMT, Thomas Stuefe wrote: >> Ioi Lam has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - add errno to log >> - added debug log and tweaked comment > > Looks good! Thanks to @tstuefe and @jerboaa for the review. - PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v7]
> Some Kubernetes setups share the /tmp directory across multiple containers. > On rare occasions, the JVM may crash when it tries to write to > `/tmp/hsperfdata_/` when a process in a separate container decides > to do the same thing (because they happen to have the same namespaced pid). > > This patch avoids the crash by using `flock()` to allow only one of these > processes to write to the file. All other competing processes that fail to > grab the lock will give up the file and run with PerfMemory disabled. We will > try to enable PerfMemory for the failed processes in a follow-up RFE: > [JDK-8289883](https://bugs.openjdk.org/browse/JDK-8289883) > > Thanks to Vitaly Davidovich and Nico Williams for coming up with the idea of > using `flock()`. > > I kept the use of `kill()` for stale file detection to be compatible with > older JVMs. > > I also took the opportunity to clean up the comments and remove dead code. > The old code was using "shared memory resources" which sounds unclear and > odd. I changed the terminology to say "shared memory file" instead. Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into 8286030-avoid-jvm-crash-when-containers-share-tmp-dir - Fixed typo in comments - add errno to log - added debug log and tweaked comment - fixed typo - added more comments - Simplified code and improved comments - @tstuefe comments - Merge branch 'master' into 8286030-avoid-jvm-crash-when-containers-share-tmp-dir - Cleaned up comments and avoid using the word "resource" to describe the shared mem file - ... and 5 more: https://git.openjdk.org/jdk/compare/7575df9c...efc00f6e - Changes: - all: https://git.openjdk.org/jdk/pull/9406/files - new: https://git.openjdk.org/jdk/pull/9406/files/ecac352b..efc00f6e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9406=06 - incr: https://webrevs.openjdk.org/?repo=jdk=9406=05-06 Stats: 85601 lines in 1888 files changed: 43830 ins; 26943 del; 14828 mod Patch: https://git.openjdk.org/jdk/pull/9406.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9406/head:pull/9406 PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v6]
On Tue, 12 Jul 2022 22:39:36 GMT, Ioi Lam wrote: >> Some Kubernetes setups share the /tmp directory across multiple containers. >> On rare occasions, the JVM may crash when it tries to write to >> `/tmp/hsperfdata_/` when a process in a separate container >> decides to do the same thing (because they happen to have the same >> namespaced pid). >> >> This patch avoids the crash by using `flock()` to allow only one of these >> processes to write to the file. All other competing processes that fail to >> grab the lock will give up the file and run with PerfMemory disabled. We >> will try to enable PerfMemory for the failed processes in a follow-up RFE: >> [JDK-8289883](https://bugs.openjdk.org/browse/JDK-8289883) >> >> Thanks to Vitaly Davidovich and Nico Williams for coming up with the idea of >> using `flock()`. >> >> I kept the use of `kill()` for stale file detection to be compatible with >> older JVMs. >> >> I also took the opportunity to clean up the comments and remove dead code. >> The old code was using "shared memory resources" which sounds unclear and >> odd. I changed the terminology to say "shared memory file" instead. > > Ioi Lam has updated the pull request incrementally with two additional > commits since the last revision: > > - add errno to log > - added debug log and tweaked comment @jerboaa could you take a look? Thanks. - PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8290218: AIX build failure by JDK-8289780 [v2]
On Thu, 14 Jul 2022 00:42:55 GMT, Ichiroh Takiguchi wrote: >> AIX build was failed by following messages: >> >> * For target hotspot_variant-server_libjvm_objs_BUILD_LIBJVM_link: >> ld: 0711-317 ERROR: Undefined symbol: collector_func_load >> ld: 0711-317 ERROR: Undefined symbol: .collector_func_load >> ld: 0711-344 See the loadmap file >> /home/jdkbld/git/jdk/build/aix-ppc64-server-release/hotspot/variant-server/libjvm/objs/libjvm.loadmap >> for more information. >> >> In my investigation, >> [JDK-8289780](https://bugs.openjdk.org/browse/JDK-8289780) affects this >> issue on src/hotspot/share/prims/forte.cpp. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > 8290218: AIX build failure by JDK-8289780 LGTM and this can be considered as a trivial change that requires only one review. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/9482
Re: RFR: 8290218: AIX build failure by JDK-8289780
On Wed, 13 Jul 2022 16:55:38 GMT, Ichiroh Takiguchi wrote: > AIX build was failed by following messages: > > * For target hotspot_variant-server_libjvm_objs_BUILD_LIBJVM_link: > ld: 0711-317 ERROR: Undefined symbol: collector_func_load > ld: 0711-317 ERROR: Undefined symbol: .collector_func_load > ld: 0711-344 See the loadmap file > /home/jdkbld/git/jdk/build/aix-ppc64-server-release/hotspot/variant-server/libjvm/objs/libjvm.loadmap > for more information. > > In my investigation, > [JDK-8289780](https://bugs.openjdk.org/browse/JDK-8289780) affects this issue > on src/hotspot/share/prims/forte.cpp. Changes requested by iklam (Reviewer). src/hotspot/share/prims/forte.cpp line 670: > 668: // Because it is weakly bound, the calls become NOP's when the library > 669: // isn't present. > 670: #if defined(__APPLE__) I think this can be combined with the condition below: #if defined(__APPLE__) || defined(_AIX) I am curious why `collector_func_load` worked before but `collector_func_load_enabled()` doesn't work, since they are using the same pattern. Anyway, I think the new change is better, since the profiling tool that requires `collector_func_load` isn't available on AIX. - PR: https://git.openjdk.org/jdk/pull/9482
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v5]
On Mon, 11 Jul 2022 04:53:19 GMT, Thomas Stuefe wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed typo
>
> src/hotspot/os/posix/perfMemory_posix.cpp line 733:
>
>> 731: while ((entry = os::readdir(dirp)) != NULL) {
>> 732: const char* filename = entry->d_name;
>> 733: pid_t pid = filename_to_pid(filename);
>
> Pre-existing. An error value of -1 would be somewhat cleaner since strictly
> speaking pid 0 is a valid PID. (Feel free to ignore this comment)
I think I'll leave it for now to minimize the behavior changes of this PR. I
read somewhere that pid 0 is the scheduler
https://unix.stackexchange.com/questions/83322/which-process-has-pid-0
So no actual JVM process will create a stale file with name "0". If such a file
exists for some other reason we would remove it, which be consistent with the
new comment "any other files found in this directory may be removed".
> src/hotspot/os/posix/perfMemory_posix.cpp line 760:
>
>> 758: // Something wrong happened. Ignore the error and don't try to
>> remove the
>> 759: // file.
>> 760: errno = 0;
>
> Can we have a debug or trace log line here?
Fixed
> src/hotspot/os/posix/perfMemory_posix.cpp line 804:
>
>> 802: }
>> 803:
>> 804: #if defined(LINUX)
>
> Indentation
Fixed
> src/hotspot/os/posix/perfMemory_posix.cpp line 807:
>
>> 805: // Hold the lock until here to prevent other JVMs from using this
>> file
>> 806: // while we are in the middle of deleting it.
>> 807: ::close(fd);
>
> I don't understand this comment, it seems to contradict what you are doing.
> We are closing the only fd referring to this lock file, right? So the lock
> should get unlocked here too? If we want to keep the lock open, shouldn't we
> avoid closing the fd?
I want to prevent the following race condition. Let's assume this process has
PID 10 and there's another process (in a different pid namespace) with PID 20.
Both process see a file named "20".
0. No one holds a lock on this file.
1. Process 20 successfully locks the file in cleanup_sharedmem_files().
2. Process 20 gives up the lock.
3. Process 20 decides it can delete the file (PID 20 matches its own PID).
4. This process successfully locks the file in cleanup_sharedmem_files().
5. This process gives up the lock
6. This process decides it can delete the file (PID 20 does not exist in my pid
namespace)
7. Process 20 deletes the file. Creates a new version of this file.
Successfully locks the new file.
8. This process deletes the new version of this file (by name).
By holding the lock between steps 4 and 8, we can guaranteed that if a process
can successfully lock the file in create_sharedmem_file(), this file will never
be unintentionally deleted.
I changed the comment slightly:
// Hold the lock until here to prevent other JVMs from using this file
- // while we are in the middle of deleting it.
+ // while we were in the middle of deleting it.
> src/hotspot/os/posix/perfMemory_posix.cpp line 926:
>
>> 924: (errno == EWOULDBLOCK) ?
>> 925: "it is locked by another process" :
>> 926: "flock() failed");
>
> strerror would be helpful here, or at least errno
I added just the errno. Example:
[0.003s][warning][perf,memops] Cannot use file /tmp/hsperfdata_root/1
because it is locked by another process (errno = 11)
If we print the `os::strerror()` it would look like this:
... because it is locked by another process (errno = 11, Operation would block)
which seems too verbose and could be confusing.
-
PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v6]
> Some Kubernetes setups share the /tmp directory across multiple containers. > On rare occasions, the JVM may crash when it tries to write to > `/tmp/hsperfdata_/` when a process in a separate container decides > to do the same thing (because they happen to have the same namespaced pid). > > This patch avoids the crash by using `flock()` to allow only one of these > processes to write to the file. All other competing processes that fail to > grab the lock will give up the file and run with PerfMemory disabled. We will > try to enable PerfMemory for the failed processes in a follow-up RFE: > [JDK-8289883](https://bugs.openjdk.org/browse/JDK-8289883) > > Thanks to Vitaly Davidovich and Nico Williams for coming up with the idea of > using `flock()`. > > I kept the use of `kill()` for stale file detection to be compatible with > older JVMs. > > I also took the opportunity to clean up the comments and remove dead code. > The old code was using "shared memory resources" which sounds unclear and > odd. I changed the terminology to say "shared memory file" instead. Ioi Lam has updated the pull request incrementally with two additional commits since the last revision: - add errno to log - added debug log and tweaked comment - Changes: - all: https://git.openjdk.org/jdk/pull/9406/files - new: https://git.openjdk.org/jdk/pull/9406/files/78254917..ecac352b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9406=05 - incr: https://webrevs.openjdk.org/?repo=jdk=9406=04-05 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/9406.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9406/head:pull/9406 PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v2]
On Sat, 9 Jul 2022 05:19:08 GMT, Ioi Lam wrote:
>> src/hotspot/os/posix/perfMemory_posix.cpp line 796:
>>
>>> 794: if (!is_locked_by_another_process) {
>>> 795: if ((pid == os::current_process_id()) ||
>>> 796: (kill(pid, 0) == OS_ERR && (errno == ESRCH || errno ==
>>> EPERM))) {
>>
>> Thinking about this, I now was confused. AFAICS this code is only ever
>> called from `mmap_create_shared` with the directory containing our own pid.
>> I do not see this called for PIDs from other processes. Why do we handle
>> that case? Or am I overlooking something?
>
> The current JVM process only cleans up stale files in the current user's own
> directory. I.e., /tmp/hsperfdata_bob/. However, when we see a file like
> /tmp/hsperfdata_bob/1234, it's possible that
>
> - 1234 was a JVM process created by bob, but this process died without
> cleaning up
> - A new process 1234 is now running, but it belongs to a different user, john
>
> So if `kill(1234, 0)` fails, the JVM concludes that,
> "/tmp/hsperfdata_bob/1234 must belong to a terminated JVM process owned by
> bob".
>
> I think this is the intention of the code, in spite of what the comment says
> above the `kill` call.
I simplified the code and restructured the comments to be interspersed with the
code. Hopefully that would make the logic a little easier to understand.
Despite the large diff in cleanup_sharedmem_files(), the only actual change is
to `flock` in the middle of the loop, and avoid deleting the file if the
`flock` fails.
I removed this comment
// ... are removed because the resources for such a
// process should be in a different user specific directory.
and replaced it with something more comprehensive which hopefully makes more
sense:
// This directory should be used only by JVM processes owned by the
// current user to store PerfMemory files. Any other files found
// in this directory may be removed.
-
PR: https://git.openjdk.org/jdk/pull/9406
Re: RFR: 8286030: Avoid JVM crash when containers share the same /tmp dir [v5]
> Some Kubernetes setups share the /tmp directory across multiple containers. > On rare occasions, the JVM may crash when it tries to write to > `/tmp/hsperfdata_/` when a process in a separate container decides > to do the same thing (because they happen to have the same namespaced pid). > > This patch avoids the crash by using `flock()` to allow only one of these > processes to write to the file. All other competing processes that fail to > grab the lock will give up the file and run with PerfMemory disabled. We will > try to enable PerfMemory for the failed processes in a follow-up RFE: > [JDK-8289883](https://bugs.openjdk.org/browse/JDK-8289883) > > Thanks to Vitaly Davidovich and Nico Williams for coming up with the idea of > using `flock()`. > > I kept the use of `kill()` for stale file detection to be compatible with > older JVMs. > > I also took the opportunity to clean up the comments and remove dead code. > The old code was using "shared memory resources" which sounds unclear and > odd. I changed the terminology to say "shared memory file" instead. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: fixed typo - Changes: - all: https://git.openjdk.org/jdk/pull/9406/files - new: https://git.openjdk.org/jdk/pull/9406/files/73242937..78254917 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9406=04 - incr: https://webrevs.openjdk.org/?repo=jdk=9406=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9406.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9406/head:pull/9406 PR: https://git.openjdk.org/jdk/pull/9406
