RFR: 8234534: Simplify CardTable code after CMS removal

[Thomas Schatzl-4]

Hi,

  can I have reviews for this cleanup that removes CMS specific code from `CardTable/CardTableRS`?

Note that there is still this "conc_scan" parameter passed to the card table that affects barrier code generation, for some reason also G1 barrier code generation although it should not as `G1CardTable::scanned_concurrently()` only used for the "normal" card table. Initial attempts showed that removing this is not straightforward, causing crashes and so I left it out for [JDK-8250941](https://bugs.openjdk.java.net/browse/JDK-8260941) so that this change is solely about removing unused code.

Testing: tier1-4, some tier1-5 runs earlier (before some removal of hunks for files only containing copyright updates or newline changes)

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

Commit messages:
 - Initial commit

Changes: https://git.openjdk.java.net/jdk/pull/2354/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2354&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8234534
  Stats: 197 lines in 7 files changed: 0 ins; 185 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2354.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2354/head:pull/2354

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

[Thomas Schatzl-4]

On Tue, 2 Feb 2021 15:13:38 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi,
>
>   can I have reviews for this cleanup that removes CMS specific code from `CardTable/CardTableRS`?
>
> Note that there is still this "conc_scan" parameter passed to the card table that affects barrier code generation, for some reason also G1 barrier code generation although it should not as `G1CardTable::scanned_concurrently()` only used for the "normal" card table. Initial attempts showed that removing this is not straightforward, causing crashes and so I left it out for [JDK-8250941](https://bugs.openjdk.java.net/browse/JDK-8260941) so that this change is solely about removing unused code.
>
> Testing: tier1-4, some tier1-5 runs earlier (before some removal of hunks for files only containing copyright updates or newline changes)

(latest tier1-4 testing still stuck on linux-aarch64, but everything else passed. I think there is no particular aarch64 specific change in there...)

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

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

[Albert Mingkun Yang]

On Tue, 2 Feb 2021 15:13:38 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi,
>
>   can I have reviews for this cleanup that removes CMS specific code from `CardTable/CardTableRS`?
>
> Note that there is still this "conc_scan" parameter passed to the card table that affects barrier code generation, for some reason also G1 barrier code generation although it should not as `G1CardTable::scanned_concurrently()` only used for the "normal" card table. Initial attempts showed that removing this is not straightforward, causing crashes and so I left it out for [JDK-8250941](https://bugs.openjdk.java.net/browse/JDK-8260941) so that this change is solely about removing unused code.
>
> Testing: tier1-4, some tier1-5 runs earlier (before some removal of hunks for files only containing copyright updates or newline changes)

Marked as reviewed by ayang (Author).

src/hotspot/share/gc/shared/cardTableRS.cpp line 442:

> 440:   CardTable(whole_heap, scanned_concurrently) { }
> 441:
> 442: CardTableRS::~CardTableRS() { }

Now that it's empty, is it possible to remove it completely?

src/hotspot/share/gc/shared/cardTableRS.hpp line 55:

> 53:   virtual void verify_used_region_at_save_marks(Space* sp) const NOT_DEBUG_RETURN;
> 54:
> 55:   void inline_write_ref_field_gc(void* field, oop new_val) {

It seems that the arg `new_val` is not used. Maybe remove it or add a comment saying it's an intentional omission.

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

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

[Kim Barrett-2]

On Tue, 2 Feb 2021 15:13:38 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi,
>
>   can I have reviews for this cleanup that removes CMS specific code from `CardTable/CardTableRS`?
>
> Note that there is still this "conc_scan" parameter passed to the card table that affects barrier code generation, for some reason also G1 barrier code generation although it should not as `G1CardTable::scanned_concurrently()` only used for the "normal" card table. Initial attempts showed that removing this is not straightforward, causing crashes and so I left it out for [JDK-8250941](https://bugs.openjdk.java.net/browse/JDK-8260941) so that this change is solely about removing unused code.
>
> Testing: tier1-4, some tier1-5 runs earlier (before some removal of hunks for files only containing copyright updates or newline changes)

Looks good to me, with the one minor nit I commented on and Albert's suggestions.

src/hotspot/share/gc/shared/cardTableRS.cpp line 43:

> 41: inline bool ClearNoncleanCardWrapper::clear_card(CardValue* entry) {
> 42:   CardValue entry_val = *entry;
> 43:   assert(entry_val == CardTableRS::dirty_card_val(),

Consider eliminating `entry_val` - just use `*entry` in the assert.

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

Marked as reviewed by kbarrett (Reviewer).

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

[Thomas Schatzl-4]

> Hi,
>
>   can I have reviews for this cleanup that removes CMS specific code from `CardTable/CardTableRS`?
>
> Note that there is still this "conc_scan" parameter passed to the card table that affects barrier code generation, for some reason also G1 barrier code generation although it should not as `G1CardTable::scanned_concurrently()` only used for the "normal" card table. Initial attempts showed that removing this is not straightforward, causing crashes and so I left it out for [JDK-8250941](https://bugs.openjdk.java.net/browse/JDK-8260941) so that this change is solely about removing unused code.
>
> Testing: tier1-4, some tier1-5 runs earlier (before some removal of hunks for files only containing copyright updates or newline changes)

Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:

  kimbarret, albertnetymk review

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2354/files
  - new: https://git.openjdk.java.net/jdk/pull/2354/files/5aa23d74..849c79bb

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

  Stats: 11 lines in 4 files changed: 0 ins; 6 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2354.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2354/head:pull/2354

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

[Thomas Schatzl-4]

On Thu, 4 Feb 2021 10:29:18 GMT, Kim Barrett <[hidden email]> wrote:

>> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   kimbarret, albertnetymk review
>
> Looks good to me, with the one minor nit I commented on and Albert's suggestions.

All fixed as suggested. Still compiles.

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

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

[Thomas Schatzl-4]

On Thu, 4 Feb 2021 10:29:18 GMT, Kim Barrett <[hidden email]> wrote:

>> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   kimbarret, albertnetymk review
>
> Looks good to me, with the one minor nit I commented on and Albert's suggestions.

Thanks @kimbarrett @albertnetymk for your reviews.

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

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

[Thomas Schatzl-4]

On Tue, 2 Feb 2021 15:13:38 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi,
>
>   can I have reviews for this cleanup that removes CMS specific code from `CardTable/CardTableRS`?
>
> Note that there is still this "conc_scan" parameter passed to the card table that affects barrier code generation, for some reason also G1 barrier code generation although it should not as `G1CardTable::scanned_concurrently()` only used for the "normal" card table. Initial attempts showed that removing this is not straightforward, causing crashes and so I left it out for [JDK-8250941](https://bugs.openjdk.java.net/browse/JDK-8260941) so that this change is solely about removing unused code.
>
> Testing: tier1-4, some tier1-5 runs earlier (before some removal of hunks for files only containing copyright updates or newline changes)

This pull request has now been integrated.

Changeset: 78b0d327
Author:    Thomas Schatzl <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/78b0d327
Stats:     205 lines in 9 files changed: 0 ins; 191 del; 14 mod

8234534: Simplify CardTable code after CMS removal

Reviewed-by: ayang, kbarrett

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

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