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&pr=16868&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=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 Sat, 2 Dec 2023 00:38:58 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. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed indentation Thanks for addressing my comments. Approved! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1766049580
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:38:58 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. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed indentation Looks good. Did not find any functional difference to the original code. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1760796058
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:32:30 GMT, Ioi Lam wrote: >> 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. Thanks for the explanation. Looks good then. - PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412714559
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:38:58 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. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed indentation Marked as reviewed by ccheung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1760771168
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&pr=16868&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=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
Re: RFR: 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 looks good! I think this is a good opportunity to refactor some of the
code for better readability so I left some comments below.
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?
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.
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.
-
PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1760626242
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412613074
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412616593
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412610795
Re: RFR: 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. Code migration from arguments cpp to cdsConfig.cpp looks good. Found a minor simplification regarding the include statements. 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. - PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1756281866 PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1409910394
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&pr=16868&range=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
