Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253113: [lworld] [lw3] C1 should avoid copying element of flattened arrays when reading a sub-element #187

Closed
wants to merge 10 commits into from

Conversation

fparain
Copy link
Collaborator

@fparain fparain commented Sep 14, 2020

Please review these changes optimizing access to flattened arrays in C1.

The optimization avoids unnecessary copies of intermediate values when the code accesses a sub-element of a flattened array.

For instance, with the following code:

inline class Point { int x = 0, y = 0; }
void foo() {
Point[] array = new Point[10];
int i = array[1].x;
}

C1 used to create a new instance of Point, copy the content from the array, then reads the x field from this new instance.
With the optimization, C1 directly accesses the x field from the flattened array, and makes no heap allocation.

A simple benchmark on the "int i = array[1].x;" line gives these results:

baseline:
Benchmark Mode Samples Score Score error Units
o.s.MyBenchmark.testArrayReads avgt 200 4.250 0.011 ns/op

optimized:
Benchmark Mode Samples Score Score error Units
o.s.MyBenchmark.testArrayReads avgt 200 2.228 0.004 ns/op

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8253113: [lworld] [lw3] C1 should avoid copying element of flattened arrays when reading a sub-element

Reviewers

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/187/head:pull/187
$ git checkout pull/187

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 14, 2020

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

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@fparain This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253113: [lworld] [lw3] C1 should avoid copying element of flattened arrays when reading a sub-element

Reviewed-by: thartmann
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the lworld branch:

  • 07e2fc5: 8253161: [lworld] C1's substitutability check should use andptr instead of andl for mark word

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge lworld into your branch, and then specify the current head hash when integrating, like this: /integrate 07e2fc53c1407f6e34e2cb147f5d61190b4227d0.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2020

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me, added some minor comments.

// Find the starting address of the source (inside the array)
ciType* array_type = array.value()->declared_type();
ciFlatArrayKlass* flat_array_klass = array_type->as_flat_array_klass();
assert(flat_array_klass->is_loaded(), "must be");

ciInlineKlass* elem_klass = flat_array_klass->element_klass()->as_inline_klass();
ciInlineKlass* elem_klass = NULL;
Copy link
Member

@TobiHartmann TobiHartmann Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused.

Copy link
Collaborator Author

@fparain fparain Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


BasicType subelt_type = field->type()->basic_type();

#ifndef _LP64
Copy link
Member

@TobiHartmann TobiHartmann Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong indentation.

Copy link
Collaborator Author

@fparain fparain Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -1595,13 +1595,84 @@ class TempResolvedAddress: public Instruction {
virtual const char* name() const { return "TempResolvedAddress"; }
};

void LIRGenerator::access_flattened_array(bool is_load, LIRItem& array, LIRItem& index, LIRItem& obj_item) {
void LIRGenerator::access_sub_element(LIRItem& array, LIRItem& index, LIR_Opr& result, ciField* field, int sub_offset) {
Copy link
Member

@TobiHartmann TobiHartmann Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is quite some code duplication here with access_flattened_array, can we factor it out?

Copy link
Collaborator Author

@fparain fparain Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code has been refactored, and the common code in access_flattened_array() and access_sub_element() is now shared in the new method get_and_load_element_address().

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good, thanks for making these changes!

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Nice refactoring. Looks good to me.

void LIRGenerator::access_sub_element(LIRItem& array, LIRItem& index, LIR_Opr& result, ciField* field, int sub_offset) {
assert(field != NULL, "Need a subelement type specified");

// Find the starting address of the source (inside the array)
Copy link
Member

@TobiHartmann TobiHartmann Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is wrong.

ciField* field, int sub_offset) {
assert(sub_offset == 0 || field != NULL, "Sanity check");

// Find the starting address of the source (inside the array)
Copy link
Member

@TobiHartmann TobiHartmann Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is wrong.

@fparain
Copy link
Collaborator Author

@fparain fparain commented Sep 15, 2020

Thanks Tobias for your review.
I've fixed the wrong indentations.

Fred

@fparain
Copy link
Collaborator Author

@fparain fparain commented Sep 15, 2020

/integrate

@openjdk openjdk bot closed this Sep 15, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 15, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2020

@fparain Since your change was applied there has been 1 commit pushed to the lworld branch:

  • 07e2fc5: 8253161: [lworld] C1's substitutability check should use andptr instead of andl for mark word

Your commit was automatically rebased without conflicts.

Pushed as commit 1a52191.

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

@fparain fparain deleted the c1_aaload_opt branch Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants