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

Move MonoClass:inlinearray_value out of MonoClass #109363

Merged
merged 11 commits into from
Nov 25, 2024

Conversation

syfFerdinand
Copy link
Contributor

@syfFerdinand syfFerdinand commented Oct 29, 2024

Summary

This PR addresses issue #103913 by moving the inlinearray_value property from MonoClass to the infrequent_data structure. This change aims to optimize memory usage by storing rarely used properties in a less frequently accessed structure.

Details

  • Moved inlinearray_value from MonoClass to MonoPropertyBag within the infrequent_data structure.
  • Set explicit value for PROP_INLINEARRAY_VALUE in InfrequentDataKind enum, as per review comments.
  • Updated relevant getters and setters to reflect the new structure.
  • Marked mono_class_get_inlinearray_value with MONO_COMPONENT_API as per review suggestion to ensure proper export when compiled as shared libraries.
  • Modified all occurrences of inlinearray_value access to use the new structure.
  • Performed testing to ensure no functionality is broken due to this change.

Related Issue

Fixes #103913

@syfFerdinand
Copy link
Contributor Author

@dotnet-policy-service agree

@lambdageek
Copy link
Member

Thanks for the contribution @syfFerdinand !

@lambdageek
Copy link
Member

/cc @steveisok

src/mono/mono/metadata/property-bag.h Outdated Show resolved Hide resolved
@syfFerdinand syfFerdinand changed the title Moved inlinearray_value to infrequent_data structure in MonoClass Move MonoClass:inlinearray_value out of MonoClass Nov 8, 2024
@steveisok
Copy link
Member

@kg can you please give this a review?

@steveisok
Copy link
Member

Can you see https://dev.azure.com/dnceng-public/public/_build/results?buildId=865527&view=artifacts&pathAsName=false&type=publishedArtifacts ? I believe there are downloads for the offsets there, which might be what you need.

Note #109612 added the actual offset files. Any updates should go into these.

@syfFerdinand
Copy link
Contributor Author

Hi @kg and @steveisok,
Just to confirm, my current code in the src/mono/mono/offsets directory should be updated to match the downloaded offset files, right?

@kg
Copy link
Member

kg commented Nov 13, 2024

I've never dealt with this problem before, but it appears that if you download each of the offsets artifacts I linked to, it will be a ZIP file with updated versions of the .h files in it. You would then check in these new .h files to update the offsets. I don't think you want to manually edit the offsets.

@steveisok
Copy link
Member

I've never dealt with this problem before, but it appears that if you download each of the offsets artifacts I linked to, it will be a ZIP file with updated versions of the .h files in it. You would then check in these new .h files to update the offsets. I don't think you want to manually edit the offsets.

Unless I'm completely mistaken, we don't auto-generate the offsets any longer and they need to be updated in tree.

@akoeplinger is that correct?

@akoeplinger
Copy link
Member

akoeplinger commented Nov 13, 2024

We generate them in PR builds so you can just download and check in the updated offsets, like @kg mentioned.

This is the message on the build, let me know if I can make it clearer:

Detected changes in Mono offsets files. When running in a PR build you can download the MonoAOTOffsets artifacts and update the files in the src/mono/mono/offsets folder.

edit I just noticed that the name of the artifacts in the message is not right, it's Mono_Offsets_<platform> right now

akoeplinger added a commit that referenced this pull request Nov 13, 2024
To match the message in the build log, see #109363 (comment)
@syfFerdinand
Copy link
Contributor Author

Thank you all for your assistance and guidance throughout this process. I have updated the offset files with the latest versions from the build artifacts and committed the changes.

Is there anything else that needs to be corrected or adjusted?

@kg
Copy link
Member

kg commented Nov 13, 2024

Once the necessary tests pass I think we should be good. I'll keep an eye on it.

@syfFerdinand
Copy link
Contributor Author

hi @kg,
I think the tests that are currently failing had been deactivated in this merge... #108178 so I don't understand why there's still this build error...

@kg
Copy link
Member

kg commented Nov 25, 2024

Out of memory issues on the wasm lanes are common, so I don't think the librarytests lane failure indicates a bug you created.

@kg kg merged commit 834de07 into dotnet:main Nov 25, 2024
67 of 70 checks passed
@kg
Copy link
Member

kg commented Nov 25, 2024

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[metadata] Move MonoClass:inlinearray_value out of MonoClass
5 participants