-
Notifications
You must be signed in to change notification settings - Fork 143
8374800: [lworld] Add a NULLABLE_NON_ATOMIC_FLAT layout #1407
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
base: lworld
Are you sure you want to change the base?
Conversation
|
👋 Welcome back fparain! A progress list of the required criteria for merging this PR into |
|
@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: 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
This would forbid the class which contains the
This is equally valid to null-restricted field of non- |
|
@fparain This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
|
/keepalive |
|
@fparain The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
@fparain this pull request can not be integrated into git checkout flat_final
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push |
|
@fparain This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
Webrevs
|
Yes, there are some restrictions where the NULLABLE_NON_ATOMIC_FLAT can be used, see the comments and the |
|
I assume this PR is for flattening of nullable fields at immutable memory (strict final fields) only. But I think the same flattening can be done for null-free field, right? |
| _non_atomic_size_in_bytes(-1), _non_atomic_alignment(-1), | ||
| _atomic_layout_size_in_bytes(-1), _nullable_layout_size_in_bytes(-1), | ||
| _atomic_layout_size_in_bytes(-1), _nullable_atomic_layout_size_in_bytes(-1), | ||
| _nullable_non_atomic_layout_size_in_bytes(-1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need nullable_non_atomic_alignment, similar to non_atomic_alignment. Besides, it may be better to rename the null_free fields, too.
coleenp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple questions and comments. Looks good as an extension to the existing field layout design.
| break; | ||
| case LayoutKind::NULLABLE_NON_ATOMIC_FLAT: | ||
| assert(has_nullable_non_atomic_layout(), "Layout not available"); | ||
| return null_free_non_atomic_alignment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be nullable_non_atomic_alignment() ? Or is it the same alignment as null_free_non_atomic_alignment? (if so pls comment why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be nullable_non_atomic_alignment() ? Or is it the same alignment as null_free_non_atomic_alignment? (if so pls comment why)
Nullable non-atomic layouts have the same alignment constraint as null_free_non_atomic layouts.
Null-free non atomic layouts have an alignment constraint based on the constraints of individual fields inside the layout. The addition of the null marker, which is currently encoded with a byte, just adds the weakest alignment constraint to the set of constraints to be considered, so it has no effect, and the end results for nullable-non-atomic layouts is the same as for null-free-non-atomic layouts.
| break; | ||
| case LayoutKind::NULL_FREE_ATOMIC_FLAT: | ||
| return has_atomic_layout(); | ||
| return has_null_free_atomic_layout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a good renaming.
| int _null_free_non_atomic_alignment; // alignment requirement for null-free non-atomic layout | ||
| int _null_free_atomic_size_in_bytes; // size and alignment requirement for a null-free atomic layout, -1 if no atomic flat layout is possible | ||
| int _nullable_atomic_size_in_bytes; // size and alignment requirement for a nullable layout (always atomic), -1 if no nullable flat layout is possible | ||
| int _nullable_non_atomic_size_in_bytes; // size and alignment requirement for a nullable non-atomic layout, -1 if not available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is something that I'm confused about. Does an InlineKlass have multiple possible layouts and sizes depending on the container? Is that why the InlineKlass needs to save all these size non-static data members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you align the // s please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is something that I'm confused about. Does an InlineKlass have multiple possible layouts and sizes depending on the container? Is that why the InlineKlass needs to save all these size non-static data members?
An InlineKlass supports a set of layout according to its own characteristics (number of fields, atomicity, etc.).
When computing the layout of a container with a value field, the JVM looks at the characteristics of the field (static, final , strict, null-free, etc.) and the layouts available for this InlineKlass and selects the best one that satisfies all requirements.
Yes, there are some restrictions where the NULLABLE_NON_ATOMIC_FLAT can be used, see the comments and the
Strict final null-free fields could also be flattened without having to be concerned about atomicity, but null-freeness is not part of JEP 401. So this case is not supported in this PR. It can be added later when the null-restriction JEP has been finalized. |
coleenp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
coleenp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Strict final instance fields are not subject to concurrent writes during a read access, so they can be flattened even if they are nullable and bigger than 64 bits. The NULLABLE_NON_ATOMIC_FLAT layout is added for this particular case.
This new layout can also be used in the special case of nullable empty value classes, because their payload contains a single entry, the null-marker, which is naturally atomic.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1407/head:pull/1407$ git checkout pull/1407Update a local copy of the PR:
$ git checkout pull/1407$ git pull https://git.openjdk.org/valhalla.git pull/1407/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1407View PR using the GUI difftool:
$ git pr show -t 1407Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1407.diff
Using Webrev
Link to Webrev Comment