8350485: C2: factor out common code in Node::grow() and Node::out_grow() #23928

sarannat · 2025-03-06T09:08:41Z

Node:grow() and Node::out_grow() are copy-pasted from each other and their core logic could be factored out into a third function or at least cleaned up. Hence,the fix includes a function Node::array_resize() that implements the core logic of Node::grow() and Node::out_grow().

Link to Github action which had no failures : https://github.com/sarannat/jdk/actions/runs/13677508359

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-8350485: C2: factor out common code in Node::grow() and Node::out_grow() (Enhancement - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23928

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-06T09:09:16Z

Hi @sarannat, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user sarannat" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-03-06T09:09:28Z

@sarannat 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:

8350485: C2: factor out common code in Node::grow() and Node::out_grow()

Reviewed-by: thartmann, rcastanedalo

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 184 new commits pushed to the master branch:

e57b272: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor
577ede7: 8352302: Test sun/security/tools/jarsigner/TimestampCheck.java is failing
20d4fe3: 8351464: Shenandoah: Hang on ShenandoahController::handle_alloc_failure when run test TestAllocHumongousFragment#generational
8e53063: 8352275: Clean up dead code in jpackage revealed with improved negative test coverage
c8a11f2: 8352293: jpackage tests build rpm packages on Ubuntu test machines after JDK-8351372
4a02de8: 8352098: -Xrunjdwp fails on static JDK
355b2f3: 8351374: Improve comment about queue.remove timeout in CleanerImpl.run
a3540be: 8352163: [AIX] SIGILL in AttachOperation::ReplyWriter::write_fully after 8319055
53c5b93: 8352180: AttachListenerThread causes many tests to timeout on Windows
b025d8c: 8350663: AArch64: Enable UseSignumIntrinsic by default
... and 174 more: https://git.openjdk.org/jdk/compare/8073914af7d4ddd7bbd93d75104c7637e38a7ad9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@eme64, @TobiHartmann, @robcasloz) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-03-06T09:09:51Z

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

hotspot-compiler

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.

sarannat · 2025-03-06T09:36:47Z

/covered

bridgekeeper · 2025-03-06T09:37:00Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

eme64

@sarannat I'm mostly leaving code style comments. I promise I won't be so pedantic in the future ;)

It may be good for you to read through this, at least to have an overview:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md

src/hotspot/share/opto/node.hpp

src/hotspot/share/opto/node.cpp

TobiHartmann

Congratulations on your first PR, Saranya! 🥳

I also left a code style comment but looks good to me otherwise.

TobiHartmann · 2025-03-07T14:20:18Z

src/hotspot/share/opto/node.cpp

-  _in = (Node**)arena->Arealloc(_in, _max*sizeof(Node*), new_max*sizeof(Node*));
-  Copy::zero_to_bytes(&_in[_max], (new_max-_max)*sizeof(Node*)); // null all new space
-  _max = new_max;               // Record new max length
+  if(!is_in){


Suggested change

if(!is_in){

if (!is_in) {

Same below.

mlbridge · 2025-03-10T10:58:26Z

Webrevs

robcasloz

Thanks for cleaning up this code, Saranya! I have a few more style/naming suggestions, the refactored logic looks otherwise good to me.

robcasloz · 2025-03-11T08:49:39Z

src/hotspot/share/opto/node.hpp

+  // Resize input or output array to grow it the next larger power-of-2 bigger
+  // than len.


Suggested change

// Resize input or output array to grow it the next larger power-of-2 bigger

// than len.

// Resize input or output array to grow it to the next larger power-of-2

// bigger than len.

robcasloz · 2025-03-11T08:53:11Z

src/hotspot/share/opto/node.cpp

-//------------------------------grow-------------------------------------------
-// Grow the input array, making space for more edges
-void Node::grow(uint len) {
+// Resize input or output array to grow it the next larger power-of-2 bigger


Suggested change

// Resize input or output array to grow it the next larger power-of-2 bigger

// Resize input or output array to grow it to the next larger power-of-2 bigger

robcasloz · 2025-03-11T09:02:23Z

src/hotspot/share/opto/node.cpp

  // This assertion makes sure that Node::_max is wide enough to
  // represent the numerical value of new_max.
-  assert(_max == new_max && _max > len, "int width of _max is too small");
+  assert(max_size == new_max && max_size > len, "int width of _max is too small");


This is pre-existing, but I think it is worth simplifying anyway:

Suggested change

assert(max_size == new_max && max_size > len, "int width of _max is too small");

assert(max_size > len, "int width of _max is too small");

robcasloz · 2025-03-11T09:03:27Z

src/hotspot/share/opto/node.cpp

  new_max = next_power_of_2(len);
  // Trimming to limit allows a uint8 to handle up to 255 edges.
  // Previously I was using only powers-of-2 which peaked at 128 edges.
  //if( new_max >= limit ) new_max = limit-1;


I suggest to remove this line that was already commented out before this changeset.

robcasloz · 2025-03-11T09:05:47Z

src/hotspot/share/opto/node.cpp

+  if (!is_input_array) {
+    assert(array != nullptr && array != NO_OUT_ARRAY, "out must have sensible value");
+  }


This is somewhat subjective, but I prefer to inline the !is_input_array pre-condition into the assertion itself, for compactness.

Suggested change

if (!is_input_array) {

assert(array != nullptr && array != NO_OUT_ARRAY, "out must have sensible value");

}

assert(is_input_array || (array != nullptr && array != NO_OUT_ARRAY), "out must have sensible value");

robcasloz · 2025-03-11T09:10:23Z

src/hotspot/share/opto/node.cpp


 //-----------------------------out_grow----------------------------------------
 // Grow the input array, making space for more edges
 void Node::out_grow( uint len ) {


For style consistency with Node::grow:

Suggested change

void Node::out_grow( uint len ) {

void Node::out_grow(uint len) {

robcasloz · 2025-03-11T09:15:13Z

src/hotspot/share/opto/node.cpp

-void Node::grow(uint len) {
+// Resize input or output array to grow it the next larger power-of-2 bigger
+// than len.
+void Node::resize_array(Node**& array, node_idx_t& max_size, uint len, bool is_input_array) {


This is subjective, but I would find it clearer if the name of bool is_input_array reflected what does resize_array needs to do with array rather than what is the source/origin of array. My suggestion would be something like bool needs_clearing, bool initialize_to_null, or similar.

…emoving comments

robcasloz

Looks good, thanks!

TobiHartmann

Looks good to me too.

sarannat · 2025-03-19T08:55:43Z

/integrate

openjdk · 2025-03-19T08:57:01Z

@sarannat
Your change (at version 6e94377) is now ready to be sponsored by a Committer.

robcasloz · 2025-03-19T09:58:39Z

/sponsor

openjdk · 2025-03-19T09:59:19Z

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

c2be19c: 8351902: RISC-V: Several tests fail after JDK-8351145
e57b272: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor
577ede7: 8352302: Test sun/security/tools/jarsigner/TimestampCheck.java is failing
20d4fe3: 8351464: Shenandoah: Hang on ShenandoahController::handle_alloc_failure when run test TestAllocHumongousFragment#generational
8e53063: 8352275: Clean up dead code in jpackage revealed with improved negative test coverage
c8a11f2: 8352293: jpackage tests build rpm packages on Ubuntu test machines after JDK-8351372
4a02de8: 8352098: -Xrunjdwp fails on static JDK
355b2f3: 8351374: Improve comment about queue.remove timeout in CleanerImpl.run
a3540be: 8352163: [AIX] SIGILL in AttachOperation::ReplyWriter::write_fully after 8319055
53c5b93: 8352180: AttachListenerThread causes many tests to timeout on Windows
... and 175 more: https://git.openjdk.org/jdk/compare/8073914af7d4ddd7bbd93d75104c7637e38a7ad9...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-19T09:59:29Z

@robcasloz @sarannat Pushed as commit 8f64ccc.

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

sarannat added 3 commits March 4, 2025 23:59

JDK-8350485: Added resize_array

db43806

JDK-8350485:Code formatting and comments added to resize_array

a268bbb

JDK-8350485: Code formatting to array_resize

017fc16

bridgekeeper bot added the oca Needs verification of OCA signatory status label Mar 6, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 6, 2025

sarannat changed the title ~~JDK 8350485~~ JDK 8350485: C2: factor out common code in Node::grow() and Node::out_grow() Mar 6, 2025

sarannat changed the title ~~JDK 8350485: C2: factor out common code in Node::grow() and Node::out_grow()~~ 8350485: C2: factor out common code in Node::grow() and Node::out_grow() Mar 6, 2025

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Mar 6, 2025

eme64 suggested changes Mar 7, 2025

View reviewed changes

TobiHartmann suggested changes Mar 7, 2025

View reviewed changes

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Mar 10, 2025

openjdk bot added the rfr Pull request is ready for review label Mar 10, 2025

8350485: Addressing review comments on code style

f308762

robcasloz reviewed Mar 11, 2025

View reviewed changes

8350485: Addressing review comments with code formatting and fixing/r…

6e94377

…emoving comments

robcasloz approved these changes Mar 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 12, 2025

TobiHartmann approved these changes Mar 14, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 19, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 19, 2025

openjdk bot closed this Mar 19, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 19, 2025

		// Resize input or output array to grow it the next larger power-of-2 bigger
		// than len.

	assert(max_size == new_max && max_size > len, "int width of _max is too small");
	assert(max_size > len, "int width of _max is too small");

	void Node::out_grow( uint len ) {
	void Node::out_grow(uint len) {

8350485: C2: factor out common code in Node::grow() and Node::out_grow() #23928

8350485: C2: factor out common code in Node::grow() and Node::out_grow() #23928

Uh oh!

Conversation

sarannat commented Mar 6, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 6, 2025

Uh oh!

openjdk bot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 6, 2025

Uh oh!

sarannat commented Mar 6, 2025

Uh oh!

bridgekeeper bot commented Mar 6, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

robcasloz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robcasloz left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

sarannat commented Mar 19, 2025

Uh oh!

openjdk bot commented Mar 19, 2025

Uh oh!

robcasloz commented Mar 19, 2025

Uh oh!

openjdk bot commented Mar 19, 2025

Uh oh!

openjdk bot commented Mar 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

sarannat commented Mar 6, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 6, 2025 •

edited

Loading

mlbridge bot commented Mar 10, 2025 •

edited

Loading