8295658 G1: Refactor G1SegmentedArray to indicate that it is an allocator #10767

walulyai · 2022-10-19T14:52:07Z

Hi,

Please review this refactoring change of G1SegmentedArray to G1MonotonicArena which is a more appropriate name for the intended use of the class objects.

The change has two commits, with the first commit refactoring the class names and the second commit adding the corresponding changes to file names.

Testing: builds

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8295658: G1: Refactor G1SegmentedArray to indicate that it is an allocator

Reviewers

Thomas Schatzl (@tschatzl - Reviewer)
Stefan Johansson (@kstefanj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10767/head:pull/10767
$ git checkout pull/10767

Update a local copy of the PR:
$ git checkout pull/10767
$ git pull https://git.openjdk.org/jdk pull/10767/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10767

View PR using the GUI difftool:
$ git pr show -t 10767

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10767.diff

bridgekeeper · 2022-10-19T15:01:48Z

👋 Welcome back iwalulya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-10-19T15:04:36Z

@walulyai The following label will be automatically applied to this pull request:

hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-10-19T15:08:30Z

Webrevs

01: Full - Incremental (5de77499)
00: Full (48be550f)

tschatzl

Lgtm sans the minor formatting issues.

tschatzl · 2022-10-20T10:00:04Z

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -145,7 +145,7 @@ class G1CollectedHeap : public CollectedHeap {
 private:
  G1ServiceThread* _service_thread;
  G1ServiceTask* _periodic_gc_task;
-  G1SegmentedArrayFreeMemoryTask* _free_segmented_array_memory_task;
+  G1MonotonicArenaFreeMemoryTask* _free_monotonic_arena_memory_task;


I do not think it is required to have variable names that repeat the whole type name, particularly if there is no ambiguity. I.e. _free_arena_memory_task seems sufficient instead of _free_monotonic_arena.... but I do not mind either way.

tschatzl · 2022-10-20T10:03:13Z

src/hotspot/share/gc/g1/g1SegmentedArray.hpp

+// Actual allocation from the C heap occurs as memory blocks called Segments.
+// The allocation pattern for these Segments is assumed to be strictly two-phased:
+//
+// - in the first phase, Segment are allocated from the C heap (or a free


(probably pre-existing) s/Segment/Segments or "Segment instances"

tschatzl · 2022-10-20T10:04:09Z

src/hotspot/share/gc/g1/g1SegmentedArray.hpp

+// The class also manages a few counters for statistics using atomic operations.
+// Their values are only consistent within each other with extra global
+// synchronization.
+


(pre-existing) additional newline

tschatzl · 2022-10-20T10:06:36Z

src/hotspot/share/gc/g1/g1SegmentedArray.hpp

+  // AllocOptions provides parameters for allocation segment
+  // sizing and expansion.


Maybe:

AllocOptions provides parameters for Segment sizing and expansion.

tschatzl · 2022-10-20T10:07:13Z

src/hotspot/share/gc/g1/g1SegmentedArray.hpp

+  volatile size_t _mem_size;      // Memory used by all segments.
+
+  SegmentFreeList* _segment_free_list;   // The global free segment list to preferentially
+                                      // get new segments from.


Comment should be aligned to the start of the one above.

openjdk · 2022-10-20T10:13:51Z

@walulyai This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8295658: G1: Refactor G1SegmentedArray to indicate that it is an allocator

Reviewed-by: tschatzl, sjohanss

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 210 new commits pushed to the master branch:

d634dde: 8295354: Remove G1 incremental non-copy time calculation
8836b92: 8296226: Add constructors (String,Throwable) and (Throwable) to InvalidParameterException
51f8e9b: 8296443: NMT: Remove cmdline_tracking_level handling code
087cedc: 8295261: RISC-V: Support ReductionV instructions for Vector API
556377a: 8296270: Memory leak in ClassLoader::setup_bootstrap_search_path_impl
c2f7638: 8296335: Fix accessibility manual test instruction
91292d5: 8286301: Port JEP 425 to RISC-V
581133a: 8294696: BufferedInputStream.transferTo should drain buffer when mark set
d8573b2: 8294488: Delete KCMS transforms wrappers
f857f79: 8015739: Background of JInternalFrame is located out of JInternalFrame
... and 200 more: https://git.openjdk.org/jdk/compare/857b0f9b05bc711f3282a0da85fcff131fffab91...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

tschatzl

still good.

kstefanj

Nice refactoring, looks good.

walulyai · 2022-11-08T09:00:32Z

Thanks @tschatzl and @kstefanj for the reviews.

/integrate

openjdk · 2022-11-08T09:02:26Z

Going to push as commit dd5d4df.
Since your change was applied there have been 225 commits pushed to the master branch:

cf65605: 8296445: C++ syntax error in jdwpTransport.h
1169dc0: 8296447: RISC-V: Make the operands order of vrsub_vx/vrsub_vi consistent with RVV 1.0 spec
4c80dff: 8296435: RISC-V: Small refactoring for increment/decrement
47d2c7b: 8295376: Improve debug agent virtual thread performance when no debugger is attached
76790ad: 8295673: Deprecate and disable legacy parallel class loading workaround for non-parallel-capable class loaders
b6738c1: 8295663: Rephrase introduction to testing.md
7e85b41: 8296154: [macos] Change "/Applications" to "Applications" in DMG image
60db5f2: 8294020: improve errors for record declarations
520db1e: 8296485: BuildEEBasicConstraints.java test fails with SunCertPathBuilderException
b29817a: 8296479: Remove stray comment about POST_STRIP_CMD
... and 215 more: https://git.openjdk.org/jdk/compare/857b0f9b05bc711f3282a0da85fcff131fffab91...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-08T09:02:43Z

@walulyai Pushed as commit dd5d4df.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

walulyai added 2 commits October 19, 2022 12:36

refactor

a744991

Rename files

48be550

openjdk bot added the rfr Pull request is ready for review label Oct 19, 2022

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Oct 19, 2022

tschatzl approved these changes Oct 20, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 20, 2022

Thomas review

5de7749

tschatzl approved these changes Nov 2, 2022

View reviewed changes

kstefanj approved these changes Nov 7, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 8, 2022

openjdk bot closed this Nov 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 8, 2022

walulyai deleted the JDK-8295658 branch February 20, 2023 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295658 G1: Refactor G1SegmentedArray to indicate that it is an allocator #10767

8295658 G1: Refactor G1SegmentedArray to indicate that it is an allocator #10767

walulyai commented Oct 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 19, 2022

openjdk bot commented Oct 19, 2022

mlbridge bot commented Oct 19, 2022 •

edited

Loading

tschatzl left a comment

tschatzl Oct 20, 2022

tschatzl Oct 20, 2022

tschatzl Oct 20, 2022

tschatzl Oct 20, 2022

tschatzl Oct 20, 2022

openjdk bot commented Oct 20, 2022 •

edited

Loading

tschatzl left a comment

kstefanj left a comment

walulyai commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

		// AllocOptions provides parameters for allocation segment
		// sizing and expansion.

8295658 G1: Refactor G1SegmentedArray to indicate that it is an allocator #10767

8295658 G1: Refactor G1SegmentedArray to indicate that it is an allocator #10767

Conversation

walulyai commented Oct 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 19, 2022

openjdk bot commented Oct 19, 2022

mlbridge bot commented Oct 19, 2022 • edited Loading

Webrevs

tschatzl left a comment

Choose a reason for hiding this comment

tschatzl Oct 20, 2022

Choose a reason for hiding this comment

tschatzl Oct 20, 2022

Choose a reason for hiding this comment

tschatzl Oct 20, 2022

Choose a reason for hiding this comment

tschatzl Oct 20, 2022

Choose a reason for hiding this comment

tschatzl Oct 20, 2022

Choose a reason for hiding this comment

openjdk bot commented Oct 20, 2022 • edited Loading

tschatzl left a comment

Choose a reason for hiding this comment

kstefanj left a comment

Choose a reason for hiding this comment

walulyai commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

walulyai commented Oct 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 19, 2022 •

edited

Loading

openjdk bot commented Oct 20, 2022 •

edited

Loading