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

[Tracking] Mono support for InlineArrayAttribute #80798

Closed
4 of 5 tasks
lambdageek opened this issue Jan 18, 2023 · 10 comments
Closed
4 of 5 tasks

[Tracking] Mono support for InlineArrayAttribute #80798

lambdageek opened this issue Jan 18, 2023 · 10 comments
Assignees
Labels
area-VM-meta-mono tracking This issue is tracking the completion of other related issues.
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Jan 18, 2023

This issue tracks the work for implementing Mono support for InlineArrayAttribute #61135 and the related C# features such as params ReadOnlySpan, safe fixed size buffers, stackalloc of reference types, etc.

Required work: Completed in .NET8

Nice to have: Future Release.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 18, 2023
@lambdageek lambdageek added area-VM-meta-mono tracking This issue is tracking the completion of other related issues. and removed untriaged New issue has not been triaged by the area owner labels Jan 18, 2023
@lambdageek lambdageek added this to the 8.0.0 milestone Jan 18, 2023
@lambdageek
Copy link
Member Author

/cc @SamMonoRT @kotlarmilos @thaystg

@am11
Copy link
Member

am11 commented Jan 18, 2023

Does this supersede #48113?

@lambdageek
Copy link
Member Author

Does this supersede #48113?

Good catch - I forgot about that one. 🥲
But also I think the details of the API proposal changed enough that it's worth tracking in a new issue.

@lambdageek
Copy link
Member Author

lambdageek commented Jan 19, 2023

One interesting technical challenge is how to extract the Length property from InlineArrayAttribute without triggering class loading and managed object construction for possibly unrelated custom attributes on every class.

We have already a custom attribute iterator mono_class_metadata_foreach_custom_attr that avoids triggering assembly loads and so on. It takes a callback that gets the method token of the constructor and the namespace and name of the custom attribute. I think what we need to do is to also pass to it the custom attribute row index so that it can find the blob data and length.

Roughly like this:

  1. Extend mono_class_metadata_foreach_custom_attr to pass the custom attribute row index to the MonoAssemblyMetadatacustomAttrIterFunc callback.
  2. Extract the loop body in mono_custom_attrs_from_index_checked into a helper function:
    void mono_metadata_custom_attrs_get_blob_info (MonoImage *image, uint32_t custom_attr_idx, MonoMethod **out_ctor, uint32_t *out_blob_size, const char** out_blob) that gets passed a custom attribute row index and returns the MonoMethod a blob size and the start of the blob that encodes the custom attribute data in that one row in the custom attribute table.
  3. Then call mono_reflection_create_custom_attr_data_args_noalloc passing it the ctor, blob size and blob data pointer - this will return a data structure containing the property values.

It might also be possible to just pass the blob pointer directly to the MonoAsemblyMetadataCustomAttrIterFunc callback directly from mono_class_metadata_foreach_custom_attr without bothering with the custom attribute row index - all the relevant info (method token for the ctor, blob data size, blob data ptr) should be retrievable by mono_class_metadata_foreach_custom_attr. Then if we know we're looking at InlineArrayAttribute, we can call mono_method_get_method_checked on the method token to get a MonoMethod* for the ctor and then just pass all the right args to mono_reflection_create_custom_attr_data_args_noalloc directly in the iterator callback.

@kotlarmilos
Copy link
Member

kotlarmilos commented Jan 24, 2023

metadata_foreach_custom_attr_from_index has inlined loop of mono_custom_attrs_from_index_checked which makes it easy to create MonoCustomAttrEntry struct and pass it to the callback function, where if the attribute exists mono_reflection_create_custom_attr_data_args_noalloc is invoked. Does it make sense to extract the loop body in mono_custom_attrs_from_index_checked into mono_metadata_custom_attrs_get_blob_info which returns MonoCustomAttrEntry * instead of out arguments?

The current PoC retrieves the Length property from InlineArrayAttribute in metadata_foreach_custom_attr_from_index. As the function iterates through all attributes, it might be inefficient. Also, for further extensibility I suggest adding a helper function with an array of attributes that has a value, like gboolean attr_has_value (char *attr_name)

@lambdageek
Copy link
Member Author

metadata_foreach_custom_attr_from_index has inlined loop of mono_custom_attrs_from_index_checked which makes it easy to create MonoCustomAttrEntry struct and pass it to the callback function, where if the attribute exists mono_reflection_create_custom_attr_data_args_noalloc is invoked. Does it make sense to extract the loop body in mono_custom_attrs_from_index_checked into mono_metadata_custom_attrs_get_blob_info which returns MonoCustomAttrEntry * instead of out arguments?

The current PoC retrieves the Length property from InlineArrayAttribute in metadata_foreach_custom_attr_from_index. As the function iterates through all attributes, it might be inefficient. Also, for further extensibility I suggest adding a helper function with an array of attributes that has a value, like gboolean attr_has_value (char *attr_name)

I think this is generally the right direction to head in, but there are a couple of issues:

  1. I don't think it's a good idea for metadata_foreach_custom_attr_from_index to know anything about InlineArrayAttribute. Ideally it's just a generic traversal mechanism. The logic to deal with InlineArrayAttribute should be part of the callback that is passed to foreach by the class-init.c code.
  2. MonoCustomAttrEntry is not a great struct to use for this particular case. The issue is that to create a MonoMethod from a method token, we have to create and initialize the class in which the method is defined; and that may require us to load the assembly in which the class is defined. In the code, as you have it, we only try to load InlineArrayAttribute's ctor, but if we make foreach less specialized, it would not be right to try to create a MonoMethod for every attribute that we see.

So I really think more should be delegated to the callback

But overall the rest of the structure looks reasonable

@SamMonoRT
Copy link
Member

Moving to 9.0.0

@SamMonoRT SamMonoRT modified the milestones: 8.0.0, 9.0.0 Aug 11, 2023
@kotlarmilos kotlarmilos modified the milestones: 9.0.0, Future Feb 9, 2024
@kotlarmilos
Copy link
Member

@lambdageek Can we close this issue?

@lambdageek
Copy link
Member Author

Yep. the planned .NET 9 work is done. future work (e.g. making known custom attribute lookup faster) is tracked in follow up issues

@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-meta-mono tracking This issue is tracking the completion of other related issues.
Projects
None yet
Development

No branches or pull requests

4 participants