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.

Sign up for GitHub

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

Jump to bottom

Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag structure we have for all source methods #68158

Merged
merged 37 commits into from
May 16, 2023
Merged

Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag structure we have for all source methods #68158

merged 37 commits into from
May 16, 2023

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 10, 2023 •
edited
Loading

Drop 0.8% of memory from:

image

to

image

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 10, 2023
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 10, 2023 18:18
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 10, 2023 18:18
@CyrusNajmabadi CyrusNajmabadi marked this pull request as draft May 10, 2023 18:22
@CyrusNajmabadi
Copy link
Member Author

Draft until #68157 goes in.

CyrusNajmabadi and others added 2 commits May 10, 2023 12:40
@CyrusNajmabadi
Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag stru…
3e67b7c 
…cture we have for all source methods (dotnet#68132)

* In progresS

* Utilize

* initialize once

* Save space in other methods

* reorder

* reorder

* Move down

* Reorder

* Simplify

* Add docs

* move up
@CyrusNajmabadi
Compute IsVarArg up front
ce343a6
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 10, 2023 19:41
@CyrusNajmabadi
Copy link
Member Author

@AlekseyTs this is ready for review.

@CyrusNajmabadi
Copy link
Member Author

@333fred as well for the updated change at the end. Original PR is the first commit, revision is hte second commit.

@AlekseyTs
Copy link
Contributor

@CyrusNajmabadi I think the title of the PR should be changed to the title of the original PR.

@AlekseyTs
Copy link
Contributor

And the description should probably be copied as well

@CyrusNajmabadi CyrusNajmabadi changed the title Revert "Revert "Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag structure we have for all source methods Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag structure we have for all source methods May 10, 2023
AlekseyTs
AlekseyTs reviewed May 11, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs Outdated
@@ -285,7 +276,7 @@ protected override MethodSymbol FindExplicitlyImplementedMethod(BindingDiagnosti

protected override TypeSymbol ExplicitInterfaceType => _explicitInterfaceType;

protected override bool HasAnyBody => _hasAnyBody;
protected override bool HasAnyBody => flags.HasAnyBody;
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

protected override bool HasAnyBody => flags.HasAnyBody;

It feels like this property should be moved to SourceMemberMethodSymbol and made non-virtual. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure :)

AlekseyTs
AlekseyTs reviewed May 11, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs Outdated
@@ -161,7 +156,7 @@ internal sealed override ImmutableArray<string> NotNullWhenFalseMembers
// ReturnsVoid property is overridden in this class so
// returnsVoid argument to MakeFlags is ignored.
bool isExplicitInterfaceImplementation = property.IsExplicitInterfaceImplementation;
this.MakeFlags(MethodKind.PropertyGet, declarationModifiers, returnsVoid: false, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled,
this.MakeFlags(MethodKind.PropertyGet, declarationModifiers, returnsVoid: false, isExpressionBodied: true, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled,
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this.MakeFlags(MethodKind.PropertyGet, declarationModifiers, returnsVoid: false, isExpressionBodied: true, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled,

Why is it Ok to not initialize refKind, hasBody, isVarArg here? #Closed

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure how to answer this. What would make it not ok?

Afaict, these values are not stored for this part of the inheritance tree at all. They are stored for:

image

But a SourcePropertyAccessorSymbol is not a SourceOrdinaryMethodSymbol... so it's ok to not initialize... because it's not state you seem to care about?

(genuinely not sure what is being asked here).

@CyrusNajmabadi
Copy link
Member Author

Please, ping me once the PR is ready for another look.

This is ready for anohte rpass :)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 30)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 32)

AlekseyTs
AlekseyTs approved these changes May 13, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 34), modulo suggested comment adjustment.

@CyrusNajmabadi
Copy link
Member Author

@333fred @dotnet/roslyn-compiler could you take another look? Thanks!

AlekseyTs
AlekseyTs approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 36)

AlekseyTs
AlekseyTs approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 37)

@AlekseyTs
Copy link
Contributor

@CyrusNajmabadi Please squash commits while merging.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) May 16, 2023 01:40
@CyrusNajmabadi
Copy link
Member Author

@CyrusNajmabadi Please squash commits while merging.

Yup yup!

@CyrusNajmabadi CyrusNajmabadi merged commit 35d72b8 into dotnet:main May 16, 2023
@ghost ghost added this to the Next milestone May 16, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the revertRevert branch May 16, 2023 02:06
@allisonchou allisonchou mentioned this pull request May 18, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.7 P2
Development

Successfully merging this pull request may close these issues.
4 participants
@CyrusNajmabadi @AlekseyTs @333fred @Cosifne