Skip to content

Allow skeletons to be reused in frozen-partial scenarios (for inheritance margin)#61286

Merged
CyrusNajmabadi merged 8 commits intodotnet:mainfrom
CyrusNajmabadi:frozenSkeleton
May 19, 2022
Merged

Allow skeletons to be reused in frozen-partial scenarios (for inheritance margin)#61286
CyrusNajmabadi merged 8 commits intodotnet:mainfrom
CyrusNajmabadi:frozenSkeleton

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

This PR now allows skeleton references to be reused in frozen-partial scenarios. Now, if we have a frozen partial project, and it has a p2p reference to a different language, we'll just reuse whatever skeleton we built for that language last, not caring if it is up to date or not with teh compilation for that project. This allows some amount of semantics to flow p2p, without taking on the full cost of producing the full skeleton.

The remote side of inheritance margin is also updated to use frozen-partial for the data it is operating on. This will hopefully get it to consume far less work producing skeletons in the p2p case.

@CyrusNajmabadi CyrusNajmabadi requested a review from sharwell May 13, 2022 17:55
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@sharwell for another look.

Note: as part of this PR i was finding the dataflow over different operations confusing. So i split this up into two remote calls. One for getting global imports, the other for getting symbol information.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@Cosifne @sharwell this is ready for review.

@CyrusNajmabadi CyrusNajmabadi requested a review from Cosifne May 19, 2022 04:41
{
// If we're starting from a document, use it to go to a frozen partial version of it to lower the amount of
// work we need to do running source generators or producing skeleton references.
if (document != null && frozenPartialSemantics)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel the 'frozenPartialSemantics' pattern is somewhat strange to me. (The flag seems always to be true)
Is this the pattern that Sam mentioned in the previous comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are correct. It is always true. It's just trying to be explicit on both endsThat it wants a frozen partial view of the solution :-)

Copy link
Copy Markdown
Member

@Cosifne Cosifne left a comment

Choose a reason for hiding this comment

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

LGTM except for the strange always true flag.
But if this is our pattern then that's fine : )

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge May 19, 2022 16:35
@CyrusNajmabadi CyrusNajmabadi dismissed sharwell’s stale review May 19, 2022 16:35

Feedback handled

@CyrusNajmabadi CyrusNajmabadi merged commit b438f9f into dotnet:main May 19, 2022
@ghost ghost added this to the Next milestone May 19, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the frozenSkeleton branch May 19, 2022 16:36
@Cosifne Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants