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

8257600: [type-restrictions] Implement RestrictedField in C1 #288

Closed
wants to merge 12 commits into from

Conversation

fparain
Copy link
Collaborator

@fparain fparain commented Dec 2, 2020

Please review theses changes adding RestrictedField support to C1.
Tests have been added using the annotation added by JDK-8255856.
Tests revealed some issues with the support of RestrictedField in the interpreter, fixes are included in this patch.
The changes also includes some renaming after discussions with Dan.

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8257600: [type-restrictions] Implement RestrictedField in C1

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 2, 2020

👋 Welcome back fparain! A progress list of the required criteria for merging this PR into type-restrictions 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 bot commented Dec 2, 2020

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

8257600: [type-restrictions] Implement RestrictedField in C1

Reviewed-by: lfoltan, thartmann

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 152 new commits pushed to the type-restrictions branch:

  • 2f7d9f7: Merge lworld
  • 8efeb68: Merge jdk
  • 7aed9b6: 8256016: Dacapo24H.java failed with "assert(false) failed: unscheduable graph"
  • 26e6cb3: 8256489: Make gtest for long path names on Windows more resilient in the presence of virus scanners
  • 911f16d: 8257056: Submit workflow should apt-get update to avoid package installation errors
  • b0bd0c2: 8256755: Update build.tools.depend.Depend to handle record components in API signatures
  • 9aeadbb: 8256865: Foreign Memory Access and Linker API are missing NPE checks
  • 8cd2e0f: 8243315: ParallelScavengeHeap::initialize() passes GenAlignment as page size to os::trace_page_sizes instead of actual page size
  • cdb41ba: 8255904: Remove superfluous use of reflection in Class::isRecord
  • c45725e: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing
  • ... and 142 more: https://git.openjdk.java.net/valhalla/compare/4902cf9c248bfc51d370ed930adad9474adb03d4...type-restrictions

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 type-restrictions branch, type /integrate in a new comment.

@mlbridge
Copy link

mlbridge bot commented Dec 2, 2020

Webrevs

@forax
Copy link
Member

forax commented Dec 2, 2020

Hi,
the requirement that a restricted type must be an inline type is too strong,
the restricted type should be able to be at least the same as the field descriptor too otherwise you can not using it in a template class if the argument type is not an inline class.

And for other languages than Java that may want to support full reification, it's better to support any subtypes.

lfoltan
lfoltan approved these changes Dec 2, 2020
Copy link
Member

@lfoltan lfoltan left a comment

The interpreter and class file parsing changes look good. I left a few minor comments to consider. I did not review in depth the c1 changes.
Thanks,
Lois

// The current model for restricted field is that such a field has a descriptor signature used
// as the normal signature for this field (for instance in field access bytecodes) but it also
// has a restricted type that will be used internally by the VM as the real type of the field.
// Current constrains are that the restricted type must be an inline type and the descriptor
Copy link
Member

@lfoltan lfoltan Dec 2, 2020

Choose a reason for hiding this comment

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

Small nit - contrains --> contraints

Klass* desc_klass = SystemDictionary::resolve_or_null(descriptor_name,
Handle(THREAD, _loader_data->class_loader()),
_protection_domain, CHECK);
if (desc_klass == NULL || !klass->is_subtype_of(desc_klass)) {
Copy link
Member

@lfoltan lfoltan Dec 2, 2020

Choose a reason for hiding this comment

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

Consider if using a ResolvingSignatureStream & a call to ResolvingSignatureStream::as_klass() might be a more JVM consistent way to obtain the Klass from the descriptor signature.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Compiler changes look good to me!

@fparain
Copy link
Collaborator Author

fparain commented Dec 3, 2020

Lois, Tobias,

thank you for your reviews.
Lois' comments have been addressed in the latest commit.

@fparain
Copy link
Collaborator Author

fparain commented Dec 3, 2020

Remi,

The current goal of the type restrictions branch is to measure the impact and the cost of type restrictions checks, so features are being added incrementally (not a full model yet). The current goal is not to provide a support for full reification, and some models being proposed on top of type restrictions are relying on the fact that type restrictions can only be applied to primitive classes.

For the first phase of this exploration, we should stick to the simple model until we reach the first mile-stone: full support of RestrictedField and RestrictedMethod in the interpreter, C1 and C2.

By the time we implement this first step, we hope we'll have a better understanding on how to evolve the model, relaxing constraints, keeping them or modifying them.

Regards,

Fred

@fparain
Copy link
Collaborator Author

fparain commented Dec 3, 2020

/integrate

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

openjdk bot commented Dec 3, 2020

@fparain Since your change was applied there have been 152 commits pushed to the type-restrictions branch:

  • 2f7d9f7: Merge lworld
  • 8efeb68: Merge jdk
  • 7aed9b6: 8256016: Dacapo24H.java failed with "assert(false) failed: unscheduable graph"
  • 26e6cb3: 8256489: Make gtest for long path names on Windows more resilient in the presence of virus scanners
  • 911f16d: 8257056: Submit workflow should apt-get update to avoid package installation errors
  • b0bd0c2: 8256755: Update build.tools.depend.Depend to handle record components in API signatures
  • 9aeadbb: 8256865: Foreign Memory Access and Linker API are missing NPE checks
  • 8cd2e0f: 8243315: ParallelScavengeHeap::initialize() passes GenAlignment as page size to os::trace_page_sizes instead of actual page size
  • cdb41ba: 8255904: Remove superfluous use of reflection in Class::isRecord
  • c45725e: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing
  • ... and 142 more: https://git.openjdk.java.net/valhalla/compare/4902cf9c248bfc51d370ed930adad9474adb03d4...type-restrictions

Your commit was automatically rebased without conflicts.

Pushed as commit ab439dc.

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

@forax
Copy link
Member

forax commented Dec 3, 2020

Hi Frédéric,
nevermind, in the meantime, i can patch the attribute name so if it's not a record the attribute itself will not be recognized

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants