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

Allow suppressing emit of localsinit flag via an attribute. #1223

Merged
merged 2 commits into from
Jan 22, 2018

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Dec 28, 2017

It does not seem to be big enough to be a real C# language feature (as in "required by all conforming C# implementations"), but we can have it as a compiler feature where compiler may optionally recognize a specific attribute as a directive/hint to omit the flag.

There are few scenarios where forced zero-initialization may noticeably impact performance.

The feature could be implemented as a compiler switch - similar to /optimize, but it could be more valuable when can be applied more selectively to specific methods or types.

@VSadov
Copy link
Member Author

VSadov commented Dec 28, 2017

CC: @jkotas @jaredpar @KrzysztofCwalina

@VSadov
Copy link
Member Author

VSadov commented Dec 28, 2017

This is related to the dotnet/roslyn#23951

One change would help another.

## Unresolved questions
[unresolved]: #unresolved-questions

- Should the attribute be actually emitted to metadata?
Copy link
Member

@jkotas jkotas Dec 28, 2017

Choose a reason for hiding this comment

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

Are we going to add the attribute to public surface so that folks can use it without local definition? (I assume that local internal per-assembly definition would be possible as well.)

Copy link
Member Author

@VSadov VSadov Dec 28, 2017

Choose a reason for hiding this comment

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

Compiler does not care about the identity of attribute types, so it can be defined anywhere including an internal type in source.
Even though it is not a requirement, adding the attribute to public surface might be useful.

I did not mention that intentionally, to not get into matters of nuget packages, reference assemblies, netstandard v-next and the like. - To defer to people who know better what treatment would be appropriate in this case.
I should add this to "unresolved questions" though.


## Detailed design

Allow specifying `System.Runtime.CompilerServices.SkipLocalsInitAttribute` as a way to tell the compiler to not emit `localsinit` flag.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would a better name of the attribute be NoLocalsInitAttribute?

I believe that attribute names do not typically start with a verb.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be ok with any attribute name as long as it tells that it is suppressing localsinit flag - because that is all what it does.

There might be more suggestions for the attribute name from which we can chose.

Copy link
Member

Choose a reason for hiding this comment

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

Any reason we can't add this as a flag to MethodImplOptions?

Copy link
Member

Choose a reason for hiding this comment

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

Ex: [MethodImpl(MethodImplOptions.NoLocalsInit)]

It would fit in well with the existing usage of MethodImplOptions. I one of the drawbacks would be that MethodImplAttribute is only Constructor | Method currently.

Copy link
Member

Choose a reason for hiding this comment

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

MethodImplOptions are understood by the runtime, not by the compiler. Would the compiler be responsible for masking this bit out before saving it to the binary?

Copy link
Member Author

@VSadov VSadov Dec 28, 2017

Choose a reason for hiding this comment

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

I thought about MethodImplOptions.
It is attractive, since there is no need for a new type, however MethodImpl can only target methods or constructors and expanding its applicability would not be trivial since it impacts run/jit time behavior as well.

I will list this in "alternative solutions".
If the idea of allowing the attribute on method containers falls through for some reason, then this will be a viable option.

Copy link
Member Author

Choose a reason for hiding this comment

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

The metadata emit is listed as one of the items that needs to be discussed further.
Attributes that have compile-time effect at declaration is a new scenario to us.

IMO - Compiler will likely not interfere with how attribute is emitted. At least to not cause versioning/compat problems when switching between compilers.

That could be an issue with MethodImplOptions since MethodImpl is a "pseudoattribute" and options are emitted into bit fields in the MethodImpl metadata table. Not sure how a new bit would fit there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Attributes that have compile-time effect at declaration is a new scenario to us.

What about [Conditional]? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@Joe4evr [Conditional] has effect at the place where method is called, but at the point of declaration (like where Debug.Assert is declared) it is just an attribute - it gets emitted to metadata.

@VSadov
Copy link
Member Author

VSadov commented Jan 22, 2018

Merging this, since this has been opened for a long time, and has approvals (as a viable proposal).

NOTE: The proposal does not have direct impact on the language, but has effect on the compiler.
(that is why it fits in csharplang repo)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants