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=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
