RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp

Ioi Lam-2
This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:

- Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp

- Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up

- Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

-------------

Commit messages:
 - 8261608: Move common CDS archive building code to archiveBuilder.cpp

Changes: https://git.openjdk.java.net/jdk/pull/2536/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261608
  Stats: 943 lines in 31 files changed: 327 ins; 464 del; 152 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2536.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2536/head:pull/2536

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp

Ioi Lam-2
On Thu, 11 Feb 2021 23:41:34 GMT, Ioi Lam <[hidden email]> wrote:

> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>
> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>
> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>
> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

Example of detailed stats that are now available for dynamic dump:

[info ][cds] Shared file region (mc )  0:       24 bytes, addr 0x0000000800c30000 file offset 0x00001000 crc 0xa3c1ca20
[info ][cds] Shared file region (rw )  1:     1016 bytes, addr 0x0000000800c31000 file offset 0x00002000 crc 0xb7970a35
[info ][cds] Shared file region (ro )  2:     1632 bytes, addr 0x0000000800c32000 file offset 0x00003000 crc 0x4edeacfa
[info ][cds] Shared file region (bm )  3:      160 bytes, addr 0x0000000000000000 file offset 0x00004000 crc 0x27cf167e
[debug][cds] mc  space:        24 [  0.1% of total] out of      4096 bytes [  0.6% used] at 0x0000000800c30000
[debug][cds] rw  space:      1016 [  6.2% of total] out of      4096 bytes [ 24.8% used] at 0x0000000800c31000
[debug][cds] ro  space:      1632 [ 10.0% of total] out of      4096 bytes [ 39.8% used] at 0x0000000800c32000
[debug][cds] bm  space:       160 [  1.0% of total] out of       160 bytes [100.0% used]
[debug][cds] total    :      2832 [100.0% of total] out of     16384 bytes [ 17.3% used]
[debug][cds] Detailed metadata info (excluding heap regions; rw stats include mc regions):
[debug][cds]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[debug][cds] --------------------+---------------------------+---------------------------+--------------------------
[debug][cds] Class               :        0          0   0.0 |        1        544  52.3 |        1        544  20.4
[debug][cds] Symbol              :        3         64   3.9 |        0          0   0.0 |        3         64   2.4
[debug][cds] TypeArrayU1         :        3        168  10.3 |        1         40   3.8 |        4        208   7.8
[debug][cds] TypeArrayU2         :        2         16   1.0 |        0          0   0.0 |        2         16   0.6
[debug][cds] TypeArrayU4         :        1         16   1.0 |        0          0   0.0 |        1         16   0.6
[debug][cds] TypeArrayU8         :        2        672  41.2 |        1         40   3.8 |        3        712  26.6
[debug][cds] TypeArrayOther      :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] Method              :        0          0   0.0 |        2        192  18.5 |        2        192   7.2
[debug][cds] ConstMethod         :        2        144   8.8 |        0          0   0.0 |        2        144   5.4
[debug][cds] MethodData          :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] ConstantPool        :        1        312  19.1 |        0          0   0.0 |        1        312  11.7
[debug][cds] ConstantPoolCache   :        0          0   0.0 |        1        136  13.1 |        1        136   5.1
[debug][cds] Annotations         :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] MethodCounters      :        0          0   0.0 |        1         64   6.2 |        1         64   2.4
[debug][cds] RecordComponent     :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] SymbolHashentry     :        3         32   2.0 |        0          0   0.0 |        3         32   1.2
[debug][cds] SymbolBucket        :        2         16   1.0 |        0          0   0.0 |        2         16   0.6
[debug][cds] StringHashentry     :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] StringBucket        :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] ModulesNatives      :        0          0   0.0 |        0          0   0.0 |        0          0   0.0
[debug][cds] Other               :        0        192  11.8 |        0         24   2.3 |        0        216   8.1
[debug][cds] --------------------+---------------------------+---------------------------+--------------------------
[debug][cds] Total               :       19       1632 100.0 |        7       1040 100.0 |       26       2672 100.0

-------------

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Thu, 11 Feb 2021 23:41:34 GMT, Ioi Lam <[hidden email]> wrote:

> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>
> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>
> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>
> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

Looks like a good change.

src/hotspot/share/classfile/moduleEntry.cpp line 33:

> 31: #include "logging/log.hpp"
> 32: #include "memory/archiveBuilder.hpp"
> 33: #include "memory/archiveUtils.hpp"

Do these files need archiveUtils.hpp or does archiveBuilder.hpp need it?

src/hotspot/share/memory/archiveBuilder.cpp line 178:

> 176:   _num_instance_klasses = 0;
> 177:   _num_obj_array_klasses = 0;
> 178:   _num_type_array_klasses = 0;

Should these be initializers also?

src/hotspot/share/memory/archiveUtils.hpp line 44:

> 42: class ArchivePtrMarker : AllStatic {
> 43:   static CHeapBitMap*  _ptrmap;
> 44:   static VirtualSpace* _vs;

I think this is a good change.

src/hotspot/share/memory/metaspaceShared.inline.hpp line 34:

> 32:
> 33: #if INCLUDE_CDS_JAVA_HEAP
> 34: bool MetaspaceShared::is_archive_object(oop p) {

Was this unused? I don't find 'is_archive_object' in the diff.

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>
> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>
> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>
> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:

 - fixed spaces
 - use member initializer list; clean up log message

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2536/files
  - new: https://git.openjdk.java.net/jdk/pull/2536/files/f32c40b6..9582e40f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=00-01

  Stats: 48 lines in 1 file changed: 20 ins; 17 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2536.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2536/head:pull/2536

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Fri, 12 Feb 2021 00:46:04 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - fixed spaces
>>  - use member initializer list; clean up log message
>
> src/hotspot/share/memory/metaspaceShared.inline.hpp line 34:
>
>> 32:
>> 33: #if INCLUDE_CDS_JAVA_HEAP
>> 34: bool MetaspaceShared::is_archive_object(oop p) {
>
> Was this unused? I don't find 'is_archive_object' in the diff.

This whole file was unused (and the function was not even declared in the MetaspaceShared class) so I removed it.

> src/hotspot/share/memory/archiveBuilder.cpp line 178:
>
>> 176:   _num_instance_klasses = 0;
>> 177:   _num_obj_array_klasses = 0;
>> 178:   _num_type_array_klasses = 0;
>
> Should these be initializers also?

I changed them to initializers.

> src/hotspot/share/classfile/moduleEntry.cpp line 33:
>
>> 31: #include "logging/log.hpp"
>> 32: #include "memory/archiveBuilder.hpp"
>> 33: #include "memory/archiveUtils.hpp"
>
> Do these files need archiveUtils.hpp or does archiveBuilder.hpp need it?

Both packageEntry.cpp and moduleEntry.cpp use ArchivePtrMarker which is defined in archiveUtils.hpp.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v2]

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Fri, 12 Feb 2021 04:08:03 GMT, Ioi Lam <[hidden email]> wrote:

>> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>>
>> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>>
>> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>>
>> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump
>
> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>
>  - fixed spaces
>  - use member initializer list; clean up log message

Looks good!

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v2]

Calvin Cheung
In reply to this post by Ioi Lam-2
On Fri, 12 Feb 2021 04:08:03 GMT, Ioi Lam <[hidden email]> wrote:

>> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>>
>> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>>
>> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>>
>> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump
>
> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>
>  - fixed spaces
>  - use member initializer list; clean up log message

Looks good overall. Couple of minor comments.

src/hotspot/share/memory/archiveBuilder.cpp line 197:

> 195:
> 196:   assert(_current == NULL, "must be");
> 197:   _current = this;

These lines used to be at the beginning of the function. Any reasons why they are moved?

test/hotspot/jtreg/runtime/cds/appcds/LotsOfClasses.java line 53:

> 51:         opts.addSuffix("-Xlog:gc+region+cds");
> 52:         //opts.addSuffix("-Xlog:gc+region=trace");
> 53:         opts.addSuffix("-Xlog:cds=debug");  // test detailed metadata info printing

Remove the commented line #52?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>
> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>
> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>
> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

  review comments by @calvinccheung

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2536/files
  - new: https://git.openjdk.java.net/jdk/pull/2536/files/9582e40f..54e7185f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=01-02

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

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v2]

Ioi Lam-2
In reply to this post by Calvin Cheung
On Fri, 12 Feb 2021 23:13:36 GMT, Calvin Cheung <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - fixed spaces
>>  - use member initializer list; clean up log message
>
> src/hotspot/share/memory/archiveBuilder.cpp line 197:
>
>> 195:
>> 196:   assert(_current == NULL, "must be");
>> 197:   _current = this;
>
> These lines used to be at the beginning of the function. Any reasons why they are moved?

I think it's better to set `_current` after the instance has been fully initialized.

> test/hotspot/jtreg/runtime/cds/appcds/LotsOfClasses.java line 53:
>
>> 51:         opts.addSuffix("-Xlog:gc+region+cds");
>> 52:         //opts.addSuffix("-Xlog:gc+region=trace");
>> 53:         opts.addSuffix("-Xlog:cds=debug");  // test detailed metadata info printing
>
> Remove the commented line #52?

I removed it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v3]

Calvin Cheung
In reply to this post by Ioi Lam-2
On Sat, 13 Feb 2021 05:04:58 GMT, Ioi Lam <[hidden email]> wrote:

>> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>>
>> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>>
>> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>>
>> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   review comments by @calvinccheung

Marked as reviewed by ccheung (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Fri, 12 Feb 2021 14:09:24 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - fixed spaces
>>  - use member initializer list; clean up log message
>
> Looks good!

Thanks @coleenp and @calvinccheung for your review!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261608: Move common CDS archive building code to archiveBuilder.cpp [v4]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>
> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>
> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>
> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

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

 - Merge branch 'master' into 8261608-move-common-archive-building-code
 - review comments by @calvinccheung
 - fixed spaces
 - use member initializer list; clean up log message
 - 8261608: Move common CDS archive building code to archiveBuilder.cpp

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2536/files
  - new: https://git.openjdk.java.net/jdk/pull/2536/files/54e7185f..8aaf33f6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2536&range=02-03

  Stats: 4934 lines in 207 files changed: 2805 ins; 891 del; 1238 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2536.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2536/head:pull/2536

PR: https://git.openjdk.java.net/jdk/pull/2536
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261608: Move common CDS archive building code to archiveBuilder.cpp

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 11 Feb 2021 23:41:34 GMT, Ioi Lam <[hidden email]> wrote:

> This is a follow-up to https://git.openjdk.java.net/jdk/pull/2296:
>
> - Move common code for writing the CDS archive from metaspaceShared.cpp to archiveBuilder.cpp
>
> - Data structures related to dumping were haphazardly organized in several classes (e.g., `DumpRegions`). We needed various APIs to access them across classes. These should be consolidated in archiveBuilder.cpp and the API should be cleaned up
>
> - Detailed stats (`DumpAllocStats::print_stats`) were available only for static dump. Refactor the code so they are also printed for dynamic dump

This pull request has now been integrated.

Changeset: d9744f65
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/d9744f65
Stats:     982 lines in 31 files changed: 346 ins; 481 del; 155 mod

8261608: Move common CDS archive building code to archiveBuilder.cpp

Reviewed-by: coleenp, ccheung

-------------

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