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

Flatten non-static NullRestricted fields #18173

Merged
merged 1 commit into from Sep 21, 2023

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Sep 20, 2023

Related: #17340

runtime/oti/j9.h Outdated
@@ -337,6 +337,7 @@ static const struct { \
* Disable flattening of volatile field that is > 8 bytes for now, as the current implementation of copyObjectFields() will tear this field.
*/
#define J9_IS_FIELD_FLATTENED(fieldClazz, romFieldShape) \
J9_ARE_NO_BITS_SET((romFieldShape)->modifiers, J9FieldFlagIsNullRestricted) || \
Copy link
Contributor

Choose a reason for hiding this comment

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

Only null restricted field can be potentially flattened with the newer javac, so it should be sth like J9_ARE_ALL_BITS_SET((romFieldShape)->modifiers, J9FieldFlagIsNullRestricted) && (ALL the existing conditions of J9_IS_FIELD_FLATTENED)

As the current javac does not produce null restricted attribute yet, for now to keep the builds working, we can keep this macro unchanged and add a new macro J9_IS_NULL_RESTRICTED_FIELD_FLATTENED and define it to

J9_ARE_ALL_BITS_SET((romFieldShape)->modifiers, J9FieldFlagIsNullRestricted) && J9_IS_FIELD_FLATTENED().

Then in this line, you can use J9_IS_NULL_RESTRICTED_FIELD_FLATTENED. Eventually we will remove the current J9_IS_FIELD_FLATTENED and rename J9_IS_NULL_RESTRICTED_FIELD_FLATTENED to J9_IS_FIELD_FLATTENED.

Also this line should be removed as it will always be false with the new javac. (If there are tests failed due to this remval, the test should be either rewritten or disabled)

But I think these changes can be done under #18157, not necessarily in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this line should be removed as it will always be false with the new javac. (If there are tests failed due to this remval, the test should be either rewritten or disabled)

I added a comment here asking about J9ROMCLASS_IS_PRIMITIVE_VALUE_TYPE().

runtime/vm/createramclass.cpp Show resolved Hide resolved
@hangshao0 hangshao0 added project:valhalla Used to track Project Valhalla related work comp:vm labels Sep 21, 2023
runtime/oti/j9.h Outdated
@@ -339,6 +339,10 @@ static const struct { \
#define J9_IS_FIELD_FLATTENED(fieldClazz, romFieldShape) \
(J9_IS_J9CLASS_FLATTENED(fieldClazz) && \
(J9_ARE_NO_BITS_SET((romFieldShape)->modifiers, J9AccVolatile) || (J9CLASS_UNPADDED_INSTANCE_SIZE(fieldClazz) <= sizeof(U_64))))
/* This will replace J9_IS_FIELD_FLATTENED when QTypes are removed */
#define J9_IS_NULL_RESTRICTED_FIELD_FLATTENED(fieldClazz, romFieldShape) \
J9_ARE_ALL_BITS_SET((romFieldShape)->modifiers, J9FieldFlagIsNullRestricted) && \
Copy link
Contributor

@hangshao0 hangshao0 Sep 21, 2023

Choose a reason for hiding this comment

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

I see J9_IS_J9CLASS_FLATTENED(fieldClazz) is removed. After looking at the code closer, I think we should still keep J9_IS_J9CLASS_FLATTENED(fieldClazz), as user can use -XX:ValueTypeFlatteningThreshold=<size> to control whether J9ClassIsFlattened will be set on a J9Class based on the instance size. This should change the field flattening behaviour.

Copy link
Contributor Author

@theresa-m theresa-m Sep 21, 2023

Choose a reason for hiding this comment

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

yes thats true. that addition will make it so J9_IS_NULL_RESTRICTED_FIELD_FLATTENED since its only true for primitive value classes but its going to get messier before it gets better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added J9_IS_NULL_RESTRICTED_FIELD_FLATTENED

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@hangshao0
Copy link
Contributor

Jenkins test sanity,extended zlinuxval jdknext

@hangshao0 hangshao0 merged commit 1d693b1 into eclipse-openj9:master Sep 21, 2023
6 checks passed
@theresa-m theresa-m deleted the flatten_instance_nr branch December 6, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants