RFR: 8253303 G1: Move static initialization of G1FromCardCache to a proper location
in src/hotspot/share/gc/g1/g1RemSet.cpp: 488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 } the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 running ------------- Commit messages: - Initial revision Changes: https://git.openjdk.java.net/jdk/pull/247/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=247&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253303 Stats: 36 lines in 5 files changed: 17 ins; 12 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/247.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/247/head:pull/247 PR: https://git.openjdk.java.net/jdk/pull/247
On Fri, 18 Sep 2020 11:17:08 GMT, Thomas Schatzl <tschatzl@openjdk.org> wrote:
in src/hotspot/share/gc/g1/g1RemSet.cpp:
488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 }
the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 running
Marked as reviewed by ayang (Author). ------------- PR: https://git.openjdk.java.net/jdk/pull/247
Hi, On 18.09.20 15:22, Albert Mingkun Yang wrote:
On Fri, 18 Sep 2020 11:17:08 GMT, Thomas Schatzl <tschatzl@openjdk.org> wrote:
in src/hotspot/share/gc/g1/g1RemSet.cpp:
488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 }
the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 running
Marked as reviewed by ayang (Author).
-------------
thanks for your review. Thomas
On Fri, 18 Sep 2020 11:17:08 GMT, Thomas Schatzl <tschatzl@openjdk.org> wrote:
in src/hotspot/share/gc/g1/g1RemSet.cpp:
488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 }
the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 passed
Nice cleanup Thomas, consider it approved. Just one minor thing before integrating. src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1690:
1688: _rem_set->initialize(max_reserved_regions()); 1689: 1690:
Please remove this additional line before integrating, unless you added it for good reason :) ------------- Marked as reviewed by sjohanss (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/247
Hi Stefan, Albert, thanks for your reviews. On 18.09.20 22:09, Stefan Johansson wrote:
On Fri, 18 Sep 2020 11:17:08 GMT, Thomas Schatzl <tschatzl@openjdk.org> wrote:
in src/hotspot/share/gc/g1/g1RemSet.cpp:
488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 }
the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 passed
Nice cleanup Thomas, consider it approved.
Just one minor thing before integrating.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1690:
1688: _rem_set->initialize(max_reserved_regions()); 1689: 1690:
Please remove this additional line before integrating, unless you added it for good reason :)
-------------
Done. Waiting a bit to push. Thanks, Thomas
in src/hotspot/share/gc/g1/g1RemSet.cpp:
488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 }
the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 passed
Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Remove newline ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/247/files - new: https://git.openjdk.java.net/jdk/pull/247/files/d327a515..17b7cd46 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=247&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=247&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/247.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/247/head:pull/247 PR: https://git.openjdk.java.net/jdk/pull/247
On Fri, 18 Sep 2020 11:17:08 GMT, Thomas Schatzl <tschatzl@openjdk.org> wrote:
in src/hotspot/share/gc/g1/g1RemSet.cpp:
488 void G1RemSet::initialize(uint max_reserved_regions) { 489 G1FromCardCache::initialize(num_par_rem_sets(), max_reserved_regions); 490 _scan_state->initialize(max_reserved_regions); 491 }
the static initialization is piggy-backed to G1RemSet instance initialization which is rather bad style. Fortunately there is only one G1RemSet instance and that method is only called once so no functional issue. This change moves the initialization call, and after a bit of analysis move some more related code to more appropriate places. Testing: local compilation, jtreg gc/g1, tier1-3 passed
This pull request has now been integrated. Changeset: 955c2e62 Author: Thomas Schatzl <tschatzl@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/955c2e62 Stats: 35 lines in 5 files changed: 12 ins; 16 del; 7 mod 8253303: G1: Move static initialization of G1FromCardCache to a proper location Reviewed-by: ayang, sjohanss ------------- PR: https://git.openjdk.java.net/jdk/pull/247
participants (4)
-
Albert Mingkun Yang
-
Stefan Johansson
-
Thomas Schatzl
-
Thomas Schatzl