Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

This is an alternative approach to #41589 (which is more contentious in the scope of hte change).

This only gets us about 50% of hte improvement of that change (65 frames instead of around 130). But it should also be much easier to get in immediately.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 13, 2020 17:37
@CyrusNajmabadi
Copy link
Member Author

Tagging @RikkiGibson @jcouv @cston Thanks!

@cston
Copy link
Contributor

cston commented Feb 13, 2020

This only gets us about 50% of ...

As @RikkiGibson suggested in #41589, if [MethodImpl(MethodImplOptions.AggressiveInlining)] is added to ParseEmbeddedStatement, it should be possible to get the full benefit with this change.

Copy link
Contributor

@cston cston left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@CyrusNajmabadi
Copy link
Member Author

As @RikkiGibson suggested in #41589, if [MethodImpl(MethodImplOptions.AggressiveInlining)] is added to ParseEmbeddedStatement, it should be possible to get the full benefit with this change.

Ok. Let me try that!

Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

does this now get us basically the same benefit as #41589?

@RikkiGibson RikkiGibson added Community The pull request was submitted by a contributor who is not a Microsoft employee. auto-merge labels Feb 13, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost removed the auto-merge label Feb 13, 2020
Copy link
Contributor

@cston cston left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@RikkiGibson
Copy link
Member

Seems like a spurious failure in debug_32

Microsoft.CodeAnalysis.Editor.UnitTests.Diagnostics.GenerateMethod.GenerateMethodCrossLanguageTests.GenerateMethodUsingTypeConstraint_InstantiatedGenerics

System.AggregateException : One or more errors occurred.\r\n---- System.InvalidOperationException : WaitAndGetResult cannot be called from a thread pool thread.

@CyrusNajmabadi
Copy link
Member Author

Seems like a spurious failure in debug_32

Looks like it. What's the right thing to do here? Open an issue and ignore the failure as a dupe of that?

@RikkiGibson
Copy link
Member

Opened an issue to track the flaky test

@ghost ghost merged commit 2b5fa1d into dotnet:master Feb 13, 2020
@CyrusNajmabadi
Copy link
Member Author

Thanks!

@CyrusNajmabadi CyrusNajmabadi deleted the embeddedParsingStack branch February 13, 2020 22:44
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers auto-merge Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants